Re: [Bridge] [PATCH net-next mlxsw v2 07/16] net: bridge: Maintain number of MDB entries in net_bridge_mcast_port
Nikolay Aleksandrov writes:
> On 01/02/2023 19:28, Petr Machata wrote:
>> @@ -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. :)
I didn't see any (relevant) KCSAN warnings on x86_64. But yeah, it
(and the others you cited) should be *_ONCE.
Re: [Bridge] [PATCH net-next mlxsw v2 07/16] net: bridge: Maintain number of MDB entries in net_bridge_mcast_port
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
>> ---
>>
>> 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
>> #include
>> #endif
>> +#include
>>
>> #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(>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(>port_mcast_ctx))
>> +pmctx = >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.
>
>> +}
>> +
Re: [Bridge] [PATCH net-next mlxsw v2 07/16] net: bridge: Maintain number of MDB entries in net_bridge_mcast_port
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
> ---
>
> 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
> #include
> #endif
> +#include
>
> #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(>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(>port_mcast_ctx))
> + pmctx = >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()
> +}
> +
> +static int br_multicast_port_ngroups_inc(struct net_bridge_port *port,
> + const struct br_ip *group,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_bridge_mcast_port *pmctx;
> + int err;
> +
> + lockdep_assert_held_once(>br->multicast_lock);
> +
> + /* Always count on the port context. */
> + err = br_multicast_port_ngroups_inc_one(>multicast_ctx, extack);
> + if (err) {
> + NL_SET_ERR_MSG_FMT_MOD(extack, "Port is already in %u groups,
> and mcast_max_groups=%u",
> +
