Re: [PATCH net v6 4/4] macsec: Support VLAN-filtering lower devices

[Cosmin Ratiu <cratiu@nvidia.com>]

On Thu, 2026-04-02 at 16:48 +0200, Sabrina Dubroca wrote:
> 2026-03-30, 16:01:30 +0300, Cosmin Ratiu wrote:
> > @@ -2616,14 +2616,22 @@ static int macsec_update_offload(struct
> > net_device *dev, enum macsec_offload off
> >  	if (!ops)
> >  		return -EOPNOTSUPP;
> >  
> > -	macsec->offload = offload;
> > -
> >  	ctx.secy = &macsec->secy;
> >  	ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops-
> > >mdo_del_secy, &ctx)
> >  					    : macsec_offload(ops-
> > >mdo_add_secy, &ctx);
> > -	if (ret) {
> > -		macsec->offload = prev_offload;
> > +	if (ret)
> >  		return ret;
> > +
> > +	/* 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);
> 
> Paolo pointed me to the sashiko review for this patch
> https://sashiko.dev/#/patchset/20260330130130.989236-1-cratiu%40nvidia.com

I should make it a habit to go and look up these reports myself.

> 
> A simple way to trigger this is to do s/VLAN_N_VID/500/ in
> nsim_vlan_rx_*_vid.
> 
> For example:
> 
> echo 1 > /sys/bus/netdevsim/new_device
> ip link add link eni1np1 macsec0 type macsec
> ip link add link macsec0 macsec0.1 type vlan id 1
> ip link add link macsec0 macsec0.1000 type vlan id 1000
> ip link set macsec0 type macsec offload mac
> cat /sys/kernel/debug/netdevsim/netdevsim1/ports/0/vlan      # empty
> 
> 
> If this happens on a real device, the VLAN filters will be broken.
> I'm
> not sure what the right behavior would be:
> 
> 1. reject the request to enable offload
> 2. switch to promiscuous mode

I implemented and tested option 1. In the unlikely scenario adding VLAN
filters prevents offloading, it's better for the driver to be explicit
and let the user turn on promisc mode themselves. Keeping track of
whether VLAN filters failed and promisc was used as a fallback adds
some extra complexity. Plus, I am not sure it is the right place for
this decision. What would be the point of IFF_UNICAST_FLT then?

Please let me know if you agree with this approach, so I can send v8
with it.

> OTOH maybe we don't need to care, since __netdev_update_features also
> (kind of) ignores those errors:
> 
> echo 1 > /sys/bus/netdevsim/new_device
> ethtool -K eni1np1 rx-vlan-filter off
> ip link add link eni1np1 eni1np1.1 type vlan id 1
> ip link add link eni1np1 eni1np1.1000 type vlan id 1000
> cat /sys/kernel/debug/netdevsim/netdevsim1/ports/0/vlan      # empty
> as expected
> ethtool -K eni1np1 rx-vlan-filter on                         #
> succeeds
> ethtool -k eni1np1 | grep rx-vlan-filter                     # "rx-
> vlan-filter: on"
> cat /sys/kernel/debug/netdevsim/netdevsim1/ports/0/vlan      # still
> empty because id=1000 was rejected
>                                                              # and
> everything got rolled back
> ip link add link eni1np1 eni1np1.123 type vlan id 123        #
> succeeds
> cat /sys/kernel/debug/netdevsim/netdevsim1/ports/0/vlan      # only
> "ctag 123"
> 
> 
> [at this point running
>     ip link del eni1np1.1
> or
>     ethtool -K eni1np1 rx-vlan-filter off
> will splat because vlan_filter_push_vids did a rollback/never added
> id=1, and now we call vlan_kill_rx_filter_info, but that's specific
> to
> this vid limit]
> 

Well, in this case we have the chance to do something nicer (even
proper error message back to the user via extack) for a small
complexity cost. Perhaps the VLAN filter handling could be improved
separately.

Cosmin.