Message-ID: <ED42A6CD-C622-42D9-B236-611E658A041B@blackwall.org>
Date:   Thu, 26 Jan 2023 22:28:54 +0200
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     Petr Machata <petrm@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>, netdev@...r.kernel.org
CC:     bridge@...ts.linux-foundation.org, Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net-next 00/16] bridge: Limit number of MDB entries per port, port-vlan

On January 26, 2023 7:01:08 PM GMT+02:00, Petr Machata <petrm@...dia.com> wrote:
>The MDB maintained by the bridge is limited. When the bridge is configured
>for IGMP / MLD snooping, a buggy or malicious client can easily exhaust its
>capacity. In SW datapath, the capacity is configurable through the
>IFLA_BR_MCAST_HASH_MAX parameter, but ultimately is finite. Obviously a
>similar limit exists in the HW datapath for purposes of offloading.
>
>In order to prevent the issue of unilateral exhaustion of MDB resources,
>introduce two parameters in each of two contexts:
>
>- Per-port and (when BROPT_MCAST_VLAN_SNOOPING_ENABLED is enabled)
>  per-port-VLAN number of MDB entries that the port is member in.
>
>- Per-port and (when BROPT_MCAST_VLAN_SNOOPING_ENABLED is enabled)
>  per-port-VLAN maximum permitted number of MDB entries, or 0 for
>  no limit.
>
>Per-port number of entries keeps track of the total number of MDB entries
>configured on a given port. The per-port-VLAN value then keeps track of the
>subset of MDB entries configured specifically for the given VLAN, on that
>port. The number is adjusted as port_groups are created and deleted, and
>therefore under multicast lock.
>
>A maximum value, if non-zero, then places a limit on the number of entries
>that can be configured in a given context. Attempts to add entries above
>the maximum are rejected.
>
>Rejection reason of netlink-based requests to add MDB entries is
>communicated through extack. This channel is unavailable for rejections
>triggered from the control path. To address this lack of visibility, the
>patchset adds a tracepoint, bridge:br_mdb_full:
>
>	# perf record -e bridge:br_mdb_full &
>	# [...]
>	# perf script | cut -d: -f4-
>	 dev v2 af 2 src 192.0.2.1/:: grp 239.1.1.1/::/00:00:00:00:00:00 vid 0
>	 dev v2 af 10 src 0.0.0.0/2001:db8:1::1 grp 0.0.0.0/ff0e::1/00:00:00:00:00:00 vid 0
>	 dev v2 af 2 src 192.0.2.1/:: grp 239.1.1.1/::/00:00:00:00:00:00 vid 10
>	 dev v2 af 10 src 0.0.0.0/2001:db8:1::1 grp 0.0.0.0/ff0e::1/00:00:00:00:00:00 vid 10
>
>This tracepoint is triggered for mcast_hash_max exhaustions as well.
>
>The following is an example of how the feature is used. A more extensive
>example is available in patch #8:
>
>	# bridge vlan set dev v1 vid 1 mcast_max_groups 1
>	# bridge mdb add dev br port v1 grp 230.1.2.3 temp vid 1
>	# bridge mdb add dev br port v1 grp 230.1.2.4 temp vid 1
>	Error: bridge: Port-VLAN is already a member in mcast_max_groups (1) groups.
>
>The patchset progresses as follows:
>
>- In patch #1, set strict_start_type at two bridge-related policies. The
>  reason is we are adding a new attribute to one of these, and want the new
>  attribute to be parsed strictly. The other was adjusted for completeness'
>  sake.
>
>- In patches #2 to #5, br_mdb and br_multicast code is adjusted to make the
>  following additions smoother.
>
>- In patch #6, add the tracepoint.
>
>- In patch #7, the code to maintain number of MDB entries is added as
>  struct net_bridge_mcast_port::mdb_n_entries. The maximum is added, too,
>  as struct net_bridge_mcast_port::mdb_max_entries, however at this point
>  there is no way to set the value yet, and since 0 is treated as "no
>  limit", the functionality doesn't change at this point. Note however,
>  that mcast_hash_max violations already do trigger at this point.
>
>- In patch #8, netlink plumbing is added: reading of number of entries, and
>  reading and writing of maximum.
>
>  The per-port values are passed through RTM_NEWLINK / RTM_GETLINK messages
>  in IFLA_BRPORT_MCAST_N_GROUPS and _MAX_GROUPS, inside IFLA_PROTINFO nest.
>
>  The per-port-vlan values are passed through RTM_GETVLAN / RTM_NEWVLAN
>  messages in BRIDGE_VLANDB_ENTRY_MCAST_N_GROUPS, _MAX_GROUPS, inside
>  BRIDGE_VLANDB_ENTRY.
>
>The following patches deal with the selftest:
>
>- Patches #9 and #10 clean up and move around some selftest code.
>
>- Patches #11 to #14 add helpers and generalize the existing IGMP / MLD
>  support to allow generating packets with configurable group addresses and
>  varying source lists for (S,G) memberships.
>
>- Patch #15 adds code to generate IGMP leave and MLD done packets.
>
>- Patch #16 finally adds the selftest itself.
>
>Petr Machata (16):
>  net: bridge: Set strict_start_type at two policies
>  net: bridge: Add extack to br_multicast_new_port_group()
>  net: bridge: Move extack-setting to br_multicast_new_port_group()
>  net: bridge: Add br_multicast_del_port_group()
>  net: bridge: Change a cleanup in br_multicast_new_port_group() to goto
>  net: bridge: Add a tracepoint for MDB overflows
>  net: bridge: Maintain number of MDB entries in net_bridge_mcast_port
>  net: bridge: Add netlink knobs for number / maximum MDB entries
>  selftests: forwarding: Move IGMP- and MLD-related functions to lib
>  selftests: forwarding: bridge_mdb: Fix a typo
>  selftests: forwarding: lib: Add helpers for IP address handling
>  selftests: forwarding: lib: Add helpers for checksum handling
>  selftests: forwarding: lib: Parameterize IGMPv3/MLDv2 generation
>  selftests: forwarding: lib: Allow list of IPs for IGMPv3/MLDv2
>  selftests: forwarding: lib: Add helpers to build IGMP/MLD leave
>    packets
>  selftests: forwarding: bridge_mdb_max: Add a new selftest
>
> include/trace/events/bridge.h                 |  67 ++
> include/uapi/linux/if_bridge.h                |   2 +
> include/uapi/linux/if_link.h                  |   2 +
> net/bridge/br_mdb.c                           |  17 +-
> net/bridge/br_multicast.c                     | 255 ++++-
> net/bridge/br_netlink.c                       |  21 +-
> net/bridge/br_netlink_tunnel.c                |   3 +
> net/bridge/br_private.h                       |  22 +-
> net/bridge/br_vlan.c                          |  11 +-
> net/bridge/br_vlan_options.c                  |  33 +-
> net/core/net-traces.c                         |   1 +
> net/core/rtnetlink.c                          |   2 +-
> .../testing/selftests/net/forwarding/Makefile |   1 +
> .../selftests/net/forwarding/bridge_mdb.sh    |  60 +-
> .../net/forwarding/bridge_mdb_max.sh          | 970 ++++++++++++++++++
> tools/testing/selftests/net/forwarding/lib.sh | 216 ++++
> 16 files changed, 1604 insertions(+), 79 deletions(-)
> create mode 100755 tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
>


Nice set, thanks! Please hold off applying until Sunday when I'll be able to properly review it.

Cheers,
  Nik

Powered by blists - more mailing lists