On Fri, Dec 16, 2016 at 5:18 PM, Tom Herbert <t...@herbertland.com>
wrote:
On Fri, Dec 16, 2016 at 2:08 PM, Josef Bacik <jba...@fb.com> wrote:
On Fri, Dec 16, 2016 at 10:21 AM, Josef Bacik <jba...@fb.com> wrote:
On Fri, Dec 16, 2016 at 9:54 AM, Josef Bacik <jba...@fb.com> wrote:
On Thu, Dec 15, 2016 at 7:07 PM, Hannes Frederic Sowa
<han...@stressinduktion.org> wrote:
Hi Josef,
On 15.12.2016 19:53, Josef Bacik wrote:
On Tue, Dec 13, 2016 at 6:32 PM, Tom Herbert
<t...@herbertland.com>
wrote:
On Tue, Dec 13, 2016 at 3:03 PM, Craig Gallek
<kraigatg...@gmail.com>
wrote:
On Tue, Dec 13, 2016 at 3:51 PM, Tom Herbert
<t...@herbertland.com>
wrote:
I think there may be some suspicious code in
inet_csk_get_port. At
tb_found there is:
if (((tb->fastreuse > 0 && reuse) ||
(tb->fastreuseport > 0 &&
!rcu_access_pointer(sk->sk_reuseport_cb) &&
sk->sk_reuseport &&
uid_eq(tb->fastuid,
uid))) &&
smallest_size == -1)
goto success;
if
(inet_csk(sk)->icsk_af_ops->bind_conflict(sk,
tb, true)) {
if ((reuse ||
(tb->fastreuseport > 0 &&
sk->sk_reuseport &&
!rcu_access_pointer(sk->sk_reuseport_cb) &&
uid_eq(tb->fastuid, uid))) &&
smallest_size != -1 &&
--attempts >=
0) {
spin_unlock_bh(&head->lock);
goto again;
}
goto fail_unlock;
}
AFAICT there is redundancy in these two conditionals. The
same
clause
is being checked in both: (tb->fastreuseport > 0 &&
!rcu_access_pointer(sk->sk_reuseport_cb) &&
sk->sk_reuseport &&
uid_eq(tb->fastuid, uid))) && smallest_size == -1. If this
is true
the
first conditional should be hit, goto done, and the
second will
never
evaluate that part to true-- unless the sk is changed (do
we need
READ_ONCE for sk->sk_reuseport_cb?).
That's an interesting point... It looks like this function
also
changed in 4.6 from using a single local_bh_disable() at the
beginning
with several spin_lock(&head->lock) to exclusively
spin_lock_bh(&head->lock) at each locking point. Perhaps
the full
bh
disable variant was preventing the timers in your stack
trace from
running interleaved with this function before?
Could be, although dropping the lock shouldn't be able to
affect the
search state. TBH, I'm a little lost in reading function, the
SO_REUSEPORT handling is pretty complicated. For instance,
rcu_access_pointer(sk->sk_reuseport_cb) is checked three
times in
that
function and also in every call to inet_csk_bind_conflict. I
wonder
if
we can simply this under the assumption that SO_REUSEPORT is
only
allowed if the port number (snum) is explicitly specified.
Ok first I have data for you Hannes, here's the time
distributions
before during and after the lockup (with all the debugging in
place
the
box eventually recovers). I've attached it as a text file
since it is
long.
Thanks a lot!
Second is I was thinking about why we would spend so much time
doing
the
->owners list, and obviously it's because of the massive
amount of
timewait sockets on the owners list. I wrote the following
dumb patch
and tested it and the problem has disappeared completely. Now
I don't
know if this is right at all, but I thought it was weird we
weren't
copying the soreuseport option from the original socket onto
the twsk.
Is there are reason we aren't doing this currently? Does this
help
explain what is happening? Thanks,
The patch is interesting and a good clue, but I am immediately a
bit
concerned that we don't copy/tag the socket with the uid also to
keep
the security properties for SO_REUSEPORT. I have to think a bit
more
about this.
We have seen hangs during connect. I am afraid this patch
wouldn't help
there while also guaranteeing uniqueness.
Yeah so I looked at the code some more and actually my patch is
really
bad. If sk2->sk_reuseport is set we'll look at
sk2->sk_reuseport_cb, which
is outside of the timewait sock, so that's definitely bad.
But we should at least be setting it to 0 so that we don't do this
normally. Unfortunately simply setting it to 0 doesn't fix the
problem. So
for some reason having ->sk_reuseport set to 1 on a timewait
socket makes
this problem non-existent, which is strange.
So back to the drawing board I guess. I wonder if doing what
craig
suggested and batching the timewait timer expires so it hurts
less would
accomplish the same results. Thanks,
Wait no I lied, we access the sk->sk_reuseport_cb, not sk2's.
This is the
code
if ((!reuse || !sk2->sk_reuse ||
sk2->sk_state == TCP_LISTEN) &&
(!reuseport || !sk2->sk_reuseport ||
rcu_access_pointer(sk->sk_reuseport_cb) ||
(sk2->sk_state != TCP_TIME_WAIT &&
!uid_eq(uid, sock_i_uid(sk2))))) {
if (!sk2->sk_rcv_saddr ||
!sk->sk_rcv_saddr
||
sk2->sk_rcv_saddr ==
sk->sk_rcv_saddr)
break;
}
so in my patches case we now have reuseport == 1,
sk2->sk_reuseport == 1.
But now we are using reuseport, so sk->sk_reuseport_cb should be
non-NULL
right? So really setting the timewait sock's sk_reuseport should
have no
bearing on how this loop plays out right? Thanks,
So more messing around and I noticed that we basically don't do the
tb->fastreuseport logic at all if we've ended up with a non
SO_REUSEPORT
socket on that tb. So before I fully understood what I was doing I
fixed it
so that after we go through ->bind_conflict() once with a
SO_REUSEPORT
socket, we reset tb->fastreuseport to 1 and set the uid to match
the uid of
the socket. This made the problem go away. Tom pointed out that
if we bind
to the same port on a different address and we have a non
SO_REUSEPORT
socket with the same address on this tb then we'd be screwed with
my code.
Which brings me to his proposed solution. We need another hash
table that
is indexed based on the binding address. Then each node
corresponds to one
address/port binding, with non-SO_REUSEPORT entries having only one
entry,
and normal SO_REUSEPORT entries having many. This cleans up the
need to
search all the possible sockets on any given tb, we just go and
look at the
one we care about. Does this make sense? Thanks,
Hi Josef,
Thinking about it some more the hash table won't work because of the
rules of binding different addresses to the same port. What I think we
can do is to change inet_bind_bucket to be structure that contains all
the information used to detect conflicts (reuse*, if, address, uid,
etc.) and a list of sockets that share that exact same information--
for instance all socket in timewait state create through some listener
socket should wind up on single bucket. When we do the bind_conflict
function we only should have to walk this buckets, not the full list
of sockets.
Thoughts on this?
This sounds good, maybe tb->owners be a list of say
struct inet_unique_shit {
struct sock_common sk;
struct hlist socks;
};
Then we make inet_unique_shit like twsks', just copy the relevant
information, then hang the real sockets off of the socks hlist.
Something like that? Thanks,
Josef
