Re: [PATCH v3 net-next 3/6] vxlan: move VXLAN_F_MDB to struct vxlan_dev flags

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
vxlan: move VXLAN_F_MDB to struct vxlan_dev flags

This moves the internal VXLAN_F_MDB runtime state out of struct
vxlan_config.flags into a new unsigned long flags field in struct
vxlan_dev, accessed with set_bit()/clear_bit()/test_bit(), in
preparation for making vxlan->cfg an RCU-protected pointer.

One question about an out-of-tree-of-this-patch consumer of
cfg->flags below.

> diff --git a/drivers/net/vxlan/vxlan_mdb.c b/drivers/net/vxlan/vxlan_mdb.c
> index 56ca928328330..cf606256d0929 100644
> --- a/drivers/net/vxlan/vxlan_mdb.c
> +++ b/drivers/net/vxlan/vxlan_mdb.c
> @@ -1219,7 +1219,7 @@ vxlan_mdb_entry_get(struct vxlan_dev *vxlan,
>  		goto err_free_entry;
>  
>  	if (hlist_is_singular_node(&mdb_entry->mdb_node, &vxlan->mdb_list))
> -		vxlan->cfg.flags |= VXLAN_F_MDB;
> +		set_bit(VXLAN_DEV_F_MDB, &vxlan->flags);

[Severity: Medium]
Does this change relax the mlxsw NVE offload validation?

mlxsw checks cfg->flags with a deny-by-default mask, so the 0x40000 bit
used to land in the "unsupported flag" test:

drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c:
#define MLXSW_SP_NVE_VXLAN_IPV4_SUPPORTED_FLAGS (VXLAN_F_UDP_ZERO_CSUM_TX | \
						 VXLAN_F_LEARN | \
						 VXLAN_F_LOCALBYPASS)

static bool mlxsw_sp_nve_vxlan_ipv4_flags_check(const struct vxlan_config *cfg,
						struct netlink_ext_ack *extack)
{
	...
	if (cfg->flags & ~MLXSW_SP_NVE_VXLAN_IPV4_SUPPORTED_FLAGS) {
		NL_SET_ERR_MSG_MOD(extack, "VxLAN: Unsupported flag");
		return false;
	}

The same mask exists for the IPv6 underlay in
mlxsw_sp_nve_vxlan_ipv6_flags_check(), and both are reached from
mlxsw_sp_nve_vxlan_can_offload(), which only looks at &vxlan->cfg.

So for a device that already has an MDB entry:

  bridge mdb add dev vxlan0 port vxlan0 grp 239.1.1.1 permanent \
      dst 198.51.100.1
    -> vxlan_mdb_add() -> __vxlan_mdb_add() -> vxlan_mdb_entry_get()

before this patch that set VXLAN_F_MDB in cfg->flags, and a later
enslavement to an mlxsw-offloaded bridge was refused with "VxLAN:
Unsupported flag". After the move, mlxsw_sp_nve_vxlan_can_offload() has
no way to observe the MDB state, since nothing outside vxlan_core.c and
vxlan_mdb.c tests VXLAN_DEV_F_MDB, and the device is accepted for
offload while the configured MDB entries are not honoured by the
hardware forwarding plane.

Note the previous rejection was already order-dependent: MDB entries
added after the FID was enabled were never re-validated, because vxlan
MDB emits no switchdev notification at all (git grep switchdev
drivers/net/vxlan/ matches only vxlan_core.c).

Would it make sense to add a test_bit(VXLAN_DEV_F_MDB, &vxlan->flags)
check in mlxsw_sp_nve_vxlan_can_offload(), or to state in the commit
message that MDB presence is intentionally no longer a bar to offload?

>  
>  	return mdb_entry;
>  

[ ... ]

> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 7b82075055237..d323f91af2364 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h

[ ... ]

> @@ -313,6 +314,10 @@ struct vxlan_dev {
>  	unsigned int mdb_seq;
>  };
>  
> +enum vxlan_dev_flags {
> +	VXLAN_DEV_F_MDB,
> +};
> +
>  #define VXLAN_F_LEARN			0x01
>  #define VXLAN_F_PROXY			0x02
>  #define VXLAN_F_RSC			0x04
> @@ -331,7 +336,6 @@ struct vxlan_dev {
>  #define VXLAN_F_IPV6_LINKLOCAL		0x8000
>  #define VXLAN_F_TTL_INHERIT		0x10000
>  #define VXLAN_F_VNIFILTER               0x20000
> -#define VXLAN_F_MDB			0x40000
>  #define VXLAN_F_LOCALBYPASS		0x80000
>  #define VXLAN_F_MC_ROUTE		0x100000