Hi, Alexandre,
On 31.01.2019 16:20, alexandre.besn...@softathome.com wrote:
> From: Alexandre Besnard <alexandre.besn...@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.besn...@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().
