[PATCH v2 net] net: bridge: Do not offload IGMP/MLD messages

[PATCH v2 net] net: bridge: Do not offload IGMP/MLD messages

[Joseph Huang]

Do not offload IGMP/MLD messages as it could lead to IGMP/MLD Reports
being unintentionally flooded to Hosts. Instead, let the bridge decide
where to send these IGMP/MLD messages.

Consider the case where the local host is sending out reports in response
to a remote querier like the following:

       mcast-listener-process (IP_ADD_MEMBERSHIP)
          \
          br0
         /   \
      swp1   swp2
        |     |
  QUERIER     SOME-OTHER-HOST

In the above setup, br0 will want to br_forward() reports for
mcast-listener-process's group(s) via swp1 to QUERIER; but since the
source hwdom is 0, the report is eligible for tx offloading, and is
flooded by hardware to both swp1 and swp2, reaching SOME-OTHER-HOST as
well. (Example and illustration provided by Tobias.)

Fixes: 472111920f1c ("net: bridge: switchdev: allow the TX data plane forwarding to be offloaded")
Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
v1: https://lore.kernel.org/netdev/20250701193639.836027-1-Joseph.Huang@garmin.com/
v2: Updated commit message.
---
 net/bridge/br_switchdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 95d7355a0407..757c34bf5931 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -18,7 +18,8 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct net_bridge_port *p,
 		return false;
 
 	return (p->flags & BR_TX_FWD_OFFLOAD) &&
-	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
+	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom) &&
+	       !br_multicast_igmp_type(skb);
 }
 
 bool br_switchdev_frame_uses_tx_fwd_offload(struct sk_buff *skb)
-- 
2.49.0

Re: [PATCH v2 net] net: bridge: Do not offload IGMP/MLD messages

[Ido Schimmel]

On Mon, Jul 14, 2025 at 11:01:00AM -0400, Joseph Huang wrote:
> Do not offload IGMP/MLD messages as it could lead to IGMP/MLD Reports
> being unintentionally flooded to Hosts. Instead, let the bridge decide
> where to send these IGMP/MLD messages.
> 
> Consider the case where the local host is sending out reports in response
> to a remote querier like the following:
> 
>        mcast-listener-process (IP_ADD_MEMBERSHIP)
>           \
>           br0
>          /   \
>       swp1   swp2
>         |     |
>   QUERIER     SOME-OTHER-HOST
> 
> In the above setup, br0 will want to br_forward() reports for
> mcast-listener-process's group(s) via swp1 to QUERIER; but since the
> source hwdom is 0, the report is eligible for tx offloading, and is
> flooded by hardware to both swp1 and swp2, reaching SOME-OTHER-HOST as
> well. (Example and illustration provided by Tobias.)
> 
> Fixes: 472111920f1c ("net: bridge: switchdev: allow the TX data plane forwarding to be offloaded")
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>

I don't have personal experience with this offload, but it makes sense
to not offload the replication of control packets to the underlying
device and instead let the CPU handle it. These shouldn't be sent at an
high rate anyway.

> ---
> v1: https://lore.kernel.org/netdev/20250701193639.836027-1-Joseph.Huang@garmin.com/
> v2: Updated commit message.
> ---
>  net/bridge/br_switchdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 95d7355a0407..757c34bf5931 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -18,7 +18,8 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct net_bridge_port *p,
>  		return false;
>  
>  	return (p->flags & BR_TX_FWD_OFFLOAD) &&
> -	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
> +	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom) &&
> +	       !br_multicast_igmp_type(skb);

I think you can just early return if the packet is IGMP/MLD. Something
like:

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 95d7355a0407..9a910cf0256e 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -17,6 +17,9 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct net_bridge_port *p,
 	if (!static_branch_unlikely(&br_switchdev_tx_fwd_offload))
 		return false;
 
+	if (br_multicast_igmp_type(skb))
+		return false;
+
 	return (p->flags & BR_TX_FWD_OFFLOAD) &&
 	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
 }

Re: [PATCH v2 net] net: bridge: Do not offload IGMP/MLD messages

[Joseph Huang]

On 7/16/2025 2:26 AM, Ido Schimmel wrote:
> On Mon, Jul 14, 2025 at 11:01:00AM -0400, Joseph Huang wrote:
>> Do not offload IGMP/MLD messages as it could lead to IGMP/MLD Reports
>> being unintentionally flooded to Hosts. Instead, let the bridge decide
>> where to send these IGMP/MLD messages.
>>
>> Consider the case where the local host is sending out reports in response
>> to a remote querier like the following:
>>
>>         mcast-listener-process (IP_ADD_MEMBERSHIP)
>>            \
>>            br0
>>           /   \
>>        swp1   swp2
>>          |     |
>>    QUERIER     SOME-OTHER-HOST
>>
>> In the above setup, br0 will want to br_forward() reports for
>> mcast-listener-process's group(s) via swp1 to QUERIER; but since the
>> source hwdom is 0, the report is eligible for tx offloading, and is
>> flooded by hardware to both swp1 and swp2, reaching SOME-OTHER-HOST as
>> well. (Example and illustration provided by Tobias.)
>>
>> Fixes: 472111920f1c ("net: bridge: switchdev: allow the TX data plane forwarding to be offloaded")
>> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> 
> I don't have personal experience with this offload, but it makes sense
> to not offload the replication of control packets to the underlying
> device and instead let the CPU handle it. These shouldn't be sent at an
> high rate anyway.
> 
> 
> I think you can just early return if the packet is IGMP/MLD. Something
> like:
> 
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 95d7355a0407..9a910cf0256e 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -17,6 +17,9 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct net_bridge_port *p,
>   	if (!static_branch_unlikely(&br_switchdev_tx_fwd_offload))
>   		return false;
>   
> +	if (br_multicast_igmp_type(skb))
> +		return false;
> +
>   	return (p->flags & BR_TX_FWD_OFFLOAD) &&
>   	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
>   }

Talking about these packets being low rate, should I add unlikely() like so:

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 95d7355a0407..9a910cf0256e 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -17,6 +17,9 @@ static bool nbp_switchdev_can_offload_tx_fwd(const 
struct net_bridge_port *p,
   	if (!static_branch_unlikely(&br_switchdev_tx_fwd_offload))
   		return false;

+	if (unlikely(br_multicast_igmp_type(skb)))
+		return false;
+
   	return (p->flags & BR_TX_FWD_OFFLOAD) &&
   	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
   }

Thanks,
Joseph

Re: [PATCH v2 net] net: bridge: Do not offload IGMP/MLD messages

[Ido Schimmel]

On Wed, Jul 16, 2025 at 10:29:55AM -0400, Joseph Huang wrote:
> Talking about these packets being low rate, should I add unlikely() like so:

I don't think it's needed and probably not measurable in most (all?)
deployments. The entire thing is hidden behind static_branch_unlikely().
It just seemed weird to perform the checks about the Tx offload and only
later check if it's even a packet for which you want this offload.

> 
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 95d7355a0407..9a910cf0256e 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -17,6 +17,9 @@ static bool nbp_switchdev_can_offload_tx_fwd(const struct
> net_bridge_port *p,
>   	if (!static_branch_unlikely(&br_switchdev_tx_fwd_offload))
>   		return false;
> 
> +	if (unlikely(br_multicast_igmp_type(skb)))
> +		return false;
> +
>   	return (p->flags & BR_TX_FWD_OFFLOAD) &&
>   	       (p->hwdom != BR_INPUT_SKB_CB(skb)->src_hwdom);
>   }
> 
> Thanks,
> Joseph