[PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[Oleksandr Mazur]

Whenever bridge driver hits the max capacity of MDBs, it disables
the MC processing (by setting corresponding bridge option), but never
notifies switchdev about such change (the notifiers are called only upon
explicit setting of this option, through the registered netlink interface).

This could lead to situation when Software MDB processing gets disabled,
but this event never gets offloaded to the underlying Hardware.

Fix this by adding a notify message in such case.

Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
---
 net/bridge/br_multicast.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index de2409889489..d53c08906bc8 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -82,6 +82,9 @@ static void br_multicast_find_del_pg(struct net_bridge *br,
 				     struct net_bridge_port_group *pg);
 static void __br_multicast_stop(struct net_bridge_mcast *brmctx);
 
+static int br_mc_disabled_update(struct net_device *dev, bool value,
+				 struct netlink_ext_ack *extack);
+
 static struct net_bridge_port_group *
 br_sg_port_find(struct net_bridge *br,
 		struct net_bridge_port_group_sg_key *sg_p)
@@ -1156,6 +1159,8 @@ struct net_bridge_mdb_entry *br_multicast_new_group(struct net_bridge *br,
 		return mp;
 
 	if (atomic_read(&br->mdb_hash_tbl.nelems) >= br->hash_max) {
+		err = br_mc_disabled_update(br->dev, false, NULL);
+		WARN_ON(err && err != -EOPNOTSUPP);
 		br_opt_toggle(br, BROPT_MULTICAST_ENABLED, false);
 		return ERR_PTR(-E2BIG);
 	}
-- 
2.17.1

Re: [PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[Jakub Kicinski]

On Fri, 11 Feb 2022 15:14:26 +0200 Oleksandr Mazur wrote:
> Whenever bridge driver hits the max capacity of MDBs, it disables
> the MC processing (by setting corresponding bridge option), but never
> notifies switchdev about such change (the notifiers are called only upon
> explicit setting of this option, through the registered netlink interface).
> 
> This could lead to situation when Software MDB processing gets disabled,
> but this event never gets offloaded to the underlying Hardware.
> 
> Fix this by adding a notify message in such case.

Any comments on this one?

We have drivers offloading mdb so presumably this should have a Fixes
tag and go to net, right?

Re: [PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[Nikolay Aleksandrov]

(+CC Ido)
On 15/02/2022 07:13, Jakub Kicinski wrote:
> On Fri, 11 Feb 2022 15:14:26 +0200 Oleksandr Mazur wrote:
>> Whenever bridge driver hits the max capacity of MDBs, it disables
>> the MC processing (by setting corresponding bridge option), but never
>> notifies switchdev about such change (the notifiers are called only upon
>> explicit setting of this option, through the registered netlink interface).
>>
>> This could lead to situation when Software MDB processing gets disabled,
>> but this event never gets offloaded to the underlying Hardware.
>>
>> Fix this by adding a notify message in such case.
> 
> Any comments on this one?
> 
> We have drivers offloading mdb so presumably this should have a Fixes
> tag and go to net, right?

The change looks ok, but it'd be nice to get switchdev folks comment as well.
About the tree, -net should be targeted. I think the issue has existed
since mdb disabled could be offloaded, so the tag should be:

Fixes: 147c1e9b902c ("switchdev: bridge: Offload multicast disabled")

Re: [PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[Ido Schimmel]

On Tue, Feb 15, 2022 at 10:31:41AM +0200, Nikolay Aleksandrov wrote:
> (+CC Ido)
> On 15/02/2022 07:13, Jakub Kicinski wrote:
> > On Fri, 11 Feb 2022 15:14:26 +0200 Oleksandr Mazur wrote:
> >> Whenever bridge driver hits the max capacity of MDBs, it disables
> >> the MC processing (by setting corresponding bridge option), but never
> >> notifies switchdev about such change (the notifiers are called only upon
> >> explicit setting of this option, through the registered netlink interface).
> >>
> >> This could lead to situation when Software MDB processing gets disabled,
> >> but this event never gets offloaded to the underlying Hardware.
> >>
> >> Fix this by adding a notify message in such case.
> > 
> > Any comments on this one?
> > 
> > We have drivers offloading mdb so presumably this should have a Fixes
> > tag and go to net, right?
> 
> The change looks ok, but it'd be nice to get switchdev folks comment as well.
> About the tree, -net should be targeted. I think the issue has existed
> since mdb disabled could be offloaded, so the tag should be:
> 
> Fixes: 147c1e9b902c ("switchdev: bridge: Offload multicast disabled")

LGTM

Oleksandr, please mark future patches as net/net-next:
https://docs.kernel.org/networking/netdev-FAQ.html#how-do-i-indicate-which-tree-net-vs-net-next-my-patch-should-be-in

Re: [PATCH] net: bridge: multicast: notify switchdev driver whenever MC processing gets disabled

[Ido Schimmel]

On Fri, Feb 11, 2022 at 03:14:26PM +0200, Oleksandr Mazur wrote:
> Whenever bridge driver hits the max capacity of MDBs, it disables
> the MC processing (by setting corresponding bridge option), but never
> notifies switchdev about such change (the notifiers are called only upon
> explicit setting of this option, through the registered netlink interface).
> 
> This could lead to situation when Software MDB processing gets disabled,
> but this event never gets offloaded to the underlying Hardware.
> 
> Fix this by adding a notify message in such case.
> 
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> ---
>  net/bridge/br_multicast.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index de2409889489..d53c08906bc8 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -82,6 +82,9 @@ static void br_multicast_find_del_pg(struct net_bridge *br,
>  				     struct net_bridge_port_group *pg);
>  static void __br_multicast_stop(struct net_bridge_mcast *brmctx);
>  
> +static int br_mc_disabled_update(struct net_device *dev, bool value,
> +				 struct netlink_ext_ack *extack);
> +
>  static struct net_bridge_port_group *
>  br_sg_port_find(struct net_bridge *br,
>  		struct net_bridge_port_group_sg_key *sg_p)
> @@ -1156,6 +1159,8 @@ struct net_bridge_mdb_entry *br_multicast_new_group(struct net_bridge *br,
>  		return mp;
>  
>  	if (atomic_read(&br->mdb_hash_tbl.nelems) >= br->hash_max) {
> +		err = br_mc_disabled_update(br->dev, false, NULL);
> +		WARN_ON(err && err != -EOPNOTSUPP);

What is the purpose of the WARN_ON()? There are a lot of operations that
can fail in rollback paths, but we never WARN_ON() there

I suggest:

br_mc_disabled_update(br->dev, false, NULL);

>  		br_opt_toggle(br, BROPT_MULTICAST_ENABLED, false);
>  		return ERR_PTR(-E2BIG);
>  	}
> -- 
> 2.17.1
>