netdev - Re: [PATCH net] packet: call fanout_release, while UNREGISTERING a netdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 Jan 2017 17:03:53 +0000
From:   Anoob Soman <anoob.soman@...rix.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
CC:     David Miller <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] packet: call fanout_release, while UNREGISTERING a
 netdev



On 30/01/17 19:44, Eric Dumazet wrote:
> On Mon, 2017-01-30 at 19:08 +0000, Anoob Soman wrote:
>> On 30/01/17 17:26, Eric Dumazet wrote:
>>> On Thu, 2016-10-06 at 20:50 -0400, David Miller wrote:
>>>> From: Anoob Soman <anoob.soman@...rix.com>
>>>> Date: Wed, 5 Oct 2016 15:12:54 +0100
>>>>
>>>>> If a socket has FANOUT sockopt set, a new proto_hook is registered
>>>>> as part of fanout_add(). When processing a NETDEV_UNREGISTER event in
>>>>> af_packet, __fanout_unlink is called for all sockets, but prot_hook which was
>>>>> registered as part of fanout_add is not removed. Call fanout_release, on a
>>>>> NETDEV_UNREGISTER, which removes prot_hook and removes fanout from the
>>>>> fanout_list.
>>>>>
>>>>> This fixes BUG_ON(!list_empty(&dev->ptype_specific)) in netdev_run_todo()
>>>>>
>>>>> Signed-off-by: Anoob Soman <anoob.soman@...rix.com>
>>>> Applied and queued up for -stable, thanks.
>>> This commit (6664498280cf "packet: call fanout_release, while
>>> UNREGISTERING a netdev")
>>> looks buggy :
>>>
>>> We end up calling fanout_release() while holding a spinlock
>>> ( spin_lock(&po->bind_lock); )
>>>
>>> But fanout_release() grabs a mutex ( mutex_lock(&fanout_mutex) ), and
>>> this is absolutely not valid while holding a spinlock.
>> Yes, that is wrong.
>>
>>> Anoob, can you cook a fix, I guess you have a way to reproduce the thing
>>> that wanted a kernel patch ?
>>>
>>> (Please build your test kernel with CONFIG_LOCKDEP=y)
>> Sure, I am planning to move fanout_release(sk) after
>> spin_unlock(bind_lock). Something like this.
>>                                   }
>>                                   if (msg == NETDEV_UNREGISTER) {
>>                                           packet_cached_dev_reset(po);
>> -                                       fanout_release(sk);
>>                                           po->ifindex = -1;
>>                                           if (po->prot_hook.dev)
>> dev_put(po->prot_hook.dev);
>>                                           po->prot_hook.dev = NULL;
>>                                   }
>>                                   spin_unlock(&po->bind_lock);
>> +                               if (msg == NETDEV_UNREGISTER) {
>> +                                       fanout_release(sk);
>> +                               }
>>                           }
>>                           break;
>>
>> I will quickly test it out.
> It wont be enough.
>
> You need to also fix a race if two cpus call fanout_release(sk) at the
> same time.
>
> Thanks.
>
>
>
Hi Eric,

I have ran into some problem trying to enable CONFIG_LOCKDEP. I think 
this particular scenario, taking mutex_lock() while holding a spin_lock 
debugging, requires CONFIG_DEBUG_ATOMIC_SLEEP to be enabled. 
CONFIG_DEBUG_ATOMIC_SLEEP, selects CONFIG_PREEMPT_COUNT and my kernel 
doesn't behave well if PREEMPTION is enabled. I am trying to reproduce 
this issue in a way that I might be able to use debug_atomic_sleep.

Meanwhile, I have modified patch fix the race.

@@ -1722,18 +1722,20 @@ static void fanout_release(struct sock *sk)
      struct packet_sock *po = pkt_sk(sk);
      struct packet_fanout *f;

+    mutex_lock(&fanout_mutex);
+
      f = po->fanout;
-    if (!f)
+    if (!f) {
+        mutex_unlock(&fanout_mutex);
          return;
-
-    mutex_lock(&fanout_mutex);
-    po->fanout = NULL;
+    }

      if (atomic_dec_and_test(&f->sk_ref)) {
          list_del(&f->list);
          dev_remove_pack(&f->prot_hook);
          fanout_release_data(f);
          kfree(f);
+        po->fanout = NULL;
      }
      mutex_unlock(&fanout_mutex);

@@ -3855,13 +3857,14 @@ static int packet_notifier(struct notifier_block 
*this,
                  }
                  if (msg == NETDEV_UNREGISTER) {
                      packet_cached_dev_reset(po);
-                    fanout_release(sk);
                      po->ifindex = -1;
                      if (po->prot_hook.dev)
                          dev_put(po->prot_hook.dev);
                      po->prot_hook.dev = NULL;
                  }
                  spin_unlock(&po->bind_lock);
+                if (msg == NETDEV_UNREGISTER)
+                    fanout_release(sk);
              }
              break;
          case NETDEV_UP:

Thanks,
Anoob.