On 02/02/2023 10:56, Nikolay Aleksandrov wrote:
> On 01/02/2023 19:28, Petr Machata 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 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.
>>
>> The per-port multicast context is used for tracking of MDB entries for the
>> port as a whole. This is available for all bridges.
>>
>> The per-port-VLAN multicast context is then only available on
>> VLAN-filtering bridges on VLANs that have multicast snooping on.
>>
>> With these changes in place, it will be possible to configure MDB limit for
>> bridge as a whole, or any one port as a whole, or any single port-VLAN.
>>
>> Note that unlike the global limit, exhaustion of the per-port and
>> per-port-VLAN maximums does not cause disablement of multicast snooping.
>> It is also permitted to configure the local limit larger than hash_max,
>> even though that is not useful.
>>
>> In this patch, introduce only the accounting for number of entries, and the
>> max field itself, but not the means to toggle the max. The next patch
>> introduces the netlink APIs to toggle and read the values.
>>
>> Signed-off-by: Petr Machata <petrm@xxxxxxxxxx>
>> ---
>>
>> Notes:
>> v2:
>> - In br_multicast_port_ngroups_inc_one(), bounce
>> if n>=max, not if n==max
>> - Adjust extack messages to mention ngroups, now that
>> the bounces appear when n>=max, not n==max
>> - In __br_multicast_enable_port_ctx(), do not reset
>> max to 0. Also do not count number of entries by
>> going through _inc, as that would end up incorrectly
>> bouncing the entries.
>>
>> net/bridge/br_multicast.c | 132 +++++++++++++++++++++++++++++++++++++-
>> net/bridge/br_private.h | 2 +
>> 2 files changed, 133 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 51b622afdb67..e7ae339a8757 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -31,6 +31,7 @@
>> #include <net/ip6_checksum.h>
>> #include <net/addrconf.h>
>> #endif
>> +#include <trace/events/bridge.h>
>>
>> #include "br_private.h"
>> #include "br_private_mcast_eht.h"
>> @@ -234,6 +235,29 @@ br_multicast_pg_to_port_ctx(const struct net_bridge_port_group *pg)
>> return pmctx;
>> }
>>
>> +static struct net_bridge_mcast_port *
>> +br_multicast_port_vid_to_port_ctx(struct net_bridge_port *port, u16 vid)
>> +{
>> + struct net_bridge_mcast_port *pmctx = NULL;
>> + struct net_bridge_vlan *vlan;
>> +
>> + lockdep_assert_held_once(&port->br->multicast_lock);
>> +
>> + if (!br_opt_get(port->br, BROPT_MCAST_VLAN_SNOOPING_ENABLED))
>> + return NULL;
>> +
>> + /* Take RCU to access the vlan. */
>> + rcu_read_lock();
>> +
>> + vlan = br_vlan_find(nbp_vlan_group_rcu(port), vid);
>> + if (vlan && !br_multicast_port_ctx_vlan_disabled(&vlan->port_mcast_ctx))
>> + pmctx = &vlan->port_mcast_ctx;
>> +
>> + rcu_read_unlock();
>> +
>> + return pmctx;
>> +}
>> +
>> /* when snooping we need to check if the contexts should be used
>> * in the following order:
>> * - if pmctx is non-NULL (port), check if it should be used
>> @@ -668,6 +692,82 @@ void br_multicast_del_group_src(struct net_bridge_group_src *src,
>> __br_multicast_del_group_src(src);
>> }
>>
>> +static int
>> +br_multicast_port_ngroups_inc_one(struct net_bridge_mcast_port *pmctx,
>> + struct netlink_ext_ack *extack)
>> +{
>> + if (pmctx->mdb_max_entries &&
>> + pmctx->mdb_n_entries >= pmctx->mdb_max_entries)
>
> These should be using *_ONCE() because of the next patch.
> KCSAN might be sad otherwise. :)
>
>> + return -E2BIG;
>> +
>> + pmctx->mdb_n_entries++;
>
> WRITE_ONCE()
>
>> + return 0;
>> +}
>> +
>> +static void br_multicast_port_ngroups_dec_one(struct net_bridge_mcast_port *pmctx)
>> +{
>> + WARN_ON_ONCE(pmctx->mdb_n_entries-- == 0);
>
> READ_ONCE()
err, I meant WRITE_ONCE() of course. :)
Need to get coffee.
>
>> +}
>> +
