Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[David Miller]

From: Christoph Paasch 
Date: Wed, 16 Sep 2015 22:41:14 -0700

> Hello,
> 
> On 10/08/15 - 11:00:15, David Miller wrote:
>> From: Daniel Borkmann 
>> Date: Fri,  7 Aug 2015 00:26:41 +0200
>> > Linus reports the following deadlock on rtnl_mutex; triggered only
>> > once so far (extract):
>>  ...
>> > It seems so far plausible that the recursive call into rtnetlink_rcv()
>> > looks suspicious. One way, where this could trigger is that the senders
>> > NETLINK_CB(skb).portid was wrongly 0 (which is rtnetlink socket), so
>> > the rtnl_getlink() request's answer would be sent to the kernel instead
>> > to the actual user process, thus grabbing rtnl_mutex() twice.
>> > 
>> > One theory would be that netlink_autobind() triggered via netlink_sendmsg()
>> > internally overwrites the -EBUSY error to 0, but where it is wrongly
>> > originating from __netlink_insert() instead. That would reset the
>> > socket's portid to 0, which is then filled into NETLINK_CB(skb).portid
>> > later on. As commit d470e3b483dc ("[NETLINK]: Fix two socket hashing 
>> > bugs.")
>> > also puts it, -EBUSY should not be propagated from netlink_insert().
>> > 
>> > It looks like it's very unlikely to reproduce. We need to trigger the
>> > rhashtable_insert_rehash() handler under a situation where rehashing
>> > currently occurs (one /rare/ way would be to hit ht->elasticity limits
>> > while not filled enough to expand the hashtable, but that would rather
>> > require a specifically crafted bind() sequence with knowledge about
>> > destination slots, seems unlikely). It probably makes sense to guard
>> > __netlink_insert() in any case and remap that error. It was suggested
>> > that EOVERFLOW might be better than an already overloaded ENOMEM.
>> > 
>> > Reference: http://thread.gmane.org/gmane.linux.network/372676
>> > Reported-by: Linus Torvalds 
>> > Signed-off-by: Daniel Borkmann 
>> 
>> Applied and queued up for -stable, thanks.
> 
> can this patch get queued up for 4.1 as well?
> It seems to fix a similar issue in 4.1.6.

Done.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Daniel Borkmann]

On 09/18/2015 04:09 AM, Herbert Xu wrote:

On Thu, Sep 17, 2015 at 11:47:12AM -0700, Linus Torvalds wrote:

On Wed, Sep 16, 2015 at 10:41 PM, Christoph Paasch
 wrote:


can this patch get queued up for 4.1 as well?
It seems to fix a similar issue in 4.1.6.


I think Herbert has an additional patch for this issue. But yes, I
think should be scheduled for stable. Herbert?


Yes this should be safe for stable, even though the real cause of
the problem is probably the one that Tejun spotted.


Yes, agreed.

Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Linus Torvalds]

On Wed, Sep 16, 2015 at 10:41 PM, Christoph Paasch
 wrote:
>
> can this patch get queued up for 4.1 as well?
> It seems to fix a similar issue in 4.1.6.

I think Herbert has an additional patch for this issue. But yes, I
think should be scheduled for stable. Herbert?

Linus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Herbert Xu]

On Thu, Sep 17, 2015 at 11:47:12AM -0700, Linus Torvalds wrote:
> On Wed, Sep 16, 2015 at 10:41 PM, Christoph Paasch
>  wrote:
> >
> > can this patch get queued up for 4.1 as well?
> > It seems to fix a similar issue in 4.1.6.
> 
> I think Herbert has an additional patch for this issue. But yes, I
> think should be scheduled for stable. Herbert?

Yes this should be safe for stable, even though the real cause of
the problem is probably the one that Tejun spotted.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[David Miller]

From: Daniel Borkmann dan...@iogearbox.net
Date: Fri,  7 Aug 2015 00:26:41 +0200

 Linus reports the following deadlock on rtnl_mutex; triggered only
 once so far (extract):
 ...
 It seems so far plausible that the recursive call into rtnetlink_rcv()
 looks suspicious. One way, where this could trigger is that the senders
 NETLINK_CB(skb).portid was wrongly 0 (which is rtnetlink socket), so
 the rtnl_getlink() request's answer would be sent to the kernel instead
 to the actual user process, thus grabbing rtnl_mutex() twice.
 
 One theory would be that netlink_autobind() triggered via netlink_sendmsg()
 internally overwrites the -EBUSY error to 0, but where it is wrongly
 originating from __netlink_insert() instead. That would reset the
 socket's portid to 0, which is then filled into NETLINK_CB(skb).portid
 later on. As commit d470e3b483dc ([NETLINK]: Fix two socket hashing bugs.)
 also puts it, -EBUSY should not be propagated from netlink_insert().
 
 It looks like it's very unlikely to reproduce. We need to trigger the
 rhashtable_insert_rehash() handler under a situation where rehashing
 currently occurs (one /rare/ way would be to hit ht-elasticity limits
 while not filled enough to expand the hashtable, but that would rather
 require a specifically crafted bind() sequence with knowledge about
 destination slots, seems unlikely). It probably makes sense to guard
 __netlink_insert() in any case and remap that error. It was suggested
 that EOVERFLOW might be better than an already overloaded ENOMEM.
 
 Reference: http://thread.gmane.org/gmane.linux.network/372676
 Reported-by: Linus Torvalds torva...@linux-foundation.org
 Signed-off-by: Daniel Borkmann dan...@iogearbox.net

Applied and queued up for -stable, thanks.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Thomas Graf]

