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

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  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(>list);
list_add_tail(_tree_list, >list);

-   /* Initialize the reference counter to the number of switches, not 1 */
kref_init(>refcount);
-   refcount_set(>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(_mutex);
err = dsa_switch_probe(ds);
+   if (ds->dst)
+   dsa_tree_put(ds->dst);
mutex_unlock(_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

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