[Intel-wired-lan] [PATCH net] ice: roll back the port VLAN when ice_eswitch_br_set_pvid() fails

[Intel-wired-lan] [PATCH net] ice: roll back the port VLAN when ice_eswitch_br_set_pvid() fails

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_eswitch_br_set_pvid() puts the VF VSI into port VLAN mode with
ice_vf_vsi_enable_port_vlan() before it programs the port VLAN itself.
Both error paths below that point return the error code straight away,
so nothing is undone when set_port_vlan() or add_vlan() fails.

The stale port VLAN is the more visible half of the problem.
set_port_vlan() turns on Rx VLAN pruning in the VSI context and relies
on the add_vlan() call right after it to install the matching prune
filter, so a failing add_vlan() leaves the VF VSI pruning away every
packet it receives, while port->pvid stays 0 and the bridge is told the
VLAN was not offloaded.

The VLAN ops table is never restored either.
ice_vf_vsi_disable_port_vlan() is only ever called from
ice_eswitch_br_clear_pvid(), which is reached from
ice_eswitch_br_vlan_cleanup() only when port->pvid matches the VLAN
being removed. Neither the failed VLAN, which was never inserted into
port->vlans, nor a port->pvid value ever got that far, so the VSI keeps
the port VLAN flavour of its inner and outer vlan_ops until the VF VSI 
is rebuilt:
dis_rx_filtering() turns into a no-op and, in Double VLAN Mode, the
inner add_vlan() and del_vlan() stop doing anything.

Unwind both steps the way ice_eswitch_br_clear_pvid() does: clear the
port VLAN from the VSI context when it was already programmed, then
hand the VLAN ops back to ice_port_vlan_off().

Fixes: 2946204b3fa8 ("ice: implement bridge port vlan")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ice/ice_eswitch_br.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
index 1d8a6b95ccda..53ce98ba7007 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
@@ -679,16 +679,23 @@ ice_eswitch_br_set_pvid(struct ice_esw_br_port *port,
 	vlan_ops = ice_get_compat_vsi_vlan_ops(port->vsi);
 	err = vlan_ops->set_port_vlan(port->vsi, &port_vlan);
 	if (err)
-		return err;
+		goto err_disable_port_vlan;
 
 	err = vlan_ops->add_vlan(port->vsi, &port_vlan);
 	if (err)
-		return err;
+		goto err_clear_port_vlan;
 
 	ice_eswitch_br_port_vlans_flush(port);
 	port->pvid = vlan->vid;
 
 	return 0;
+
+err_clear_port_vlan:
+	vlan_ops->clear_port_vlan(port->vsi);
+err_disable_port_vlan:
+	ice_vf_vsi_disable_port_vlan(port->vsi);
+
+	return err;
 }
 
 static struct ice_esw_br_vlan *
-- 
2.25.1

Re: [Intel-wired-lan] [PATCH net] ice: roll back the port VLAN when ice_eswitch_br_set_pvid() fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete rollback of the VLAN ops table: the commit message
  states the patch fixes the case where "in Double VLAN Mode, the inner…
- [High] ice_eswitch_br_set_pvid() mutates and dispatches through the
  per-VSI VLAN ops function-pointer table while holding only rtnl_lock,…
- [Medium] The newly added err_clear_port_vlan label in
  ice_eswitch_br_set_pvid()…
- [Medium] The new unwind assumes the VF VSI was not already in port-VLAN
  mode before ice_eswitch_br_set_pvid() ran, but…

--

ice: roll back the port VLAN when ice_eswitch_br_set_pvid() fails

This adds two unwind labels to ice_eswitch_br_set_pvid() so that a failing
set_port_vlan() or add_vlan() clears the port VLAN from the VSI context and
hands the VLAN ops back to ice_port_vlan_off(), instead of returning the
error with the VF VSI left in port VLAN mode.

