Messages in this thread | | | Date | Thu, 31 Jan 2019 16:14:20 +0100 (CET) | From | Alexandre BESNARD <> | Subject | Re: [PATCH] net: check negative value for signed refcnt |
| |
Hi Kirill, and thanks for your time,
On 31 Jan 19 14:49, Kirill Tkhai ktkhai@virtuozzo.com wrote :
> Hi, Alexandre,
> On 31.01.2019 16:20, alexandre.besnard@softathome.com wrote: > > From: Alexandre Besnard <alexandre.besnard@softathome.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@softathome.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().
I agree with you, as it is not a full fix for a bad behavior of the refcnt: many wrong things could happen here, and that's why I added a warning (short of a more critical flag I could think of).
However, I think this is a good approach as a global workaround for any critical situation caused by a negative refcnt, acting as a failsafe. What I try to avoid here is not the bug, but a situation such as a deadlock keeping a system from powering off, or way worse in the system life. On the other hand, I can't think of a critical situation caused by freeing the device memory. Processes or even systems may crash in some cases, but it should be an expected behavior in such a case IMHO.
Actually, I think that with the current implementation, most of the systems locked in the problem are powered off.
Do you think of any issue beyond this behavior ?
| |