| 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
| ||
|
Message-ID: <BC9DAE01-92F5-489C-BE0A-451192494B9C@nvidia.com>
Date: Wed, 27 Nov 2024 23:30:29 +0000
From: Yong Wang <yongwang@...dia.com>
To: Nikolay Aleksandrov <razor@...ckwall.org>, Roopa Prabhu
<roopa@...dia.com>, "davem@...emloft.net" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC: Andy Roulin <aroulin@...dia.com>, Ido Schimmel <idosch@...dia.com>, Nikhil
Dhar <ndhar@...dia.com>
Subject: Re: [RFC net-next 1/2] net: bridge: multicast: re-implement port
multicast enable/disable functions
On 11/27/24, 7:18 AM, "Nikolay Aleksandrov" <razor@...ckwall.org> wrote:
>On 26/11/2024 23:34, Yong Wang wrote:
>> Re-implement br_multicast_enable_port() / br_multicast_disable_port() to
>> support per vlan multicast context enabling/disabling for bridge ports.
>> The port state could be changed by STP, that impacts multicast behaviors
>> like igmp query. The corresponding context should be used for per port
>> context or per vlan context accordingly.
>>
>> Signed-off-by: Yong Wang <yongwang@...dia.com>
>> Reviewed-by: Andy Roulin <aroulin@...dia.com>
>> ---
>> net/bridge/br_multicast.c | 75 ++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 67 insertions(+), 8 deletions(-)
>>
>
>Hi,
>A few comments below
Thank you so much for the comments!
>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index b2ae0d2434d2..8b23b0dc6129 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -2105,15 +2105,45 @@ static void __br_multicast_enable_port_ctx(struct net_bridge_mcast_port *pmctx)
>> }
>> }
>>
>> -void br_multicast_enable_port(struct net_bridge_port *port)
>> +static void br_multicast_enable_port_ctx(struct net_bridge_mcast_port *pmctx)
>> {
>> - struct net_bridge *br = port->br;
>> + struct net_bridge *br = pmctx->port->br;
>>
>> spin_lock_bh(&br->multicast_lock);
>> - __br_multicast_enable_port_ctx(&port->multicast_ctx);
>> + __br_multicast_enable_port_ctx(pmctx);
>> spin_unlock_bh(&br->multicast_lock);
>> }
>>
>> +void br_multicast_enable_port(struct net_bridge_port *port)
>> +{
>> + struct net_bridge *br = port->br;
>> +
>> + if (br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED)) {
>> + struct net_bridge_vlan_group *vg;
>> + struct net_bridge_vlan *vlan;
>> +
>> + rcu_read_lock();
>> + vg = nbp_vlan_group_rcu(port);
>> + if (!vg) {
>> + rcu_read_unlock();
>> + return;
>> + }
>> +
>> + /* iterate each vlan of the port, enable port_mcast_ctx per vlan
>> + * when vlan is in allowed states.
>> + */
>> + list_for_each_entry_rcu(vlan, &vg->vlan_list, vlist) {
>> + if ((vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED) &&
>
>This is racy, the flag is changed only under multicast_lock and should be used
>only with the lock held. I'd suggest moving this check in br_multicast_enable_port_ctx()
>after taking the lock where you should check if the context is a vlan one or not.
Thanks for pointing out. Such change looks reasonable, I will update accordingly.
Alternatively, here we can call __br_multicast_enable_port_ctx instead after
checking the flag under multicast_lock.
>
>> + br_vlan_state_allowed(br_vlan_get_state(vlan), true))
>> + br_multicast_enable_port_ctx(&vlan->port_mcast_ctx);
>> + }
>> + rcu_read_unlock();
>> + } else {
>> + /* use the port's multicast context when vlan snooping is disabled */
>> + br_multicast_enable_port_ctx(&port->multicast_ctx);
>> + }
>> +}
>> +
>> static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>> {
>> struct net_bridge_port_group *pg;
>> @@ -2137,11 +2167,40 @@ static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>> br_multicast_rport_del_notify(pmctx, del);
>> }
>>
>> +static void br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>> +{
>> + struct net_bridge *br = pmctx->port->br;
>> +
>> + spin_lock_bh(&br->multicast_lock);
>> + __br_multicast_disable_port_ctx(pmctx);
>> + spin_unlock_bh(&br->multicast_lock);
>> +}
>> +
>> void br_multicast_disable_port(struct net_bridge_port *port)
>> {
>> - spin_lock_bh(&port->br->multicast_lock);
>> - __br_multicast_disable_port_ctx(&port->multicast_ctx);
>> - spin_unlock_bh(&port->br->multicast_lock);
>> + struct net_bridge *br = port->br;
>> +
>> + if (br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED)) {
>
>These blocks in enable and disable are almost identical, maybe make
>a single helper called _toggle (similar to vlan mcast snooping toggle)
>with a on/off bool argument and call the appropriate function based on it.
Sounds good, following such convention, br_multicast_toggle_port seems to be
a good candidate for the name of this helper.
>
>> + struct net_bridge_vlan_group *vg;
>> + struct net_bridge_vlan *vlan;
>> +
>> + rcu_read_lock();
>> + vg = nbp_vlan_group_rcu(port);
>> + if (!vg) {
>> + rcu_read_unlock();
>> + return;
>> + }
>> +
>> + /* iterate each vlan of the port, disable port_mcast_ctx per vlan */
>> + list_for_each_entry(vlan, &vg->vlan_list, vlist) {
>> + if (vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED)
>
>Same comment about the flag check being racy.
ACK.
>
>> + br_multicast_disable_port_ctx(&vlan->port_mcast_ctx);
>> + }
>> + rcu_read_unlock();
>> + } else {
>> + /* use the port's multicast context when vlan snooping is disabled */
>> + br_multicast_disable_port_ctx(&port->multicast_ctx);
>> + }
>> }
>>
>> static int __grp_src_delete_marked(struct net_bridge_port_group *pg)
>> @@ -4304,9 +4363,9 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>> __br_multicast_open(&br->multicast_ctx);
>> list_for_each_entry(p, &br->port_list, list) {
>> if (on)
>> - br_multicast_disable_port(p);
>> + br_multicast_disable_port_ctx(&p->multicast_ctx);
>> else
>> - br_multicast_enable_port(p);
>> + br_multicast_enable_port_ctx(&p->multicast_ctx);
>> }
>>
>> list_for_each_entry(vlan, &vg->vlan_list, vlist)
Powered by blists - more mailing lists