Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312352 > unrolled thread

Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c

Back to article view | Back to linux.kernel

Contents

  Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Marc Dionne <marc.c.dionne@gmail.com> - 2016-01-19 17:00 +0100
    Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Marc Dionne <marc.c.dionne@gmail.com> - 2016-01-19 17:10 +0100
      Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Craig Gallek <kraig@google.com> - 2016-01-19 17:40 +0100
        Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Marc Dionne <marc.c.dionne@gmail.com> - 2016-01-19 18:10 +0100
          Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Craig Gallek <kraig@google.com> - 2016-01-19 19:20 +0100
            Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c Marc Dionne <marc.c.dionne@gmail.com> - 2016-01-19 20:00 +0100
    Re: Crash with SO_REUSEPORT and  ef456144da8ef507c8cf504284b6042e9201a05c Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-19 17:20 +0100

#1312352 — Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c

[Multipart message — attachments visible in raw view] — view raw

I shared this one with Craig but I thought I'd put it out to a wider audience.

Trying to run the current kernel mainline on a test system I found
that any attempt to run many of our executables would crash the
system.  The networking code in all of these opens and listens on
multiple UDP sockets set with SO_REUSEPORT.  We also like to bind the
first socket before setting SO_REUSEPORT so we can catch some cases
where the port is actually in use by someone else (for instance a
previous incarnation of the same service).

This is easily reproduced with this sequence:
- create 2 sockets A and B
- bind socket A to an address
- set SO_REUSEPORT on socket A
- set SO_REUSEPORT on socket B
- bind socket B to the same address as A

The sk_reuseport_cb structure is only allocated at bind time if
SO_REUSEPORT is already set, so A doesn't have one.  When we bind B, A
is found as a match that has SO_REUSEPORT and reuseport_add_sock will
try to use the NULL sk_reuseport_cb structure from A, causing a crash.

Not sure what the best fix is, but seems like the structure could be
either allocated (if not already done) when setting SO_REUSEPORT, or
when we find it to be NULL in reuseport_add_sock (but locking may be
an issue there).  I was able to test that allocating sk_reuseport_cb
when setting SO_REUSEPORT makes things behave normally again; see
attached patch.  That's surely not a correct/complete fix as B (in the
scenario above) will have an unnecessary sk_reuseport_cb which will
trigger a warning and should be dealt with.

Thanks,
Marc

[toc] | [next] | [standalone]

#1312358

Resent with correct address for Eric.

On Tue, Jan 19, 2016 at 11:57 AM, Marc Dionne <marc.c.dionne@gmail.com> wrote:
> I shared this one with Craig but I thought I'd put it out to a wider audience.
>
> Trying to run the current kernel mainline on a test system I found
> that any attempt to run many of our executables would crash the
> system.  The networking code in all of these opens and listens on
> multiple UDP sockets set with SO_REUSEPORT.  We also like to bind the
> first socket before setting SO_REUSEPORT so we can catch some cases
> where the port is actually in use by someone else (for instance a
> previous incarnation of the same service).
>
> This is easily reproduced with this sequence:
> - create 2 sockets A and B
> - bind socket A to an address
> - set SO_REUSEPORT on socket A
> - set SO_REUSEPORT on socket B
> - bind socket B to the same address as A
>
> The sk_reuseport_cb structure is only allocated at bind time if
> SO_REUSEPORT is already set, so A doesn't have one.  When we bind B, A
> is found as a match that has SO_REUSEPORT and reuseport_add_sock will
> try to use the NULL sk_reuseport_cb structure from A, causing a crash.
>
> Not sure what the best fix is, but seems like the structure could be
> either allocated (if not already done) when setting SO_REUSEPORT, or
> when we find it to be NULL in reuseport_add_sock (but locking may be
> an issue there).  I was able to test that allocating sk_reuseport_cb
> when setting SO_REUSEPORT makes things behave normally again; see
> attached patch.  That's surely not a correct/complete fix as B (in the
> scenario above) will have an unnecessary sk_reuseport_cb which will
> trigger a warning and should be dealt with.
>
> Thanks,
> Marc

[toc] | [prev] | [next] | [standalone]

#1312374

