Re: [PATCH v6 2/3] batman-adv: limit number of learned VLANs from bridged-in clients

On Fri, 21 Feb 2025 18:27:32 +0100, Sven Eckelmann <[email protected]> wrote:
> diff --git a/net/batman-adv/mesh-interface.c b/net/batman-adv/mesh-interface.c
> index 3ca84176..5cf3a62c 100644
> --- a/net/batman-adv/mesh-interface.c
> +++ b/net/batman-adv/mesh-interface.c
> @@ -573,6 +577,19 @@ batadv_meshif_create_vlan(struct batadv_priv *bat_priv, unsigned short vid)
>  		return vlan;
>  	}
>  
> +	vlan_dyn_max = bat_priv->meshif_vlan_dyn_max;

See my remark regarding the possible need for READ_ONCE.

> +	vlan_dyn_count = bat_priv->meshif_vlan_dyn_count;
> +
> +	if (vid & BATADV_VLAN_HAS_TAG && !own &&
> +	    vlan_dyn_max <= vlan_dyn_count) {
> +		spin_unlock_bh(&bat_priv->meshif_vlan_list_lock);
> +
> +		net_ratelimited_function(batadv_info, bat_priv->mesh_iface,
> +					 "not adding VLAN %d, already learned %hu VID(s)\n",
> +					 batadv_print_vid(vid), vlan_dyn_max);
> +		return NULL;
> +	}
> +
>  	vlan = kzalloc(sizeof(*vlan), GFP_ATOMIC);
>  	if (!vlan) {
>  		spin_unlock_bh(&bat_priv->meshif_vlan_list_lock);

The patch still applies at 41a3e32c2a80 ("batman-adv: add missing newlines for
log macros"). But with the new way of allocating objects, you need to rebase.
There will be a conflict which I think you need to solve via:

--- a/net/batman-adv/mesh-interface.c
+++ b/net/batman-adv/mesh-interface.c
@@ -564,9 +564,7 @@
 		return vlan;
 	}
 
-<<<<<<<
 	vlan = kzalloc_obj(*vlan, GFP_ATOMIC);
-=======
 	vlan_dyn_max = bat_priv->meshif_vlan_dyn_max;
 	vlan_dyn_count = bat_priv->meshif_vlan_dyn_count;
 
@@ -580,8 +578,7 @@
 		return NULL;
 	}
 
-	vlan = kzalloc(sizeof(*vlan), GFP_ATOMIC);
->>>>>>>
+	vlan = kzalloc_obj(*vlan, GFP_ATOMIC);
 	if (!vlan) {
 		spin_unlock_bh(&bat_priv->meshif_vlan_list_lock);
 		return NULL;

> @@ -824,6 +846,8 @@ static int batadv_meshif_init_late(struct net_device *dev)
>  	bat_priv->tt.last_changeset_len = 0;
>  	bat_priv->isolation_mark = 0;
>  	bat_priv->isolation_mark_mask = 0;
> +	bat_priv->meshif_vlan_dyn_max = 0;

Just to signify the importance of the access pattern, I usually also use
WRITE_ONCE() in the initializer. but this is not technically needed - and maybe
also not preferred by other people.

>
> diff --git a/net/batman-adv/netlink.c b/net/batman-adv/netlink.c
> index c33d2c6c..677d887e 100644
> --- a/net/batman-adv/netlink.c
> +++ b/net/batman-adv/netlink.c
> @@ -610,6 +615,16 @@ static int batadv_netlink_set_mesh(struct sk_buff *skb, struct genl_info *info)
>  		atomic_set(&bat_priv->orig_interval, orig_interval);
>  	}
>  
> +	if (info->attrs[BATADV_ATTR_VLAN_DYN_MAX]) {
> +		u16 vlan_dyn_max;
> +
> +		attr = info->attrs[BATADV_ATTR_VLAN_DYN_MAX];
> +		vlan_dyn_max = nla_get_u16(attr);
> +		vlan_dyn_max = min_t(u16, vlan_dyn_max, VLAN_N_VID);
> +
> +		bat_priv->meshif_vlan_dyn_max = vlan_dyn_max;

Please double check if this is under the same lock as the readers. Because your
need to otherwise use the READ_ONCE/WRITE_ONCE helpers to avoid problems like
load/store tearing, reordering, merges, reloading, .... (just look at the
memory-barriers documentation for a full list).

>
> diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
> index 0ca0fc07..5f2f467b 100644
> --- a/net/batman-adv/types.h
> +++ b/net/batman-adv/types.h
> @@ -1751,6 +1751,12 @@ struct batadv_priv {
>  	/** @meshif_vlan_list_lock: lock protecting meshif_vlan_list */
>  	spinlock_t meshif_vlan_list_lock;

I think this one is now also protecting meshif_vlan_dyn_count

-- 
Sven Eckelmann <[email protected]>