netdev - Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown multicast as mrouters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Apr 2022 20:37:58 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Joachim Wiberg <troglobit@...il.com>,
        Roopa Prabhu <roopa@...dia.com>
Cc:     netdev@...r.kernel.org, bridge@...ts.linux-foundation.org,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH RFC net-next 08/13] net: bridge: avoid classifying unknown
 multicast as mrouters_only

On 12/04/2022 20:27, Joachim Wiberg wrote:
> 
> Hi Nik,
> 
> and thank you for taking the time to respond!
> 
> On Tue, Apr 12, 2022 at 16:59, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>> On 11/04/2022 16:38, Joachim Wiberg wrote:
>>> Unknown multicast, MAC/IPv4/IPv6, should always be flooded according to
>>> the per-port mcast_flood setting, as well as to detected and configured
>>> mcast_router ports.
> 
> I realize I should've included a reference to RFC4541 here.  Will add
> that in the non-RFC patch.
> 
>>> This patch drops the mrouters_only classifier of unknown IP multicast
>>> and moves the flow handling from br_multicast_flood() to br_flood().
>>> This in turn means br_flood() must know about multicast router ports.
>> If you'd like to flood unknown mcast traffic when a router is present please add
>> a new option which defaults to the current state (disabled).
> 
> I don't think we have to add another option, because according to the
> snooping RFC[1], section 2.1.2 Data Forwarding Rules:
> 
>  "3) [..] If a switch receives an unregistered packet, it must forward
>   that packet on all ports to which an IGMP[2] router is attached.  A
>   switch may default to forwarding unregistered packets on all ports.
>   Switches that do not forward unregistered packets to all ports must
>   include a configuration option to force the flooding of unregistered
>   packets on specified ports. [..]"
> 
> From this I'd like to argue that our current behavior in the bridge is
> wrong.  To me it's clear that, since we have a confiugration option, we
> should forward unknown IP multicast to all MCAST_FLOOD ports (as well as
> the router ports).

Definitely not wrong. In fact:
"Switches that do not forward unregistered packets to all ports must
 include a configuration option to force the flooding of unregistered
 packets on specified ports. [..]"

is already implemented because the admin can mark any port as a router and
enable flooding to it.

> 
> Also, and more critically, the current behavior of offloaded switches do
> forwarding like this already.  So there is a discrepancy currently
> between how the bridge forwards unknown multicast and how any underlying
> switchcore does it.
> 
> Sure, we'll break bridge behavior slightly by forwarding to more ports
> than previous (until the group becomes known/registered), but we'd be
> standards compliant, and the behavior can still be controlled per-port.
> 
> [1]: https://www.rfc-editor.org/rfc/rfc4541.html#section-2.1.2
> [2]: Section 3 goes on to explain how this is similar also for MLD
> 

RFC4541 is only recommending, it's not a mandatory behaviour. This default has been placed
for a very long time and a lot of users and tests take it into consideration.
We cannot break such assumptions and start suddenly flooding packets, but we can
leave it up to the admin or distribution/network software to configure it as default.

>>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
>>> index 02bb620d3b8d..ab5b97a8c12e 100644
>>> --- a/net/bridge/br_forward.c
>>> +++ b/net/bridge/br_forward.c
>>> @@ -199,9 +199,15 @@ static struct net_bridge_port *maybe_deliver(
>>>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>>>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
>>>  {
>>> +	struct net_bridge_mcast *brmctx = &br->multicast_ctx;
>> Note this breaks per-vlan mcast. You have to use the inferred mctx.
> 
> Thank you, this was one of the things I was really unsure about since
> the introduction of per-VLAN support.  I'll extend the prototype and
> include the brmctx from br_handle_frame_finish().  Thanks!
> 
> Best regards
>  /Joachim