A few questions about the new unwind path below.

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> index 1d8a6b95ccda..53ce98ba7007 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch_br.c
> @@ -679,16 +679,23 @@ ice_eswitch_br_set_pvid(struct ice_esw_br_port *port,
>  	vlan_ops = ice_get_compat_vsi_vlan_ops(port->vsi);
>  	err = vlan_ops->set_port_vlan(port->vsi, &port_vlan);
>  	if (err)
> -		return err;
> +		goto err_disable_port_vlan;
>  
>  	err = vlan_ops->add_vlan(port->vsi, &port_vlan);
>  	if (err)
> -		return err;
> +		goto err_clear_port_vlan;
>  
>  	ice_eswitch_br_port_vlans_flush(port);
>  	port->pvid = vlan->vid;
>  
>  	return 0;
> +
> +err_clear_port_vlan:
> +	vlan_ops->clear_port_vlan(port->vsi);

[Severity: Medium]
Is it intentional to drop the return value of clear_port_vlan() here?  The
op is declared fallible in struct ice_vsi_vlan_ops:

	int (*clear_port_vlan)(struct ice_vsi *vsi);

and both implementations can fail.  ice_vsi_clear_outer_port_vlan() and
ice_vsi_clear_inner_port_vlan() start with:

	ctxt = kzalloc_obj(*ctxt);
	if (!ctxt)
		return -ENOMEM;

	ice_restore_vlan_info(&vsi->info, &vsi->vlan_info);
	vsi->info.port_based_outer_vlan = 0;

and end with:

	err = ice_update_vsi(hw, vsi->idx, ctxt, NULL);
	if (err)
		dev_err(...);

So on an ice_update_vsi() failure vsi->info has already been mutated while
hardware keeps the port VLAN and the RX_VLAN_PRUNE_ENA bit programmed by
the preceding set_port_vlan(), with no matching prune filter (add_vlan() is
the call that failed).

Since port->pvid stays 0 and the VLAN was never inserted into port->vlans,
can any later path retry that clear?  ice_eswitch_br_vlan_cleanup() only
reaches ice_eswitch_br_clear_pvid() when:

	if (port->pvid == vlan->vid)
		ice_eswitch_br_clear_pvid(port);

The ops table is then handed back unconditionally, which also restores the
real dis_rx_filtering().  ice_port_vlan_on() deliberately replaces it while
a port VLAN is active:

	/* all Rx traffic should be in the domain of the assigned port VLAN,
	 * so prevent disabling Rx VLAN filtering
	 */
	vlan_ops->dis_rx_filtering = noop_vlan;

Could the VF then use virtchnl to clear the prune bit while the hardware
port VLAN is still programmed?  Would it be better to only call
ice_vf_vsi_disable_port_vlan() when clear_port_vlan() succeeded, and log or
propagate the failure otherwise?

[Severity: High]
Can vlan_ops->clear_port_vlan be NULL by the time this new call runs?

vlan_ops points at the live table (&vsi->outer_vlan_ops or
&vsi->inner_vlan_ops), not a snapshot, and clear_port_vlan only exists
while the port VLAN flavour is installed by ice_port_vlan_on():

	vlan_ops->set_port_vlan = ice_vsi_set_outer_port_vlan;
	vlan_ops->clear_port_vlan = ice_vsi_clear_outer_port_vlan;

ops_unsupported in ice_vsi_vlan_ops.c has no .clear_port_vlan initializer,
and ice_port_vlan_off() never assigns it either.

The bridge path holds only rtnl (ice_eswitch_br_event_blocking ->
ice_eswitch_br_port_obj_add -> ice_eswitch_br_port_vlan_add ->
ice_eswitch_br_vlan_create -> ice_eswitch_br_set_pvid) and
ice_eswitch_br.c never takes vf->cfg_lock, while set_port_vlan() and
add_vlan() sleep (kzalloc(GFP_KERNEL) plus an admin queue wait).

A VF-initiated reset runs concurrently under a disjoint lockset:

ice_vc_reset_vf()/ice_process_vflr_event()
  ice_reset_vf()                      /* vf->cfg_lock + pf->lag_mutex */
    ice_vf_reconfig_vsi()
      ice_vsi_cfg()
        ice_vsi_cfg_def()
          ice_vsi_init_vlan_ops()     /* reinstalls ops_unsupported */

If that lands inside the window, does the new err_clear_port_vlan label
dereference a NULL function pointer?  The same missing serialization also
lets a virtchnl handler fetch one flavour of the ops and execute it after
the unwind flipped the table back.

> +err_disable_port_vlan:
> +	ice_vf_vsi_disable_port_vlan(port->vsi);

[Severity: High]
The commit message says this hand-back addresses the case where "in Double
VLAN Mode, the inner add_vlan() and del_vlan() stop doing anything".  Does
ice_port_vlan_off() actually restore those two?

ice_port_vlan_on() sets them in DVM:

	vlan_ops = &vsi->inner_vlan_ops;

	if (ice_is_dvm_ena(&pf->hw)) {
		vlan_ops->add_vlan = noop_vlan_arg;
		vlan_ops->del_vlan = noop_vlan_arg;
		...

ice_port_vlan_off() re-points its local vlan_ops at the outer table before
it assigns del_vlan, and never touches the inner add_vlan at all:

	vlan_ops = &vsi->inner_vlan_ops;

	vlan_ops->ena_stripping = ice_vsi_ena_inner_stripping;
	...
	if (ice_is_dvm_ena(&pf->hw)) {
		vlan_ops = &vsi->outer_vlan_ops;

		vlan_ops->del_vlan = ice_vsi_del_vlan;
		...

So after the unwind a DVM VF VSI keeps inner add_vlan/del_vlan ==
noop_vlan_arg, where ice_vsi_init_unsupported_vlan_ops() installs
op_unsupported_vlan_arg for a DVM VF VSI without a port VLAN.

Those pointers are live for the VF: ice_vc_process_vlan_msg() calls

	status = vsi->inner_vlan_ops.add_vlan(vsi, &vlan);

and the matching .del_vlan() for legacy VIRTCHNL_OP_ADD_VLAN/DEL_VLAN, with
DVM explicitly allowed there.  Is the VF's C-tag filter add/del then
reported as success while ice_fltr_add_vlan() is never issued and
vsi->num_vlan is not updated, until the VF VSI is rebuilt?

Should ice_port_vlan_off() also restore the inner add_vlan/del_vlan, which
would additionally cover the same residue on the existing
ice_eswitch_br_clear_pvid() path, or should the commit message be narrowed
to what ice_port_vlan_off() really restores?

[Severity: Medium]
What happens here when the VF VSI was already in port VLAN mode before
ice_eswitch_br_set_pvid() ran?

ice_vf_vsi_enable_port_vlan() is a plain ops-table overwrite with no
nesting or refcount:

	void ice_vf_vsi_enable_port_vlan(struct ice_vsi *vsi)
	{
		if (WARN_ON_ONCE(!vsi->vf))
			return;

		ice_port_vlan_on(vsi);
	}

ice_set_vf_port_vlan() has no switchdev or bridge guard, it just records
the VLAN and resets the VF:

	vf->port_vlan_info = ICE_VLAN(local_vlan_proto, vlan_id, qos);
	...
	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);

after which ice_vf_vsi_init_vlan_ops() calls ice_port_vlan_on() because
ice_vf_is_port_vlan_ena(vf) is true, and
ice_vf_rebuild_host_vlan_cfg() programs it:

	if (ice_vf_is_port_vlan_ena(vf)) {
		err = vlan_ops->set_port_vlan(vsi, &vf->port_vlan_info);

ice_eswitch_br_set_pvid() only inspects the bridge-tracked port->pvid,
which is still 0 in that configuration, so it proceeds.  On a failing
set_port_vlan() the new path calls ice_port_vlan_off() while the SR-IOV
port VLAN is still active in hardware and vf->port_vlan_info is still set,
which makes dis_rx_filtering the real ice_vsi_dis_rx_vlan_filtering()
again.  On a failing add_vlan() the clear additionally zeroes
vsi->info.port_based_{outer,inner}_vlan.

Does the unwind need to restore the actual prior state rather than assume
"port VLAN off" was the baseline?

> +
> +	return err;
>  }
>  
>  static struct ice_esw_br_vlan *

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260920064754.1935818-1-xiaolinkui%40126.com