Re: [PATCH] net: check negative value for signed refcnt
From: Kirill Tkhai <hidden>
Date: 2019-01-31 13:49:54
Also in:
lkml
Hi, Alexandre, On 31.01.2019 16:20, alexandre.besnard@softathome.com wrote:
quoted hunk ↗ jump to hunk
From: Alexandre Besnard <redacted> 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 <redacted> --- 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().