| Message ID | CABdVr8RVXj3dg-by_W99=fPSjdfB7fYXFRBW9sropQpYKD1xOQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Possible read of uninitialized array values in reuseport_select_sock? | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Wed, Jan 6, 2021 at 10:54 PM Baptiste Lepers <baptiste.lepers@gmail.com> wrote: > > Hello, > > While reading the code of net/core/sock_reuseport.c, I think I found a > possible race in reuseport_select_sock. The current code has the > following pattern: > > socks = READ_ONCE(reuse->num_socks); > smp_rmb(); // paired with reuseport_add_sock to make sure > reuse->socks[i < num_socks] are initialized > while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { > if (i >= reuse->num_socks) // should be "socks" and not > "reuse->num_socks"? > > The barrier seems to be here to make sure that all items of > reuse->socks are initialized before being read, but the barrier only > protects indexes < socks, not indexes < reuse->num_socks. > > I have a patch ready to fix this issue, but I wanted to confirm that > the current code is indeed incorrect (if the code is correct for a > non-obvious reason, I'd be happy to add some comments to document the > behavior). > > > Here is the diff of the patch I was planning to submit: > > diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c > index bbdd3c7b6cb5..b065f0a103ed 100644 > --- a/net/core/sock_reuseport.c > +++ b/net/core/sock_reuseport.c > @@ -293,7 +293,7 @@ struct sock *reuseport_select_sock(struct sock *sk, > i = j = reciprocal_scale(hash, socks); > while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { > i++; > - if (i >= reuse->num_socks) > + if (i >= socks) > i = 0; > if (i == j) > goto out; > > > Thanks, > Baptiste. Thanks for the clear description. Yes, I believe you're correct.
Thanks. I will submit a patch. Baptiste. On Thu, Jan 7, 2021 at 3:57 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Wed, Jan 6, 2021 at 10:54 PM Baptiste Lepers > <baptiste.lepers@gmail.com> wrote: > > > > Hello, > > > > While reading the code of net/core/sock_reuseport.c, I think I found a > > possible race in reuseport_select_sock. The current code has the > > following pattern: > > > > socks = READ_ONCE(reuse->num_socks); > > smp_rmb(); // paired with reuseport_add_sock to make sure > > reuse->socks[i < num_socks] are initialized > > while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { > > if (i >= reuse->num_socks) // should be "socks" and not > > "reuse->num_socks"? > > > > The barrier seems to be here to make sure that all items of > > reuse->socks are initialized before being read, but the barrier only > > protects indexes < socks, not indexes < reuse->num_socks. > > > > I have a patch ready to fix this issue, but I wanted to confirm that > > the current code is indeed incorrect (if the code is correct for a > > non-obvious reason, I'd be happy to add some comments to document the > > behavior). > > > > > > Here is the diff of the patch I was planning to submit: > > > > diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c > > index bbdd3c7b6cb5..b065f0a103ed 100644 > > --- a/net/core/sock_reuseport.c > > +++ b/net/core/sock_reuseport.c > > @@ -293,7 +293,7 @@ struct sock *reuseport_select_sock(struct sock *sk, > > i = j = reciprocal_scale(hash, socks); > > while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { > > i++; > > - if (i >= reuse->num_socks) > > + if (i >= socks) > > i = 0; > > if (i == j) > > goto out; > > > > > > Thanks, > > Baptiste. > > Thanks for the clear description. Yes, I believe you're correct.
diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c index bbdd3c7b6cb5..b065f0a103ed 100644 --- a/net/core/sock_reuseport.c +++ b/net/core/sock_reuseport.c @@ -293,7 +293,7 @@ struct sock *reuseport_select_sock(struct sock *sk, i = j = reciprocal_scale(hash, socks); while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { i++; - if (i >= reuse->num_socks) + if (i >= socks) i = 0; if (i == j) goto out;
Hello, While reading the code of net/core/sock_reuseport.c, I think I found a possible race in reuseport_select_sock. The current code has the following pattern: socks = READ_ONCE(reuse->num_socks); smp_rmb(); // paired with reuseport_add_sock to make sure reuse->socks[i < num_socks] are initialized while (reuse->socks[i]->sk_state == TCP_ESTABLISHED) { if (i >= reuse->num_socks) // should be "socks" and not "reuse->num_socks"? The barrier seems to be here to make sure that all items of reuse->socks are initialized before being read, but the barrier only protects indexes < socks, not indexes < reuse->num_socks. I have a patch ready to fix this issue, but I wanted to confirm that the current code is indeed incorrect (if the code is correct for a non-obvious reason, I'd be happy to add some comments to document the behavior). Here is the diff of the patch I was planning to submit: Thanks, Baptiste.