inet_csk_get_port lock imbalance?
Subsystems (2)
Hi,
we've found a weird locking discipline in inet_csk_get_port. Who is supposed to unlock the spin lock in the place marked in the code below?
do {
head = &hashinfo->bhash[inet_bhashfn(net, rover,
hashinfo->bhash_size)];
spin_lock(&head->lock);
inet_bind_bucket_for_each(tb, node, &head->chain)
if (ib_net(tb) == net && tb->port == rover) {
...
}
break; ////////// here
next:
spin_unlock(&head->lock);
if (++rover > high)
rover = low;
} while (--remaining > 0);Thanks.
Quoted 5 lines · Jiri Slaby
Hi,
we've found a weird locking discipline in
inet_csk_get_port. Who is supposed to unlock the spin lock in the place marked in the code below?
Quoted 14 lines
do {
head = &hashinfo->bhash[inet_bhashfn(net, rover,
hashinfo->bhash_size)];
spin_lock(&head->lock);
inet_bind_bucket_for_each(tb, node, &head->chain)
if (ib_net(tb) == net && tb->port == rover) {
...
}
break; ////////// here
next:
spin_unlock(&head->lock);
if (++rover > high)
rover = low;
} while (--remaining > 0);looks weird, but seems intentional (and ok):
} while (--remaining > 0);
-> you arrive here after break; with head->lock held.
...
if (remaining <= 0) {
...
->if you get here, then the loop was left with head->lock unlocked.
..
} else {
have_snum:
...
spin_lock(&head->lock);
-> we do not hit this code path in the "locked" case.
...
goto tb_found;
}
tb = NULL;
goto tb_not_found;-> we go to tb_not_found, with head->lock held. it cannot be in unlocked state here.
...tb_not_found:
ret = 1;
if (!tb && (tb = inet_bind_bucket_create(hashinfo->bind_bucket_cachep,
net, head, snum)) == NULL)
goto fail_unlock;
if (hlist_empty(&tb->owners)) {
if (sk->sk_reuse && sk->sk_state != TCP_LISTEN)
tb->fastreuse = 1;
else
tb->fastreuse = 0;
} else if (tb->fastreuse &&
(!sk->sk_reuse || sk->sk_state == TCP_LISTEN))
tb->fastreuse = 0;
success:
if (!inet_csk(sk)->icsk_bind_hash)
inet_bind_hash(sk, tb, snum);
WARN_ON(inet_csk(sk)->icsk_bind_hash != tb);
ret = 0;fail_unlock:
spin_unlock(&head->lock);
-> here it will be unlocked
I'd appreciate it if you consider the above a blatant lie and check it again :-)
Quoted 4 lines · Florian Westphal
Quoted 2 lines · Jiri Slaby
we've found a weird locking discipline in
inet_csk_get_port. Who is supposed to unlock the spin lock in the place marked in the code below?I'd appreciate it if you consider the above a blatant lie and check it again :-)
Yes, you are right, I went through 'if (remaining <= 0) {' branch through which it can't go. Thanks.