netdev - Re: [PATCH] net: check negative value for signed refcnt

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8b654b17-f8b4-0c89-26b5-311aeb703f6d@virtuozzo.com>
Date:   Thu, 31 Jan 2019 16:49:38 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     alexandre.besnard@...tathome.com, davem@...emloft.net,
        ecree@...arflare.com, jiri@...lanox.com, petrm@...lanox.com,
        alexander.h.duyck@...el.com, amritha.nambiar@...el.com,
        lirongqing@...du.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: check negative value for signed refcnt

Hi, Alexandre,

On 31.01.2019 16:20, alexandre.besnard@...tathome.com wrote:
> From: Alexandre Besnard <alexandre.besnard@...tathome.com>
> 
> Device remaining references counter is get as a signed integer.
> 
> When unregistering network devices, the loop waiting for this counter
> to decrement tests the 0 strict equality. Thus if an error occurs and
> two references are given back by a protocol, we are stuck in the loop
> forever, with a -1 value.
> 
> Robustness is added by checking a negative value: the device is then
> considered free of references, and a warning is issued (it should not
> happen, one should check that behavior)
> 
> Signed-off-by: Alexandre Besnard <alexandre.besnard@...tathome.com>
> ---
>  net/core/dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ddc551f..e4190ae 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8687,6 +8687,11 @@ static void netdev_wait_allrefs(struct net_device *dev)
>  	refcnt = netdev_refcnt_read(dev);
> 
>  	while (refcnt != 0) {
> +		if (refcnt < 0) {
> +			pr_warn("Device %s refcnt negative: device considered free, but it should not happen\n",
> +				dev->name);
> +			break;
> +		}

1)I don't think this is a good approach. Negative value does not guarantee
there is just a double put of device reference. Negative value is an indicator
something goes wrong, and we definitely should not free device memory in
this case.

2)Not related to your patch -- it looks like we have problem in existing
code with this netdev_refcnt_read(). It does not imply a memory ordering
or some guarantees about reading percpu values. For example, in generic
code struct percpu_ref switches a counter into atomic mode before it checks
for the last reference. But there is nothing in netdev_refcnt_read().