Re: [PATCH net-next v2 3/3] macsec: Support VLAN-filtering lower devices

[Cosmin Ratiu <cratiu@nvidia.com>]

On Sat, 2026-02-28 at 12:27 +0100, Sabrina Dubroca wrote:
> 2026-02-27, 06:59:08 -0800, Jakub Kicinski wrote:
> > On Fri, 27 Feb 2026 11:02:27 +0200 Cosmin Ratiu wrote:
> > > VLAN-filtering is done through two netdev features
> > > (NETIF_F_HW_VLAN_CTAG_FILTER and NETIF_F_HW_VLAN_STAG_FILTER) and
> > > two
> > > netdev ops (ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid).
> > > 
> > > Implement these and advertise the features if the lower device
> > > supports
> > > them. This allows proper VLAN filtering to work on top of macsec
> > > devices, when the lower device is capable of VLAN filtering.
> > > As a concrete example, having this chain of interfaces now works:
> > > vlan_filtering_capable_dev(1) -> macsec_dev(2) ->
> > > macsec_vlan_dev(3)
> > > 
> > > Before commit [1] this used to accidentally work because the
> > > macsec
> 
> The first submission was for net-next, then Jakub asked to make this
> a
> fix for net:
> https://lore.kernel.org/netdev/20260106171027.57a7757f@kernel.org/
> which makes sense.
> 
> So "v1"
> https://lore.kernel.org/netdev/20260107104723.2750725-1-cratiu@nvidia.com
> was for net, and we discussed making it a net-next patch because the
> changes were looking invasive. But in the end by using
> vlan_{get,drop}_rx_*_filter_info it's a pretty simple patch, so I
> think this should still be for net?

You're right. I ended up thinking the complexity is higher than average
for a net fix, but it's mostly in test infra. Apologies for the back-
and-forth.

> [...]
> > > @@ -2616,7 +2616,17 @@ static int macsec_update_offload(struct
> > > net_device *dev, enum macsec_offload off
> > >  	if (!ops)
> > >  		return -EOPNOTSUPP;
> > > 
> > > +	/* Remove VLAN filters when disabling offload. */
> > > +	if (offload == MACSEC_OFFLOAD_OFF) {
> > > +		vlan_drop_rx_ctag_filter_info(dev);
> > > +		vlan_drop_rx_stag_filter_info(dev);
> > > +	}
> > >  	macsec->offload = offload;
> > > +	/* Add VLAN filters when enabling offload. */
> > > +	if (prev_offload == MACSEC_OFFLOAD_OFF) {
> > > +		vlan_get_rx_ctag_filter_info(dev);
> > > +		vlan_get_rx_stag_filter_info(dev);
> > > +	}
> > > 
> > >  	ctx.secy = &macsec->secy;
> > >  	ret = offload == MACSEC_OFFLOAD_OFF ?
> > > macsec_offload(ops->mdo_del_secy, &ctx)
> > > @@ -2633,6 +2643,11 @@ static int macsec_update_offload(struct
> > > net_device *dev, enum macsec_offload off
> > > 
> > >  	if (ret) {
> > >  		macsec->offload = prev_offload;
> > > +		if (offload == MACSEC_OFFLOAD_OFF &&
> > > prev_offload == MACSEC_OFFLOAD_MAC) {
> > > +			vlan_get_rx_ctag_filter_info(dev);
> > > +			vlan_get_rx_stag_filter_info(dev);
> > > +		}
> > > +
> > >  		return ret;
> > >  	}
> > 
> > Does the error path properly restore VLAN filter state when
> > enabling offload
> > fails? When prev_offload is MACSEC_OFFLOAD_OFF and the code calls
> > vlan_get_rx_ctag_filter_info(dev) and
> > vlan_get_rx_stag_filter_info(dev) to
> > push VLAN filters to the lower device, but then macsec_offload()
> > fails and
> > returns an error:
> [...]
> 
> Looks like it. Since the vlan_*_filter_info ops can't fail, just move
> them until after we've called mdo_*_secy, and macsec_update_offload
> can't fail anymore?

Will do.

Cosmin.