On Tue, Jan 19, 2016 at 11:04 AM, Marc Dionne <marc.c.dionne@gmail.com> wrote:
> Resent with correct address for Eric.
>
> On Tue, Jan 19, 2016 at 11:57 AM, Marc Dionne <marc.c.dionne@gmail.com> wrote:
>> I shared this one with Craig but I thought I'd put it out to a wider audience.
>>
>> Trying to run the current kernel mainline on a test system I found
>> that any attempt to run many of our executables would crash the
>> system.  The networking code in all of these opens and listens on
>> multiple UDP sockets set with SO_REUSEPORT.  We also like to bind the
>> first socket before setting SO_REUSEPORT so we can catch some cases
>> where the port is actually in use by someone else (for instance a
>> previous incarnation of the same service).
>>
>> This is easily reproduced with this sequence:
>> - create 2 sockets A and B
>> - bind socket A to an address
>> - set SO_REUSEPORT on socket A
>> - set SO_REUSEPORT on socket B
>> - bind socket B to the same address as A
>>
>> The sk_reuseport_cb structure is only allocated at bind time if
>> SO_REUSEPORT is already set, so A doesn't have one.  When we bind B, A
>> is found as a match that has SO_REUSEPORT and reuseport_add_sock will
>> try to use the NULL sk_reuseport_cb structure from A, causing a crash.
>>
>> Not sure what the best fix is, but seems like the structure could be
>> either allocated (if not already done) when setting SO_REUSEPORT, or
>> when we find it to be NULL in reuseport_add_sock (but locking may be
>> an issue there).  I was able to test that allocating sk_reuseport_cb
>> when setting SO_REUSEPORT makes things behave normally again; see
>> attached patch.  That's surely not a correct/complete fix as B (in the
>> scenario above) will have an unnecessary sk_reuseport_cb which will
>> trigger a warning and should be dealt with.
>>
>> Thanks,
>> Marc

There are really two issues here: The change in behavior of when you
can set SO_REUSEPORT on a bound socket and the NULL pointer
dereference race.

