Potential double mutex_lock bug in net/ceph/auth.c

Potential double mutex_lock bug in net/ceph/auth.c

[Iago Abal]

Hi,

I'm testing a static bug finder (EBA) on Linux 4.7 release candidates
and I may have found a potential double lock:

  Double lock in net/ceph/auth.c
  second lock at 108: mutex_lock(& ac->mutex); [ceph_auth_build_hello]
  after calling from 263: ret = ceph_auth_build_hello(ac, msg_buf, msg_len);
  if ! ac->protocol -> true at 262
  first lock at 261: mutex_lock(& ac->mutex); [ceph_build_auth]

This seems to have been introduced by commit e9966076cdd9 ("libceph:
wrap auth methods in a mutex").

I hope it helps!

Thanks,

-- iago

Fwd: Potential double mutex_lock bug in net/ceph/auth.c

[Iago Abal]

Hi,

I'm testing a static bug finder (EBA) on Linux 4.7 release candidates
and I may have found a potential double lock:

  Double lock in net/ceph/auth.c
  second lock at 108: mutex_lock(& ac->mutex); [ceph_auth_build_hello]
  after calling from 263: ret = ceph_auth_build_hello(ac, msg_buf, msg_len);
  if ! ac->protocol -> true at 262
  first lock at 261: mutex_lock(& ac->mutex); [ceph_build_auth]

This seems to have been introduced by commit e9966076cdd9 ("libceph:
wrap auth methods in a mutex").

If that's in fact a bug then I can help with a patch.

Thanks,

-- iago

Re: Potential double mutex_lock bug in net/ceph/auth.c

[Ilya Dryomov]

On Tue, Sep 20, 2016 at 11:26 PM, Iago Abal <iari@itu.dk> wrote:
> Hi,
>
> I may have found a double mutex_lock in net/ceph/auth.c and, if confirmed, I
> could help with a patch. The trace leading to the error is as follows:
>
>   1. Starting in function ceph_build_auth, the first lock is at 261:
> mutex_lock(& ac->mutex);
>       (see
> https://github.com/torvalds/linux/blob/master/net/ceph/auth.c#L261)
>   2. Assume that condition (!ac->protocol) evaluates to true at 262.
>   3. Call function ceph_auth_build_hello at 263, passing the same `ac'.
>   4. The second lock is at 108: mutex_lock(& ac->mutex);
>       (see
> https://github.com/torvalds/linux/blob/master/net/ceph/auth.c#L108)
>
> This seems to have been introduced by commit e9966076cdd9 ("libceph: wrap
> auth methods in a mutex").
>
> Function ceph_auth_build_hello is extern, so it makes sense that it takes
> the ac->mutex lock itself. On the other hand, ceph_build_auth_request is
> static, so it's fine if the caller is responsible for acquiring the
> ac->mutex.
>
> Assuming that this is a bug I can think of two simple solutions:
>
> Solution 1: Add an extra argument to ceph_auth_build_hello(..., int lock) so
> that the caller can decide whether to acquire the lock or not.
>
> Solution 2: Rewrite lines 261-266 so that ceph_auth_build_hello is called
> without the lock held. For instance,
>
>      if (!ac->protocol) {
> +       mutex_unlock(ac->mutex);
>           ret = ceph_auth_build_hello(ac, msg_buf, msg_len);
>      }
>
> PS: I have reported this bug back in June, but I didn't receive any
> feedback. I believe this can be an actual bug, so I'm insisting. :-)

Hi Iago,

Sorry about that.  I think it doesn't come up in practice because
ceph_build_auth() is only called when validating the existing auth,
which means ac->protocol != 0.

I'll take a look.

Thanks,

                Ilya