Re: Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Tue, Sep 19, 2017 at 12:09 PM, Willem de Bruijnwrote: > On Tue, Sep 19, 2017 at 3:21 AM, Nixiaoming wrote: >> On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn >> >> wrote: >> >>> >> >>> In case of failure we also need to unlink and free match. I >> >>> sent the following: >> >>> >> >>> http://patchwork.ozlabs.org/patch/813945/ >> >> >> >> + spin_lock(>bind_lock); >> >> + if (po->running && >> >> + match->type == type && >> >>match->prot_hook.type == po->prot_hook.type && >> >>match->prot_hook.dev == po->prot_hook.dev) { >> >> err = -ENOSPC; >> >> @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 >> type_flags) >> >> err = 0; >> >> } >> >>} >> >> + spin_unlock(>bind_lock); >> >> + >> >> + if (err && !refcount_read(>sk_ref)) { >> >> +list_del(>list); >> >> +kfree(match); >> >> + } >> >> >> >> >> >> In the function fanout_add add spin_lock to protect po-> running and po-> >> fanout, >> >> then whether it should be in the function fanout_release also add spin_lock >> protection ? > > po->bind_lock is held when registering and unregistering the > protocol hook. fanout_release does access po->running or > prot_hook. whoops. does *not* access.
Re: Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Tue, Sep 19, 2017 at 3:21 AM, Nixiaomingwrote: > On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijn > > wrote: > >> > >> In case of failure we also need to unlink and free match. I > >> sent the following: > >> > >> http://patchwork.ozlabs.org/patch/813945/ > > > > + spin_lock(>bind_lock); > > + if (po->running && > > + match->type == type && > >match->prot_hook.type == po->prot_hook.type && > >match->prot_hook.dev == po->prot_hook.dev) { > > err = -ENOSPC; > > @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 > type_flags) > > err = 0; > > } > >} > > + spin_unlock(>bind_lock); > > + > > + if (err && !refcount_read(>sk_ref)) { > > +list_del(>list); > > +kfree(match); > > + } > > > > > > In the function fanout_add add spin_lock to protect po-> running and po-> > fanout, > > then whether it should be in the function fanout_release also add spin_lock > protection ? po->bind_lock is held when registering and unregistering the protocol hook. fanout_release does access po->running or prot_hook. It is called from packet_release, which does hold the bind_lock when unregistering the protocol hook.
Re:Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijnwrote: > > In case of failure we also need to unlink and free match. I > sent the following: > > http://patchwork.ozlabs.org/patch/813945/ + spin_lock(>bind_lock); + if (po->running && + match->type == type && match->prot_hook.type == po->prot_hook.type && match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC; @@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) err = 0; } } + spin_unlock(>bind_lock); + + if (err && !refcount_read(>sk_ref)) { +list_del(>list); +kfree(match); + } In the function fanout_add add spin_lock to protect po-> running and po-> fanout, then whether it should be in the function fanout_release also add spin_lock protection ? static struct packet_fanout *fanout_release(struct sock *sk) mutex_lock(_mutex); f = po->fanout; if (f) { po->fanout = NULL;
Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Fri, Sep 15, 2017 at 10:46 AM, Willem de Bruijnwrote: > > In case of failure we also need to unlink and free match. I > sent the following: > > http://patchwork.ozlabs.org/patch/813945/ Ah, will take a look.
Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Fri, Sep 15, 2017 at 1:41 PM, Cong Wangwrote: > On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijn > wrote: >> On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming wrote: >>> From: l00219569 >>> >>> If fanout_add is preempted after running po-> fanout = match >>> and before running __fanout_link, >>> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink >>> >>> so, we need add mutex_lock(_mutex) to __unregister_prot_hook >> >> The packet socket code has no shortage of locks, so there are many >> ways to avoid the race condition between fanout_add and packet_set_ring. >> >> Another option would be to lock the socket when calling fanout_add: >> >> - return fanout_add(sk, val & 0x, val >> 16); >> + lock_sock(sk); >> + ret = fanout_add(sk, val & 0x, val >> 16); >> + release_sock(sk); >> + return ret; >> > > I don't think this is an option, because __unregister_prot_hook() > can be called without lock_sock(), for example in packet_notifier(). > > >> But, for consistency, and to be able to continue to make sense of the >> locking policy, we should use the most appropriate lock. This >> is po->bind_lock, as it ensures atomicity between testing whether >> a protocol hook is active through po->running and the actual existence >> of that hook on the protocol hook list. > > Yeah, register_prot_hook() and unregister_prot_hook() already assume > bind_lock. > > [...] > >>> out: >>> mutex_unlock(_mutex); >>> + spin_unlock(>bind_lock); >> >> This function can call kzalloc with GFP_KERNEL, which may sleep. It is >> not correct to sleep while holding a spinlock. Which is why I take the lock >> later and test po->running again. > > > Right, no need to mention the mutex_unlock() before the spin_unlock() > is clearly wrong. > > >> >> I will clean up that patch and send it for review. > > How about the following patch? > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index c26172995511..f5c696a548ed 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id, > u16 type_flags) > match->prot_hook.dev == po->prot_hook.dev) { > err = -ENOSPC; > if (refcount_read(>sk_ref) < PACKET_FANOUT_MAX) { > + spin_lock(>bind_lock); > __dev_remove_pack(>prot_hook); > - po->fanout = match; > - refcount_set(>sk_ref, > refcount_read(>sk_ref) + 1); > - __fanout_link(sk, po); > + if (po->running) { > + refcount_set(>sk_ref, > refcount_read(>sk_ref) + 1); > + po->fanout = match; > + __fanout_link(sk, po); > + } > + spin_unlock(>bind_lock); > err = 0; > } > } In case of failure we also need to unlink and free match. I sent the following: http://patchwork.ozlabs.org/patch/813945/
Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijnwrote: > On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming wrote: >> From: l00219569 >> >> If fanout_add is preempted after running po-> fanout = match >> and before running __fanout_link, >> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink >> >> so, we need add mutex_lock(_mutex) to __unregister_prot_hook > > The packet socket code has no shortage of locks, so there are many > ways to avoid the race condition between fanout_add and packet_set_ring. > > Another option would be to lock the socket when calling fanout_add: > > - return fanout_add(sk, val & 0x, val >> 16); > + lock_sock(sk); > + ret = fanout_add(sk, val & 0x, val >> 16); > + release_sock(sk); > + return ret; > I don't think this is an option, because __unregister_prot_hook() can be called without lock_sock(), for example in packet_notifier(). > But, for consistency, and to be able to continue to make sense of the > locking policy, we should use the most appropriate lock. This > is po->bind_lock, as it ensures atomicity between testing whether > a protocol hook is active through po->running and the actual existence > of that hook on the protocol hook list. Yeah, register_prot_hook() and unregister_prot_hook() already assume bind_lock. [...] >> out: >> mutex_unlock(_mutex); >> + spin_unlock(>bind_lock); > > This function can call kzalloc with GFP_KERNEL, which may sleep. It is > not correct to sleep while holding a spinlock. Which is why I take the lock > later and test po->running again. Right, no need to mention the mutex_unlock() before the spin_unlock() is clearly wrong. > > I will clean up that patch and send it for review. How about the following patch? diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index c26172995511..f5c696a548ed 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC; if (refcount_read(>sk_ref) < PACKET_FANOUT_MAX) { + spin_lock(>bind_lock); __dev_remove_pack(>prot_hook); - po->fanout = match; - refcount_set(>sk_ref, refcount_read(>sk_ref) + 1); - __fanout_link(sk, po); + if (po->running) { + refcount_set(>sk_ref, refcount_read(>sk_ref) + 1); + po->fanout = match; + __fanout_link(sk, po); + } + spin_unlock(>bind_lock); err = 0; } }
Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Thu, Sep 14, 2017 at 10:07 AM, nixiaomingwrote: > From: l00219569 > > If fanout_add is preempted after running po-> fanout = match > and before running __fanout_link, > it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink > > so, we need add mutex_lock(_mutex) to __unregister_prot_hook The packet socket code has no shortage of locks, so there are many ways to avoid the race condition between fanout_add and packet_set_ring. Another option would be to lock the socket when calling fanout_add: - return fanout_add(sk, val & 0x, val >> 16); + lock_sock(sk); + ret = fanout_add(sk, val & 0x, val >> 16); + release_sock(sk); + return ret; But, for consistency, and to be able to continue to make sense of the locking policy, we should use the most appropriate lock. This is po->bind_lock, as it ensures atomicity between testing whether a protocol hook is active through po->running and the actual existence of that hook on the protocol hook list. fanout_mutex protects the fanout object's list. Taking that on __unregister_prot_hook even in the case where fanout is not used (and __dev_remove_pack is called) complicates locking in this already complicated code. > or add spin_lock(>bind_lock) before po-> fanout = match > > this is a patch for add po->bind_lock in fanout_add > > test on linux 4.1.12: > ./trinity -c setsockopt -C 2 -X & Thanks for testing! > > BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()! > Kernel panic - not syncing: BUG! > CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW O4.1.12 #1 > Hardware name: Hisilicon PhosphorHi1382 FPGA (DT) > Call trace: > [] dump_backtrace+0x0/0xf8 > [] show_stack+0x20/0x28 > [] dump_stack+0xac/0xe4 > [] panic+0xf8/0x268 > [] __unregister_prot_hook+0xa0/0x144 > [] packet_set_ring+0x280/0x5b4 > [] packet_setsockopt+0x320/0x950 > [] SyS_setsockopt+0xa4/0xd4 > > Signed-off-by: nixiaoming > Tested-by: wudesheng > --- > net/packet/af_packet.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 54a18a8..7a52a3b 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1446,12 +1446,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 > type_flags) > default: > return -EINVAL; > } > - > - if (!po->running) > + spin_lock(>bind_lock); > + if (!po->running) { > + spin_unlock(>bind_lock); > return -EINVAL; > + } > > - if (po->fanout) > + if (po->fanout) { > + spin_unlock(>bind_lock); > return -EALREADY; > + } > > mutex_lock(_mutex); > match = NULL; > @@ -1501,6 +1505,7 @@ static int fanout_add(struct sock *sk, u16 id, u16 > type_flags) > } > out: > mutex_unlock(_mutex); > + spin_unlock(>bind_lock); This function can call kzalloc with GFP_KERNEL, which may sleep. It is not correct to sleep while holding a spinlock. Which is why I take the lock later and test po->running again. I will clean up that patch and send it for review.
[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: l00219569If fanout_add is preempted after running po-> fanout = match and before running __fanout_link, it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink so, we need add mutex_lock(_mutex) to __unregister_prot_hook or add spin_lock(>bind_lock) before po-> fanout = match this is a patch for add po->bind_lock in fanout_add test on linux 4.1.12: ./trinity -c setsockopt -C 2 -X & BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()! Kernel panic - not syncing: BUG! CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW O4.1.12 #1 Hardware name: Hisilicon PhosphorHi1382 FPGA (DT) Call trace: [] dump_backtrace+0x0/0xf8 [] show_stack+0x20/0x28 [] dump_stack+0xac/0xe4 [] panic+0xf8/0x268 [] __unregister_prot_hook+0xa0/0x144 [] packet_set_ring+0x280/0x5b4 [] packet_setsockopt+0x320/0x950 [] SyS_setsockopt+0xa4/0xd4 Signed-off-by: nixiaoming Tested-by: wudesheng --- net/packet/af_packet.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 54a18a8..7a52a3b 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1446,12 +1446,16 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) default: return -EINVAL; } - - if (!po->running) + spin_lock(>bind_lock); + if (!po->running) { + spin_unlock(>bind_lock); return -EINVAL; + } - if (po->fanout) + if (po->fanout) { + spin_unlock(>bind_lock); return -EALREADY; + } mutex_lock(_mutex); match = NULL; @@ -1501,6 +1505,7 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) } out: mutex_unlock(_mutex); + spin_unlock(>bind_lock); return err; } -- 2.10.1
Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
On Wed, Sep 13, 2017 at 10:40 PM, nixiaomingwrote: > If fanout_add is preempted after running po-> fanout = match > and before running __fanout_link, > it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink > > so, we need add mutex_lock(_mutex) to __unregister_prot_hook > or add spin_lock(>bind_lock) before po-> fanout = match > > test on linux 4.1.42: > ./trinity -c setsockopt -C 2 -X & > > BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()! > Kernel panic - not syncing: BUG! > CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW O4.1.12 #1 > Hardware name: Hisilicon PhosphorHi1382 FPGA (DT) > Call trace: > [] dump_backtrace+0x0/0xf8 > [] show_stack+0x20/0x28 > [] dump_stack+0xac/0xe4 > [] panic+0xf8/0x268 > [] __unregister_prot_hook+0xa0/0x144 > [] packet_set_ring+0x280/0x5b4 > [] packet_setsockopt+0x320/0x950 > [] SyS_setsockopt+0xa4/0xd4 > > Signed-off-by: nixiaoming > Tested-by: wudesheng > --- > net/packet/af_packet.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 008a45c..0300146 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, > bool sync) > > po->running = 0; > > + mutex_lock(_mutex); > if (po->fanout) > __fanout_unlink(sk, po); > else > __dev_remove_pack(>prot_hook); > + mutex_unlock(_mutex); > > __sock_put(sk); I happened to be looking at the same or a very similar race, courtesy of syzkaller. packet_set_ring and fanout_add can race. I believe that one bug is in fanout_add removing the socket protocol hook and adding the fanout protocol hook without holding po->bind_lock. That lock ensures atomic updates to po->running and the actual protocol hook. fanout_add tests po->running without holding the lock if (!po->running) goto out; and later unconditionally unbinds the socket protocol hook and binds the fanout group protocol hook: if (refcount_read(>sk_ref) < PACKET_FANOUT_MAX) { __dev_remove_pack(>prot_hook); po->fanout = match; refcount_set(>sk_ref, refcount_read(>sk_ref) + 1); __fanout_link(sk, po); err = 0; } This can happen after packet_set_ring has already removed the protocol hook, causing the socket to be added to the fanout list twice. Testing po->running again, this time while holding the bind_lock, ensures that packet_set_ring cannot have dropped it in between: + spin_lock(>bind_lock); + if (!po->running) { + net_err_ratelimited("fanout add, but unbound sock"); + err = -EFAULT; + spin_unlock(>bind_lock); + goto out; + } + __dev_remove_pack(>prot_hook)); po->fanout = match; refcount_set(>sk_ref, refcount_read(>sk_ref) + 1); __fanout_link(sk, po); + spin_unlock(>bind_lock); I verified that the reproducer logs plenty of "fanout add, but unbound sock" messages. I intend to send this fix after cleaning it up a bit. Will take a closer look at your patch to see whether these are indeed the same bug report.
[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
If fanout_add is preempted after running po-> fanout = match and before running __fanout_link, it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink so, we need add mutex_lock(_mutex) to __unregister_prot_hook or add spin_lock(>bind_lock) before po-> fanout = match test on linux 4.1.12: ./trinity -c setsockopt -C 2 -X & BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()! Kernel panic - not syncing: BUG! CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW O4.1.12 #1 Hardware name: Hisilicon PhosphorHi1382 FPGA (DT) Call trace: [] dump_backtrace+0x0/0xf8 [] show_stack+0x20/0x28 [] dump_stack+0xac/0xe4 [] panic+0xf8/0x268 [] __unregister_prot_hook+0xa0/0x144 [] packet_set_ring+0x280/0x5b4 [] packet_setsockopt+0x320/0x950 [] SyS_setsockopt+0xa4/0xd4 Signed-off-by: nixiaomingTested-by: wudesheng --- net/packet/af_packet.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 008a45c..0300146 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool sync) po->running = 0; + mutex_lock(_mutex); if (po->fanout) __fanout_unlink(sk, po); else __dev_remove_pack(>prot_hook); + mutex_unlock(_mutex); __sock_put(sk); -- 2.11.0.1
[PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
If fanout_add is preempted after running po-> fanout = match and before running __fanout_link, it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink so, we need add mutex_lock(_mutex) to __unregister_prot_hook or add spin_lock(>bind_lock) before po-> fanout = match test on linux 4.1.42: ./trinity -c setsockopt -C 2 -X & BUG: failure at net/packet/af_packet.c:1414/__fanout_unlink()! Kernel panic - not syncing: BUG! CPU: 2 PID: 2271 Comm: trinity-c0 Tainted: GW O4.1.12 #1 Hardware name: Hisilicon PhosphorHi1382 FPGA (DT) Call trace: [] dump_backtrace+0x0/0xf8 [] show_stack+0x20/0x28 [] dump_stack+0xac/0xe4 [] panic+0xf8/0x268 [] __unregister_prot_hook+0xa0/0x144 [] packet_set_ring+0x280/0x5b4 [] packet_setsockopt+0x320/0x950 [] SyS_setsockopt+0xa4/0xd4 Signed-off-by: nixiaomingTested-by: wudesheng --- net/packet/af_packet.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 008a45c..0300146 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -365,10 +365,12 @@ static void __unregister_prot_hook(struct sock *sk, bool sync) po->running = 0; + mutex_lock(_mutex); if (po->fanout) __fanout_unlink(sk, po); else __dev_remove_pack(>prot_hook); + mutex_unlock(_mutex); __sock_put(sk); -- 2.11.0.1