It's not completely safe to allocate the reuseport struct from the
setsockopt function.  Acquisition of the spin lock in reuseport_alloc
requires prior acquisition of the hlist lock for the socket (which
doesn't exist before bind).

Further, allocating this structure before bind means that it's
possible to associate two different BPF filters with two different
sockets and then try to bind them both to the same address.  This
means that the reuseport_add_sock function would need to decide to
pick between one of these two structures and get rid of the other.
There's currently a WARN_ON in that function which would complain
about this.  (the test program in
tools/testing/selftests/net/reuseport_bpf.c will trigger this warning
with your change).

I need to think about how to handle setsockopt-after-bind condition a
bit more, but the NULL pointer dereference is obviously wrong.  Do you
have a way to easily reproduce this?  I've only managed to get it to
happen once so far...

[toc] | [prev] | [next] | [standalone]

#1312391

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jan 19, 2016 at 12:31 PM, Craig Gallek <kraig@google.com> wrote:
>
> I need to think about how to handle setsockopt-after-bind condition a
> bit more, but the NULL pointer dereference is obviously wrong.  Do you
> have a way to easily reproduce this?  I've only managed to get it to
> happen once so far...

The attached code reliably triggers the crash for me.

[toc] | [prev] | [next] | [standalone]

#1312421

On Tue, Jan 19, 2016 at 12:08 PM, Marc Dionne <marc.c.dionne@gmail.com> wrote:
> On Tue, Jan 19, 2016 at 12:31 PM, Craig Gallek <kraig@google.com> wrote:
>>
>> I need to think about how to handle setsockopt-after-bind condition a
>> bit more, but the NULL pointer dereference is obviously wrong.  Do you
>> have a way to easily reproduce this?  I've only managed to get it to
>> happen once so far...
>
> The attached code reliably triggers the crash for me.

I think the patch below will address this issue (sorry in advance if
gmail screws up the whitespace...).  I'll send it for formal review
once I finish testing it.

Craig

diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index 1df98c557440..004cb2c974ac 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -97,6 +97,11 @@ int reuseport_add_sock(struct sock *sk, const
struct sock *sk2)
 {
  struct sock_reuseport *reuse;

+  if (!rcu_access_pointer(sk2->sk_reuseport_cb)) {
+   int err = reuseport_alloc(sk2);
+   if (err) return err;
+  }
+
  spin_lock_bh(&reuseport_lock);
  reuse = rcu_dereference_protected(sk2->sk_reuseport_cb,
   lockdep_is_held(&reuseport_lock)),

[toc] | [prev] | [next] | [standalone]

#1312446

On Tue, Jan 19, 2016 at 2:11 PM, Craig Gallek <kraig@google.com> wrote:
> On Tue, Jan 19, 2016 at 12:08 PM, Marc Dionne <marc.c.dionne@gmail.com> wrote:
>> On Tue, Jan 19, 2016 at 12:31 PM, Craig Gallek <kraig@google.com> wrote:
>>>
>>> I need to think about how to handle setsockopt-after-bind condition a
>>> bit more, but the NULL pointer dereference is obviously wrong.  Do you
>>> have a way to easily reproduce this?  I've only managed to get it to
>>> happen once so far...
>>
>> The attached code reliably triggers the crash for me.
>
> I think the patch below will address this issue (sorry in advance if
> gmail screws up the whitespace...).  I'll send it for formal review
> once I finish testing it.
>
> Craig
>
> diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> index 1df98c557440..004cb2c974ac 100644
> --- a/net/core/sock_reuseport.c
> +++ b/net/core/sock_reuseport.c
> @@ -97,6 +97,11 @@ int reuseport_add_sock(struct sock *sk, const
> struct sock *sk2)
>  {
>   struct sock_reuseport *reuse;
>
> +  if (!rcu_access_pointer(sk2->sk_reuseport_cb)) {
> +   int err = reuseport_alloc(sk2);
> +   if (err) return err;
> +  }
> +
>   spin_lock_bh(&reuseport_lock);
>   reuse = rcu_dereference_protected(sk2->sk_reuseport_cb,
>    lockdep_is_held(&reuseport_lock)),

That works fine, thanks..

Just wondering though, is there a bit of a race there?  Seems like it
might be safer to have a version of reuseport_alloc that doesn't take
the lock and use it here, moving the block after the lock is taken.

Marc

[toc] | [prev] | [next] | [standalone]

#1312364 — Re: Crash with SO_REUSEPORT and ef456144da8ef507c8cf504284b6042e9201a05c

On Tue, 2016-01-19 at 11:57 -0400, Marc Dionne wrote:
> I shared this one with Craig but I thought I'd put it out to a wider audience.
> 
> Trying to run the current kernel mainline on a test system I found
> that any attempt to run many of our executables would crash the
> system.  The networking code in all of these opens and listens on
> multiple UDP sockets set with SO_REUSEPORT.  We also like to bind the
> first socket before setting SO_REUSEPORT so we can catch some cases
> where the port is actually in use by someone else (for instance a
> previous incarnation of the same service).
> 
> This is easily reproduced with this sequence:
> - create 2 sockets A and B
> - bind socket A to an address
> - set SO_REUSEPORT on socket A
> - set SO_REUSEPORT on socket B
> - bind socket B to the same address as A
> 
> The sk_reuseport_cb structure is only allocated at bind time if
> SO_REUSEPORT is already set, so A doesn't have one.  When we bind B, A
> is found as a match that has SO_REUSEPORT and reuseport_add_sock will
> try to use the NULL sk_reuseport_cb structure from A, causing a crash.
> 
> Not sure what the best fix is, but seems like the structure could be
> either allocated (if not already done) when setting SO_REUSEPORT, or
> when we find it to be NULL in reuseport_add_sock (but locking may be
> an issue there).  I was able to test that allocating sk_reuseport_cb
> when setting SO_REUSEPORT makes things behave normally again; see
> attached patch.  That's surely not a correct/complete fix as B (in the
> scenario above) will have an unnecessary sk_reuseport_cb which will
> trigger a warning and should be dealt with.

Hi Marc

Your patch looks fine to me, please add a "Fixes:" tag in it ?

Fixes: e32ea7e74727 ("soreuseport: fast reuseport UDP socket selection")

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web