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

[Kirill Tkhai]

Hi, Alexandre,

On 31.01.2019 16:20, alexandre.besnard@xxxxxxxxxxxxxx wrote:
> From: Alexandre Besnard <alexandre.besnard@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx>
> ---
>  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().