[PATCH] net: check negative value for signed refcnt

[PATCH] net: check negative value for signed refcnt

[alexandre.besnard]

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;
+		}
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();

--
2.7.4

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

[Kirill Tkhai]

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().

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

[Alexandre BESNARD]

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 ? 

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

[Eric Dumazet]

On 01/31/2019 05:49 AM, Kirill Tkhai wrote:
> 
> 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().

Well, if we read an old value here, after a full and expensive synchronize_net(),
then we would have lot more problems than simply having a second round in
netdev_wait_allrefs()
 

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

[Eric Dumazet]

On 01/31/2019 07:15 AM, Eric Dumazet wrote:
> 
> 
> On 01/31/2019 05:49 AM, Kirill Tkhai wrote:
>>
>> 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().
> 
> Well, if we read an old value here, after a full and expensive synchronize_net(),
> then we would have lot more problems than simply having a second round in
> netdev_wait_allrefs()
>  
> 

percpu_ref was added more recently than the netdev_refcnt stuff, and is
interesting for users wanting a synchronous wait for the refcnt reaching 0.

netdev_wait_allrefs() was designed to be asynchronous, so that we at least release
RTNL (and current cpu) when something is wrong and a device can not be dismantled.

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

[Kirill Tkhai]

On 31.01.2019 18:14, Alexandre BESNARD wrote:
> 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 ? 

The problem is that network devices are destroyed not only during reboot
of the system. For example, this happens every time network namespace dies.
And nobody wants to have network device memory is released in some undefined
condition, while system is continuing to run.

I see two approaches there. First one is to crash the system in case of
refcounter is not released for a long time (which is user-defined parameter).
The second one is to set the counter to INT_MAX/2 or something like this,
and never release this memory (we do especially this in our virtuozzo 7 kernel).

Maybe there are more solutions and another people will say about them.

Kirill

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

[Kirill Tkhai]

On 31.01.2019 18:21, Eric Dumazet wrote:
> 
> 
> On 01/31/2019 07:15 AM, Eric Dumazet wrote:
>>
>>
>> On 01/31/2019 05:49 AM, Kirill Tkhai wrote:
>>>
>>> 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().
>>
>> Well, if we read an old value here, after a full and expensive synchronize_net(),
>> then we would have lot more problems than simply having a second round in
>> netdev_wait_allrefs()
>>  
>>
> 
> percpu_ref was added more recently than the netdev_refcnt stuff, and is
> interesting for users wanting a synchronous wait for the refcnt reaching 0.
> 
> netdev_wait_allrefs() was designed to be asynchronous, so that we at least release
> RTNL (and current cpu) when something is wrong and a device can not be dismantled.

Yeah, they are different, and I think we can't add more synchronize_rcu()-dependent
synchronizations in this code, since network namespaces are already destroyed very slow.