[net-next,v4,3/6] net: dsa: Introduce dsa tagger data operation.

Commit Message

Mattias Forsblad Sept. 6, 2022, 6:34 a.m. UTC

Support connecting dsa tagger for frame2reg decoding
with it's associated hookup functions.

Signed-off-by: Mattias Forsblad <[e-mail]>
---
 include/net/dsa.h |  5 +++++
 net/dsa/tag_dsa.c | 32 +++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 3 deletions(-)

Comments

Andrew Lunn Sept. 6, 2022, 1:08 p.m. UTC | #1

>  		switch (code) {
>  		case DSA_CODE_FRAME2REG:
> -			/* Remote management is not implemented yet,
> -			 * drop.
> -			 */
> +			tagger_data = ds->tagger_data;
> +			if (likely(tagger_data->decode_frame2reg))
> +				tagger_data->decode_frame2reg(dev, skb);
>  			return NULL;
>  		case DSA_CODE_ARP_MIRROR:
>  		case DSA_CODE_POLICY_MIRROR:
> @@ -323,6 +326,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  	return skb;
>  }
  
> +static void dsa_tag_disconnect(struct dsa_switch *ds)
> +{
> +	kfree(ds->tagger_data);
> +	ds->tagger_data = NULL;
> +}


Probably a question for Vladimir.

Is there a potential use after free here? Is it guaranteed that the
masters dev->dsa_ptr will be cleared before the disconnect function is
called?

Also, the receive function checks for tagger_data->decode_frame2reg,
but does not check for tagger_data != NULL.

This is just a straight copy of tag_qca, so if there are issues here,
they are probably not new issues.

     Andrew

Vladimir Oltean Sept. 6, 2022, 1:51 p.m. UTC | #2

On Tue, Sep 06, 2022 at 03:08:23PM +0200, Andrew Lunn wrote:
> >  		switch (code) {
> >  		case DSA_CODE_FRAME2REG:
> > -			/* Remote management is not implemented yet,
> > -			 * drop.
> > -			 */
> > +			tagger_data = ds->tagger_data;
> > +			if (likely(tagger_data->decode_frame2reg))
> > +				tagger_data->decode_frame2reg(dev, skb);
> >  			return NULL;
> >  		case DSA_CODE_ARP_MIRROR:
> >  		case DSA_CODE_POLICY_MIRROR:
> > @@ -323,6 +326,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
> >  	return skb;
> >  }
>   
> > +static void dsa_tag_disconnect(struct dsa_switch *ds)
> > +{
> > +	kfree(ds->tagger_data);
> > +	ds->tagger_data = NULL;
> > +}
> 
> 
> Probably a question for Vladimir.
> 
> Is there a potential use after free here? Is it guaranteed that the
> masters dev->dsa_ptr will be cleared before the disconnect function is
> called?

There is no use after free because there is no free...
There are 3 cases of tag protocol disconnect, one is on error path of
bidirectional connection (handled properly), another is on tag protocol
change (handled properly; we only allow it with the master down), and
another is on switch driver removal (handled incorrectly, we don't do
anything here).

The following patch is compile tested only. However it should answer
your question of order of operations too.

>From d93b2ddf0e0f4e82d6b0bac4591b346e8cd0fdb9 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <[e-mail]>
Date: Tue, 6 Sep 2022 16:24:41 +0300
Subject: [PATCH] net: dsa: don't leak tagger-owned storage on switch driver
 unbind

