| 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: <546ADA5B-12D3-442A-A423-830DD9D68545@nvidia.com>
Date: Wed, 27 Nov 2024 23:41:07 +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 2/2] net: bridge: multicast: update multicast
contex when vlan state gets changed
On 11/27/24, 7:26 AM, "Nikolay Aleksandrov" <razor@...ckwall.org> wrote:
>On 26/11/2024 23:34, Yong Wang wrote:
>> Add br_vlan_set_state_finish() helper function to be executed right after
>> br_vlan_set_state() when vlan state gets changed, similar to port state,
>> vlan state could impact multicast behaviors as well such as igmp query.
>> When bridge is running with userspace STP, vlan state can be manipulated by
>> "bridge vlan" commands. Updating the corresponding multicast context
>> will ensure the port query timer to continue when vlan state gets changed
>> to those "allowed" states like "forwarding" etc.
>>
>> Signed-off-by: Yong Wang <yongwang@...dia.com>
>> Reviewed-by: Andy Roulin <aroulin@...dia.com>
>> ---
>> net/bridge/br_mst.c | 5 +++--
>> net/bridge/br_multicast.c | 18 ++++++++++++++++++
>> net/bridge/br_private.h | 11 +++++++++++
>> net/bridge/br_vlan_options.c | 2 ++
>> 4 files changed, 34 insertions(+), 2 deletions(-)
>>
>
>A few comments below,
Thank you so much for providing these comments.
>
>> diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
>> index 1820f09ff59c..b77c31a24257 100644
>> --- a/net/bridge/br_mst.c
>> +++ b/net/bridge/br_mst.c
>> @@ -80,10 +80,11 @@ static void br_mst_vlan_set_state(struct net_bridge_vlan_group *vg,
>> if (br_vlan_get_state(v) == state)
>> return;
>>
>> - br_vlan_set_state(v, state);
>> -
>> if (v->vid == vg->pvid)
>> br_vlan_set_pvid_state(vg, state);
>> +
>> + br_vlan_set_state(v, state);
>> + br_vlan_set_state_finish(v, state);
>
>This state_finish function is called after every instance of br_vlan_set_state(),
>just add that call to br_vlan_set_state.
Moving the implementation logic into br_vlan_set_state() sounds good to me, while
the user of br_vlan_set_state() should be aware of the lock usage instead of
the annotated lock-free access.
>
>> }
>>
>> int br_mst_set_state(struct net_bridge_port *p, u16 msti, u8 state,
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 8b23b0dc6129..3a3b63c97c92 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -4270,6 +4270,24 @@ static void __br_multicast_stop(struct net_bridge_mcast *brmctx)
>> #endif
>> }
>>
>> +void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state)
>> +{
>> + struct net_bridge *br;
>> +
>> + if (!br_vlan_should_use(v))
>> + return;
>> +
>> + if (br_vlan_is_master(v))
>> + return;
>> +
>> + br = v->port->br;
>> +
>> + if (br_vlan_state_allowed(state, true) &&
>> + (v->priv_flags & BR_VLFLAG_MCAST_ENABLED) &&
>
>checking this flag without mcast lock is racy.
ACK.
>
>> + br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED))
>
>this should be the first check
ACK.
>
>> + br_multicast_enable_port_ctx(&v->port_mcast_ctx);
>
>What about disable? What if the state is != LEARNING/FORWARDING ?
In case of the state is DISABLED/BLOCKING, br_multicast_port_query_expired() timer handlers
will dis-continue after checking the port and vlan states via calling
br_multicast_port_ctx_state_stopped(), stop by themselves.
The comment inside br_port_state_selection() right after calling br_multicast_enable_port()
explains there's no need to call disable function specifically.
Refer to https://elixir.bootlin.com/linux/v6.12.1/source/net/bridge/br_stp.c#L497
I think, it's better to add similar comment here as well.
In case of LISTENING, keep multicast ctx untouched.
Please correct me if I miss anything.
>
>> +}
>> +
>> void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on)
>> {
>> struct net_bridge *br;
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 9853cfbb9d14..9c72070956e3 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -1052,6 +1052,7 @@ void br_multicast_port_ctx_init(struct net_bridge_port *port,
>> struct net_bridge_vlan *vlan,
>> struct net_bridge_mcast_port *pmctx);
>> void br_multicast_port_ctx_deinit(struct net_bridge_mcast_port *pmctx);
>> +void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state);
>> void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on);
>> int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>> struct netlink_ext_ack *extack);
>> @@ -1502,6 +1503,10 @@ static inline void br_multicast_port_ctx_deinit(struct net_bridge_mcast_port *pm
>> {
>> }
>>
>> +static inline void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state)
>> +{
>> +}
>> +
>> static inline void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan,
>> bool on)
>> {
>> @@ -1853,6 +1858,12 @@ bool br_vlan_global_opts_can_enter_range(const struct net_bridge_vlan *v_curr,
>> bool br_vlan_global_opts_fill(struct sk_buff *skb, u16 vid, u16 vid_range,
>> const struct net_bridge_vlan *v_opts);
>>
>> +/* helper function to be called right after br_vlan_set_state() when vlan state gets changed */
>> +static inline void br_vlan_set_state_finish(struct net_bridge_vlan *v, u8 state)
>> +{
>
>A one line helper that directly calls another function is not helping anything.
>Please just call that function directly and remove the helper.
We can definitely move this function inside br_vlan_set_state() by removing br_vlan_set_state_finish().
>
>> + br_multicast_update_vlan_mcast_ctx(v, state);
>> +}
>> +
>> /* vlan state manipulation helpers using *_ONCE to annotate lock-free access */
>> static inline u8 br_vlan_get_state(const struct net_bridge_vlan *v)
>> {
>> diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
>> index 8fa89b04ee94..bad187c4f16d 100644
>> --- a/net/bridge/br_vlan_options.c
>> +++ b/net/bridge/br_vlan_options.c
>> @@ -123,6 +123,8 @@ static int br_vlan_modify_state(struct net_bridge_vlan_group *vg,
>> br_vlan_set_pvid_state(vg, state);
>>
>> br_vlan_set_state(v, state);
>> + br_vlan_set_state_finish(v, state);
>> +
>> *changed = true;
>>
>> return 0;
Powered by blists - more mailing lists