All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Marek Behún" <kabel@kernel.org>
To: Vladimir Oltean <vladimir.oltean@nxp.com>,
	Tobias Waldekranz <tobias@waldekranz.com>
Cc: netdev@vger.kernel.org, "Andrew Lunn" <andrew@lunn.ch>,
	"Marek Behún" <kabel@kernel.org>, "Jan Bětík" <hagrid@svine.us>
Subject: [PATCH net] net: dsa: fix panic when port leaves a bridge
Date: Mon, 14 Mar 2022 16:34:10 +0100	[thread overview]
Message-ID: <20220314153410.31744-1-kabel@kernel.org> (raw)

Fix a data structure breaking / NULL-pointer dereference in
dsa_switch_bridge_leave().

When a DSA port leaves a bridge, dsa_switch_bridge_leave() is called by
notifier for every DSA switch that contains ports that are in the
bridge.

But the part of the code that unsets vlan_filtering expects that the ds
argument refers to the same switch that contains the leaving port.

This leads to various problems, including a NULL pointer dereference,
which was observed on Turris MOX with 2 switches (one with 8 user ports
and another with 4 user ports).

Thus we need to move the vlan_filtering change code to the non-crosschip
branch.

Fixes: d371b7c92d190 ("net: dsa: Unset vlan_filtering when ports leave the bridge")
Reported-by: Jan Bětík <hagrid@svine.us>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 net/dsa/switch.c | 97 +++++++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 42 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e3c7d2627a61..38afb1e8fcb7 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -123,9 +123,61 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 	struct dsa_port *dp;
 	int err;
 
-	if (dst->index == info->tree_index && ds->index == info->sw_index &&
-	    ds->ops->port_bridge_leave)
-		ds->ops->port_bridge_leave(ds, info->port, info->bridge);
+	if (dst->index == info->tree_index && ds->index == info->sw_index) {
+		if (ds->ops->port_bridge_leave)
+			ds->ops->port_bridge_leave(ds, info->port,
+						   info->bridge);
+
+		/* This function is called by the notifier for every DSA switch
+		 * that has ports in the bridge we are leaving, but vlan
+		 * filtering on the port should be changed only once. Since the
+		 * code expects that ds is the switch containing the leaving
+		 * port, the following code must not be called in the crosschip
+		 * branch, only here.
+		 */
+
+		if (ds->needs_standalone_vlan_filtering &&
+		    !br_vlan_enabled(info->bridge.dev)) {
+			change_vlan_filtering = true;
+			vlan_filtering = true;
+		} else if (!ds->needs_standalone_vlan_filtering &&
+			   br_vlan_enabled(info->bridge.dev)) {
+			change_vlan_filtering = true;
+			vlan_filtering = false;
+		}
+
+		/* If the bridge was vlan_filtering, the bridge core doesn't
+		 * trigger an event for changing vlan_filtering setting upon
+		 * slave ports leaving it. That is a good thing, because that
+		 * lets us handle it and also handle the case where the switch's
+		 * vlan_filtering setting is global (not per port). When that
+		 * happens, the correct moment to trigger the vlan_filtering
+		 * callback is only when the last port leaves the last
+		 * VLAN-aware bridge.
+		 */
+		if (change_vlan_filtering && ds->vlan_filtering_is_global) {
+			dsa_switch_for_each_port(dp, ds) {
+				struct net_device *br =
+					dsa_port_bridge_dev_get(dp);
+
+				if (br && br_vlan_enabled(br)) {
+					change_vlan_filtering = false;
+					break;
+				}
+			}
+		}
+
+		if (change_vlan_filtering) {
+			err = dsa_port_vlan_filtering(dsa_to_port(ds,
+								  info->port),
+						      vlan_filtering, &extack);
+			if (extack._msg)
+				dev_err(ds->dev, "port %d: %s\n", info->port,
+					extack._msg);
+			if (err && err != -EOPNOTSUPP)
+				return err;
+		}
+	}
 
 	if ((dst->index != info->tree_index || ds->index != info->sw_index) &&
 	    ds->ops->crosschip_bridge_leave)
@@ -133,45 +185,6 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 						info->sw_index, info->port,
 						info->bridge);
 
-	if (ds->needs_standalone_vlan_filtering &&
-	    !br_vlan_enabled(info->bridge.dev)) {
-		change_vlan_filtering = true;
-		vlan_filtering = true;
-	} else if (!ds->needs_standalone_vlan_filtering &&
-		   br_vlan_enabled(info->bridge.dev)) {
-		change_vlan_filtering = true;
-		vlan_filtering = false;
-	}
-
-	/* If the bridge was vlan_filtering, the bridge core doesn't trigger an
-	 * event for changing vlan_filtering setting upon slave ports leaving
-	 * it. That is a good thing, because that lets us handle it and also
-	 * handle the case where the switch's vlan_filtering setting is global
-	 * (not per port). When that happens, the correct moment to trigger the
-	 * vlan_filtering callback is only when the last port leaves the last
-	 * VLAN-aware bridge.
-	 */
-	if (change_vlan_filtering && ds->vlan_filtering_is_global) {
-		dsa_switch_for_each_port(dp, ds) {
-			struct net_device *br = dsa_port_bridge_dev_get(dp);
-
-			if (br && br_vlan_enabled(br)) {
-				change_vlan_filtering = false;
-				break;
-			}
-		}
-	}
-
-	if (change_vlan_filtering) {
-		err = dsa_port_vlan_filtering(dsa_to_port(ds, info->port),
-					      vlan_filtering, &extack);
-		if (extack._msg)
-			dev_err(ds->dev, "port %d: %s\n", info->port,
-				extack._msg);
-		if (err && err != -EOPNOTSUPP)
-			return err;
-	}
-
 	return dsa_tag_8021q_bridge_leave(ds, info);
 }
 
-- 
2.34.1


             reply	other threads:[~2022-03-14 15:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-14 15:34 Marek Behún [this message]
2022-03-14 15:41 ` [PATCH net] net: dsa: fix panic when port leaves a bridge Vladimir Oltean
2022-03-14 15:45 ` Vladimir Oltean
2022-03-14 16:06   ` Marek Behún
2022-03-14 15:48 ` Tobias Waldekranz
2022-03-14 16:05   ` Marek Behún
2022-03-14 16:17     ` Vladimir Oltean
2022-03-14 16:34       ` Marek Behún
2022-03-14 16:42         ` Vladimir Oltean
2022-03-15  0:04           ` Jakub Kicinski
2022-03-14 16:47   ` Marek Behún
2022-03-14 18:09     ` Marek Behún
2022-03-14 18:19       ` Tobias Waldekranz
2022-03-14 19:27         ` Marek Behún

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220314153410.31744-1-kabel@kernel.org \
    --to=kabel@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=hagrid@svine.us \
    --cc=netdev@vger.kernel.org \
    --cc=tobias@waldekranz.com \
    --cc=vladimir.oltean@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.