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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 09 Nov 2017 13:24:09 -0500
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>, lkp@...org
Subject: Re: [PATCH net-next] net: dsa: remove tree refcount

Hi Andrew,

Andrew Lunn <andrew@...n.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