In the initial commit dc452a471dba ("net: dsa: introduce tagger-owned
storage for private and shared data"), we had a call to
tag_ops->disconnect(dst) issued from dsa_tree_free(), which is called at
tree teardown time.

There were problems with connecting to a switch tree as a whole, so this
got reworked to connecting to individual switches within the tree. In
this process, tag_ops->disconnect(ds) was made to be called only from
switch.c (cross-chip notifiers emitted as a result of dynamic tag proto
changes), but the normal driver teardown code path wasn't replaced with
anything.

Solve this problem by adding a function that does the opposite of
dsa_switch_setup_tag_protocol(), which is called from the equivalent
spot in dsa_switch_teardown(). The positioning here also ensures that we
won't have any use-after-free in tagging protocol (*rcv) ops, since the
teardown sequence is as follows:

dsa_tree_teardown
-> dsa_tree_teardown_master
   -> dsa_master_teardown
      -> unsets master->dsa_ptr, making no further packets match the
         ETH_P_XDSA packet type handler
-> dsa_tree_teardown_ports
   -> dsa_port_teardown
      -> dsa_slave_destroy
         -> unregisters DSA net devices, there is even a synchronize_net()
            in unregister_netdevice_many()
-> dsa_tree_teardown_switches
   -> dsa_switch_teardown
      -> dsa_switch_teardown_tag_protocol
         -> finally frees the tagger-owned storage

Fixes: 7f2973149c22 ("net: dsa: make tagging protocols connect to individual switches from a tree")
Signed-off-by: Vladimir Oltean <[e-mail]>
---
 net/dsa/dsa2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ed56c7a554b8..4bb0a203b85c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -864,6 +864,14 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
 	return err;
 }
 
+static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds)
+{
+	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
+
+	if (tag_ops->disconnect)
+		tag_ops->disconnect(ds);
+}
+
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;
@@ -967,6 +975,8 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
 		ds->slave_mii_bus = NULL;
 	}
 
+	dsa_switch_teardown_tag_protocol(ds);
+
 	if (ds->ops->teardown)
 		ds->ops->teardown(ds);

Patch

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 70a358641235..21f9eadb9543 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -130,6 +130,11 @@  struct dsa_lag {
 	refcount_t refcount;
 };
 
+struct dsa_tagger_data {
+	void (*decode_frame2reg)(struct net_device *netdev,
+				 struct sk_buff *skb);
+};
+
 struct dsa_switch_tree {
 	struct list_head	list;
 
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index e4b6e3f2a3db..3dd1dcddaf05 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -198,7 +198,10 @@  static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 				  u8 extra)
 {
+	struct dsa_tagger_data *tagger_data;
+	struct dsa_port *dp = dev->dsa_ptr;
 	bool trap = false, trunk = false;
+	struct dsa_switch *ds = dp->ds;
 	int source_device, source_port;
 	enum dsa_code code;
 	enum dsa_cmd cmd;
@@ -218,9 +221,9 @@  static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 
 		switch (code) {
 		case DSA_CODE_FRAME2REG:
-			/* Remote management is not implemented yet,
-			 * drop.
-			 */
+			tagger_data = ds->tagger_data;
+			if (likely(tagger_data->decode_frame2reg))
+				tagger_data->decode_frame2reg(dev, skb);
 			return NULL;
 		case DSA_CODE_ARP_MIRROR:
 		case DSA_CODE_POLICY_MIRROR:
@@ -323,6 +326,25 @@  static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 }
 
+static int dsa_tag_connect(struct dsa_switch *ds)
+{
+	struct dsa_tagger_data *tagger_data;
+
+	tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
+	if (!tagger_data)
+		return -ENOMEM;
+
+	ds->tagger_data = tagger_data;
+
+	return 0;
+}
+
+static void dsa_tag_disconnect(struct dsa_switch *ds)
+{
+	kfree(ds->tagger_data);
+	ds->tagger_data = NULL;
+}
+
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
 
 static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -343,6 +365,8 @@  static const struct dsa_device_ops dsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_DSA,
 	.xmit	  = dsa_xmit,
 	.rcv	  = dsa_rcv,
+	.connect  = dsa_tag_connect,
+	.disconnect = dsa_tag_disconnect,
 	.needed_headroom = DSA_HLEN,
 };
 
@@ -385,6 +409,8 @@  static const struct dsa_device_ops edsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_EDSA,
 	.xmit	  = edsa_xmit,
 	.rcv	  = edsa_rcv,
+	.connect  = dsa_tag_connect,
+	.disconnect = dsa_tag_disconnect,
 	.needed_headroom = EDSA_HLEN,
 };