Skip to contentSkip to conversation

inet_csk_get_port lock imbalance?

DISCUSSION Jiri Slaby 2 participants 3 messages Jun 18, 2009

Subsystems (2)
lore
Share & tools
lore

Jiri Slaby

author
#0 lore raw

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.

#1 lore raw
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 :-)

author
#2 lore raw
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.