inet_csk_get_port lock imbalance?

inet_csk_get_port lock imbalance?

[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?

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.

Re: inet_csk_get_port lock imbalance?

[Florian Westphal]

Jiri Slaby <jirislaby@gmail.com> wrote:
> 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);

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 :-)

Re: inet_csk_get_port lock imbalance?

[Jiri Slaby]

On 06/18/2009 04:14 PM, Florian Westphal wrote:
> Jiri Slaby <jirislaby@gmail.com> wrote:
>> 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.