[PATCH net-next] net: dsa: remove tree refcount

[PATCH net-next] net: dsa: remove tree refcount

[Vivien Didelot]

Setting the refcount to 0 when allocating a tree to match the number of
switch devices it holds may cause an 'increment on 0; use-after-free'.

Tracking the number of devices in a tree with a kref is not really
appropriate anyway so removes it completely in favor of a basic counter.

Fixes: 8e5bf9759a06 ("net: dsa: simplify tree reference counting")
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h |  2 +-
 net/dsa/dsa2.c    | 29 ++++-------------------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 35b5dee7bb23..bbcdff43526f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -119,7 +119,7 @@ struct dsa_switch_tree {
 	unsigned int index;
 
 	/* Number of switches attached to this tree */
-	struct kref refcount;
+	unsigned int nr_devices;
 
 	/* Has this tree been applied to the hardware? */
 	bool setup;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index fd54a8e17986..fda3e5415eaf 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -51,10 +51,6 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
 	INIT_LIST_HEAD(&dst->list);
 	list_add_tail(&dsa_tree_list, &dst->list);
 
-	/* Initialize the reference counter to the number of switches, not 1 */
-	kref_init(&dst->refcount);
-	refcount_set(&dst->refcount.refcount, 0);
-
 	return dst;
 }
 
@@ -75,25 +71,6 @@ static struct dsa_switch_tree *dsa_tree_touch(int index)
 	return dst;
 }
 
-static void dsa_tree_get(struct dsa_switch_tree *dst)
-{
-	kref_get(&dst->refcount);
-}
-
-static void dsa_tree_release(struct kref *ref)
-{
-	struct dsa_switch_tree *dst;
-
-	dst = container_of(ref, struct dsa_switch_tree, refcount);
-
-	dsa_tree_free(dst);
-}
-
-static void dsa_tree_put(struct dsa_switch_tree *dst)
-{
-	kref_put(&dst->refcount, dsa_tree_release);
-}
-
 static bool dsa_port_is_dsa(struct dsa_port *port)
 {
 	return port->type == DSA_PORT_TYPE_DSA;
@@ -492,7 +469,9 @@ static void dsa_tree_remove_switch(struct dsa_switch_tree *dst,
 	dsa_tree_teardown(dst);
 
 	dst->ds[index] = NULL;
-	dsa_tree_put(dst);
+	dst->nr_devices--;
+	if (!dst->nr_devices)
+		dsa_tree_free(dst);
 }
 
 static int dsa_tree_add_switch(struct dsa_switch_tree *dst,
@@ -504,7 +483,7 @@ static int dsa_tree_add_switch(struct dsa_switch_tree *dst,
 	if (dst->ds[index])
 		return -EBUSY;
 
-	dsa_tree_get(dst);
+	dst->nr_devices++;
 	dst->ds[index] = ds;
 
 	err = dsa_tree_setup(dst);
-- 
2.15.0

Re: [PATCH net-next] net: dsa: remove tree refcount

[Andrew Lunn]

On Thu, Nov 09, 2017 at 12:36:52PM -0500, Vivien Didelot wrote:
> Setting the refcount to 0 when allocating a tree to match the number of
> switch devices it holds may cause an 'increment on 0; use-after-free'.
> 
> Tracking the number of devices in a tree with a kref is not really
> appropriate anyway so removes it completely in favor of a basic counter.

Hi Vivien

How are you protecting this basic counter? switches can come and go at
random, modules are loaded and unloaded, probing can happen in
parallel, probes can fail with EPROBE_DEFFER causing a switch to
unregister itself while others are registering themselves, etc.

The point of using a kref is that it is a well known kernel method of
safely handling this situation. When the last member of the tree goes
away, we safely and atomically remove the tree. It worked well for a
few years, until you refactored it. Maybe the correct solution is to
revert your change?

      Andrew

Re: [PATCH net-next] net: dsa: remove tree refcount

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Thu, Nov 09, 2017 at 12:36:52PM -0500, Vivien Didelot wrote:
>> Setting the refcount to 0 when allocating a tree to match the number of
>> switch devices it holds may cause an 'increment on 0; use-after-free'.
>> 
>> Tracking the number of devices in a tree with a kref is not really
>> appropriate anyway so removes it completely in favor of a basic counter.
>
> How are you protecting this basic counter? switches can come and go at
> random, modules are loaded and unloaded, probing can happen in
> parallel, probes can fail with EPROBE_DEFFER causing a switch to
> unregister itself while others are registering themselves, etc.

As for the kref, the counter is protected by dsa2_mutex which locks
switch registration, nothing changed.

> The point of using a kref is that it is a well known kernel method of
> safely handling this situation. When the last member of the tree goes
> away, we safely and atomically remove the tree. It worked well for a
> few years, until you refactored it. Maybe the correct solution is to
> revert your change?

The kref doesn't add any value here and make the code more complex. If
you prefer to keep it, a simple alternative can be provided to init the
refcount to 1 at initialization and decrement it after registration:

    diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
    index fd54a8e17986..1fb8beb66493 100644
    --- a/net/dsa/dsa2.c
    +++ b/net/dsa/dsa2.c
    @@ -51,9 +51,7 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
            INIT_LIST_HEAD(&dst->list);
            list_add_tail(&dsa_tree_list, &dst->list);

    -	/* Initialize the reference counter to the number of switches, not 1 */
            kref_init(&dst->refcount);
    -	refcount_set(&dst->refcount.refcount, 0);

            return dst;
    }
    @@ -69,7 +67,9 @@ static struct dsa_switch_tree *dsa_tree_touch(int index)
            struct dsa_switch_tree *dst;

            dst = dsa_tree_find(index);
    -	if (!dst)
    +	if (dst)
    +		dsa_tree_get(dst);
    +	else
                    dst = dsa_tree_alloc(index);

            return dst;
    @@ -765,6 +765,8 @@ int dsa_register_switch(struct dsa_switch *ds)

            mutex_lock(&dsa2_mutex);
            err = dsa_switch_probe(ds);
    +	if (ds->dst)
    +		dsa_tree_put(ds->dst);
            mutex_unlock(&dsa2_mutex);

            return err;


Getting rid of the refcount seems simpler, but we can use this
alternative instead. Let me know what you prefer.


Thanks,

        Vivien