From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 44B97C5479D for ; Mon, 9 Jan 2023 12:20:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237103AbjAIMUO (ORCPT ); Mon, 9 Jan 2023 07:20:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53296 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234538AbjAIMUJ (ORCPT ); Mon, 9 Jan 2023 07:20:09 -0500 Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C37D81A828; Mon, 9 Jan 2023 04:20:07 -0800 (PST) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 3AF0E5C017C; Mon, 9 Jan 2023 07:20:07 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Mon, 09 Jan 2023 07:20:07 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1673266807; x=1673353207; bh=BoRrhg2qoLX8AzR5nlod213O8Dn0 cuabW6bstFyKh6g=; b=DFepMdIDoB+mlolDcbW2KqnE5pmmpY+RdAochu7RfvCA 9oc+Bv2B0P8Dg4MCtPhAY3VvbvVNwAVqI62ZqJsY5fE0bZmyJ62KC9TA2OhnxS01 InOGx5C8ZAmsKMmOJBsN4qNUud9CcI0iWUYZ+qD8plQ2AxLmmLtniJggTgFuK8mg YVFYYonVCZ2uRj3qXJ+AkeDxlA5Lr0I/2PX00UNM5cKrK/8+25ehqQ4g76akEOBV cd/l23v3UYN/q+CfUEeyGKnQrrVktiHsgbO1d9iP+5+VrT0WG7TOQuBJBBuBt1nj 6hsAQCXix7tobllTf1RJqzut1NlyA8f8Z/+geLEmUg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrkeeigdeffecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehttdertddttddvnecuhfhrohhmpefkughoucfu tghhihhmmhgvlhcuoehiughoshgthhesihguohhstghhrdhorhhgqeenucggtffrrghtth gvrhhnpedvudefveekheeugeeftddvveefgfduieefudeifefgleekheegleegjeejgeeg hfenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehiug hoshgthhesihguohhstghhrdhorhhg X-ME-Proxy: Feedback-ID: i494840e7:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 9 Jan 2023 07:20:05 -0500 (EST) Date: Mon, 9 Jan 2023 14:20:02 +0200 From: Ido Schimmel To: Vladimir Oltean Cc: Tobias Waldekranz , davem@davemloft.net, kuba@kernel.org, Nikolay Aleksandrov , Andrew Lunn , Vivien Didelot , Florian Fainelli , Jiri Pirko , Ivan Vecera , Roopa Prabhu , Russell King , Petr Machata , Ido Schimmel , Matt Johnston , Cooper Lees , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bridge@lists.linux-foundation.org Subject: Re: [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode Message-ID: References: <20220316150857.2442916-1-tobias@waldekranz.com> <20220316150857.2442916-2-tobias@waldekranz.com> <20230109100236.euq7iaaorqxrun7u@skbuf> <20230109115653.6yjijaj63n2v35lw@skbuf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230109115653.6yjijaj63n2v35lw@skbuf> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 09, 2023 at 01:56:53PM +0200, Vladimir Oltean wrote: > On Mon, Jan 09, 2023 at 01:43:46PM +0200, Ido Schimmel wrote: > > OK, thanks for confirming. Will send a patch later this week if Tobias > > won't take care of it by then. First patch will probably be [1] to make > > sure we dump the correct MST state to user space. It will also make it > > easier to show the problem and validate the fix. > > > > [1] > > diff --git a/net/bridge/br.c b/net/bridge/br.c > > index 4f5098d33a46..f02a1ad589de 100644 > > --- a/net/bridge/br.c > > +++ b/net/bridge/br.c > > @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) > > case BR_BOOLOPT_MCAST_VLAN_SNOOPING: > > return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); > > case BR_BOOLOPT_MST_ENABLE: > > - return br_opt_get(br, BROPT_MST_ENABLED); > > + return br_mst_is_enabled(br); > > Well, this did report the correct MST state despite the incorrect static > branch state, no? The users of br_mst_is_enabled(br) are broken, not > those of br_opt_get(br, BROPT_MST_ENABLED). I should have said "actual"/"effective" instead of "correct". IMO, it's better to use the same conditional in the both the data and control paths to eliminate discrepancies. Without the patch, a user will see that MST is supposedly enabled when it is actually disabled in the data path. > > Anyway, I see there's a br_mst_is_enabled() and also a br_mst_enabled()?! > One is used in the fast path and the other in the slow path. They should > probably be merged, I guess. They both exist probably because somebody > thought that the "if (!netif_is_bridge_master(dev))" test is redundant > in the fast path. The single user of br_mst_enabled() (DSA) is not affected by the bug (only the SW data path is), so I suggest making this consolidation in net-next after the bug is fixed. OK? > > > default: > > /* shouldn't be called with unsupported options */ > > WARN_ON(1); > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 75aff9bbf17e..7f0475f62d45 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) > > /* br_mst.c */ > > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > > DECLARE_STATIC_KEY_FALSE(br_mst_used); > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return static_branch_unlikely(&br_mst_used) && > > br_opt_get(br, BROPT_MST_ENABLED); > > @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, > > int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, > > struct netlink_ext_ack *extack); > > #else > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return false; > > } From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org A28C660EFD DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org D771760784 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1673266807; x=1673353207; bh=BoRrhg2qoLX8AzR5nlod213O8Dn0 cuabW6bstFyKh6g=; b=DFepMdIDoB+mlolDcbW2KqnE5pmmpY+RdAochu7RfvCA 9oc+Bv2B0P8Dg4MCtPhAY3VvbvVNwAVqI62ZqJsY5fE0bZmyJ62KC9TA2OhnxS01 InOGx5C8ZAmsKMmOJBsN4qNUud9CcI0iWUYZ+qD8plQ2AxLmmLtniJggTgFuK8mg YVFYYonVCZ2uRj3qXJ+AkeDxlA5Lr0I/2PX00UNM5cKrK/8+25ehqQ4g76akEOBV cd/l23v3UYN/q+CfUEeyGKnQrrVktiHsgbO1d9iP+5+VrT0WG7TOQuBJBBuBt1nj 6hsAQCXix7tobllTf1RJqzut1NlyA8f8Z/+geLEmUg== Date: Mon, 9 Jan 2023 14:20:02 +0200 From: Ido Schimmel Message-ID: References: <20220316150857.2442916-1-tobias@waldekranz.com> <20220316150857.2442916-2-tobias@waldekranz.com> <20230109100236.euq7iaaorqxrun7u@skbuf> <20230109115653.6yjijaj63n2v35lw@skbuf> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230109115653.6yjijaj63n2v35lw@skbuf> Subject: Re: [Bridge] [PATCH v5 net-next 01/15] net: bridge: mst: Multiple Spanning Tree (MST) mode List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Oltean Cc: Ivan Vecera , Andrew Lunn , Florian Fainelli , Jiri Pirko , Petr Machata , Nikolay Aleksandrov , bridge@lists.linux-foundation.org, Russell King , Vivien Didelot , Ido Schimmel , netdev@vger.kernel.org, Cooper Lees , Roopa Prabhu , kuba@kernel.org, Matt Johnston , davem@davemloft.net, linux-kernel@vger.kernel.org, Tobias Waldekranz On Mon, Jan 09, 2023 at 01:56:53PM +0200, Vladimir Oltean wrote: > On Mon, Jan 09, 2023 at 01:43:46PM +0200, Ido Schimmel wrote: > > OK, thanks for confirming. Will send a patch later this week if Tobias > > won't take care of it by then. First patch will probably be [1] to make > > sure we dump the correct MST state to user space. It will also make it > > easier to show the problem and validate the fix. > > > > [1] > > diff --git a/net/bridge/br.c b/net/bridge/br.c > > index 4f5098d33a46..f02a1ad589de 100644 > > --- a/net/bridge/br.c > > +++ b/net/bridge/br.c > > @@ -286,7 +286,7 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt) > > case BR_BOOLOPT_MCAST_VLAN_SNOOPING: > > return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED); > > case BR_BOOLOPT_MST_ENABLE: > > - return br_opt_get(br, BROPT_MST_ENABLED); > > + return br_mst_is_enabled(br); > > Well, this did report the correct MST state despite the incorrect static > branch state, no? The users of br_mst_is_enabled(br) are broken, not > those of br_opt_get(br, BROPT_MST_ENABLED). I should have said "actual"/"effective" instead of "correct". IMO, it's better to use the same conditional in the both the data and control paths to eliminate discrepancies. Without the patch, a user will see that MST is supposedly enabled when it is actually disabled in the data path. > > Anyway, I see there's a br_mst_is_enabled() and also a br_mst_enabled()?! > One is used in the fast path and the other in the slow path. They should > probably be merged, I guess. They both exist probably because somebody > thought that the "if (!netif_is_bridge_master(dev))" test is redundant > in the fast path. The single user of br_mst_enabled() (DSA) is not affected by the bug (only the SW data path is), so I suggest making this consolidation in net-next after the bug is fixed. OK? > > > default: > > /* shouldn't be called with unsupported options */ > > WARN_ON(1); > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > > index 75aff9bbf17e..7f0475f62d45 100644 > > --- a/net/bridge/br_private.h > > +++ b/net/bridge/br_private.h > > @@ -1827,7 +1827,7 @@ static inline bool br_vlan_state_allowed(u8 state, bool learn_allow) > > /* br_mst.c */ > > #ifdef CONFIG_BRIDGE_VLAN_FILTERING > > DECLARE_STATIC_KEY_FALSE(br_mst_used); > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return static_branch_unlikely(&br_mst_used) && > > br_opt_get(br, BROPT_MST_ENABLED); > > @@ -1845,7 +1845,7 @@ int br_mst_fill_info(struct sk_buff *skb, > > int br_mst_process(struct net_bridge_port *p, const struct nlattr *mst_attr, > > struct netlink_ext_ack *extack); > > #else > > -static inline bool br_mst_is_enabled(struct net_bridge *br) > > +static inline bool br_mst_is_enabled(const struct net_bridge *br) > > { > > return false; > > }