On 08/07/15 at 12:26am, Daniel Borkmann wrote:
 Linus reports the following deadlock on rtnl_mutex; triggered only
 once so far (extract):
 
[...] 
 Reference: http://thread.gmane.org/gmane.linux.network/372676
 Reported-by: Linus Torvalds torva...@linux-foundation.org
 Signed-off-by: Daniel Borkmann dan...@iogearbox.net

Acked-by: Thomas Graf tg...@suug.ch
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Daniel Borkmann]

Linus reports the following deadlock on rtnl_mutex; triggered only
once so far (extract):

[12236.694209] NetworkManager  D 00013b80 0  1047  1 0x
[12236.694218]  88003f902640  815d15a9 
0018
[12236.694224]  880119538000 88003f902640 81a8ff84 

[12236.694230]  81a8ff88 880119c47f00 815d133a 
81a8ff80
[12236.694235] Call Trace:
[12236.694250]  [815d15a9] ? schedule_preempt_disabled+0x9/0x10
[12236.694257]  [815d133a] ? schedule+0x2a/0x70
[12236.694263]  [815d15a9] ? schedule_preempt_disabled+0x9/0x10
[12236.694271]  [815d2c3f] ? __mutex_lock_slowpath+0x7f/0xf0
[12236.694280]  [815d2cc6] ? mutex_lock+0x16/0x30
[12236.694291]  [814f1f90] ? rtnetlink_rcv+0x10/0x30
[12236.694299]  [8150ce3b] ? netlink_unicast+0xfb/0x180
[12236.694309]  [814f5ad3] ? rtnl_getlink+0x113/0x190
[12236.694319]  [814f202a] ? rtnetlink_rcv_msg+0x7a/0x210
[12236.694331]  [8124565c] ? sock_has_perm+0x5c/0x70
[12236.694339]  [814f1fb0] ? rtnetlink_rcv+0x30/0x30
[12236.694346]  [8150d62c] ? netlink_rcv_skb+0x9c/0xc0
[12236.694354]  [814f1f9f] ? rtnetlink_rcv+0x1f/0x30
[12236.694360]  [8150ce3b] ? netlink_unicast+0xfb/0x180
[12236.694367]  [8150d344] ? netlink_sendmsg+0x484/0x5d0
[12236.694376]  [810a236f] ? __wake_up+0x2f/0x50
[12236.694387]  [814cad23] ? sock_sendmsg+0x33/0x40
[12236.694396]  [814cb05e] ? ___sys_sendmsg+0x22e/0x240
[12236.694405]  [814cab75] ? ___sys_recvmsg+0x135/0x1a0
[12236.694415]  [811a9d12] ? eventfd_write+0x82/0x210
[12236.694423]  [811a0f9e] ? fsnotify+0x32e/0x4c0
[12236.694429]  [8108cb70] ? wake_up_q+0x60/0x60
[12236.694434]  [814cba09] ? __sys_sendmsg+0x39/0x70
[12236.694440]  [815d4797] ? entry_SYSCALL_64_fastpath+0x12/0x6a

It seems so far plausible that the recursive call into rtnetlink_rcv()
looks suspicious. One way, where this could trigger is that the senders
NETLINK_CB(skb).portid was wrongly 0 (which is rtnetlink socket), so
the rtnl_getlink() request's answer would be sent to the kernel instead
to the actual user process, thus grabbing rtnl_mutex() twice.

One theory would be that netlink_autobind() triggered via netlink_sendmsg()
internally overwrites the -EBUSY error to 0, but where it is wrongly
originating from __netlink_insert() instead. That would reset the
socket's portid to 0, which is then filled into NETLINK_CB(skb).portid
later on. As commit d470e3b483dc ([NETLINK]: Fix two socket hashing bugs.)
also puts it, -EBUSY should not be propagated from netlink_insert().

It looks like it's very unlikely to reproduce. We need to trigger the
rhashtable_insert_rehash() handler under a situation where rehashing
currently occurs (one /rare/ way would be to hit ht-elasticity limits
while not filled enough to expand the hashtable, but that would rather
require a specifically crafted bind() sequence with knowledge about
destination slots, seems unlikely). It probably makes sense to guard
__netlink_insert() in any case and remap that error. It was suggested
that EOVERFLOW might be better than an already overloaded ENOMEM.

Reference: http://thread.gmane.org/gmane.linux.network/372676
Reported-by: Linus Torvalds torva...@linux-foundation.org
Signed-off-by: Daniel Borkmann dan...@iogearbox.net
---
 net/netlink/af_netlink.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index d8e2e39..67d2104 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1096,6 +1096,11 @@ static int netlink_insert(struct sock *sk, u32 portid)
 
err = __netlink_insert(table, sk);
if (err) {
+   /* In case the hashtable backend returns with -EBUSY
+* from here, it must not escape to the caller.
+*/
+   if (unlikely(err == -EBUSY))
+   err = -EOVERFLOW;
if (err == -EEXIST)
err = -EADDRINUSE;
nlk_sk(sk)-portid = 0;
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] netlink: make sure -EBUSY won't escape from netlink_insert

[Herbert Xu]

On Fri, Aug 07, 2015 at 12:26:41AM +0200, Daniel Borkmann wrote:

 Reference: http://thread.gmane.org/gmane.linux.network/372676
 Reported-by: Linus Torvalds torva...@linux-foundation.org
 Signed-off-by: Daniel Borkmann dan...@iogearbox.net

Acked-by: Herbert Xu herb...@gondor.apana.org.au

But as I said earlier, this should not happen and if it does,
then our rhashtable is setup is broken.

Cheers,
-- 
Email: Herbert Xu herb...@gondor.apana.org.au
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html