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

[Jakub Kicinski <kuba@kernel.org>]

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
> device (and thus the lower device) was put in promiscuous mode and the
> VLAN filter was not used. But after commit [1] correctly made the macsec
> driver expose the IFF_UNICAST_FLT flag, promiscuous mode was no longer
> used and VLAN filters on dev 1 kicked in. Without support in dev 2 for
> propagating VLAN filters down, the register_vlan_dev -> vlan_vid_add ->
> __vlan_vid_add -> vlan_add_rx_filter_info call from dev 3 is silently
> eaten (because vlan_hw_filter_capable returns false and
> vlan_add_rx_filter_info silently succeeds).
> 
> For macsec, VLAN filters are only relevant for offload, otherwise
> the VLANs are encrypted and the lower devices don't care about them. So
> VLAN filters are only passed on to lower devices in offload mode.
> Flipping between offload modes now needs to offload/unoffload the
> filters with vlan_{get,drop}_rx_*_filter_info().

AI says:

This commit implements VLAN-filtering support for macsec devices by
adding ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid callbacks and
advertising the appropriate netdev features when the lower device
supports VLAN filtering.

> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index f6cad0746a02..73e0b75d9b69 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -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:

	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)
		goto rollback;

	...

rollback:
	if (ret) {
		macsec->offload = prev_offload;
		return ret;
	}

The error path restores macsec->offload to prev_offload (MACSEC_OFFLOAD_OFF)
but doesn't call vlan_drop_rx_ctag_filter_info(dev) and
vlan_drop_rx_stag_filter_info(dev) to remove the VLAN filters that were
already pushed to the lower device. This leaves stale VLAN filter entries on
real_dev.

Since macsec->offload is now back to MACSEC_OFFLOAD_OFF, future calls to
macsec_vlan_rx_kill_vid will return early without removing these VIDs, so the
leaked state persists until the lower device is brought down.

[ ... ]