Re: [PATCH] dlm: don't return a lkb that has no rsb from find_lkb()

[Alexander Aring]

Hi,

On Wed, Sep 9, 2026 at 7:21 AM Yogesh Gaur <yogeshgaur.83@xxxxxxxxx> wrote:
>
> _create_lkb() publishes a new lkb in ls_lkbxa, which is what assigns its
> lkb_id:
>
>         rv = xa_alloc(&ls->ls_lkbxa, &lkb->lkb_id, lkb, limit, GFP_ATOMIC);
>
> but the lkb only gets an rsb later, once request_lock() has resolved the
> resource name and calls attach_lkb():
>
>         static void attach_lkb(struct dlm_rsb *r, struct dlm_lkb *lkb)
>         {
>                 hold_rsb(r);
>                 lkb->lkb_resource = r;
>         }
>
> So between those two points the lkb is fully addressable by its lkb_id
> while lkb_resource is still NULL. find_lkb() will hand it out, and its
> callers all go straight for the rsb without checking:
>
>         r = lkb->lkb_resource;
>
>         hold_rsb(r);
>         lock_rsb(r);
>
> For the userspace API the lkid is simply whatever was written to the
> misc device, so a lkid can be aimed at a lkb that is still being built
> by another thread. hold_rsb() then reads res_flags off NULL:
>
>   BUG: KASAN: null-ptr-deref in rsb_flag fs/dlm/dlm_internal.h:386 [inline]
>   BUG: KASAN: null-ptr-deref in hold_rsb fs/dlm/lock.c:334 [inline]
>   BUG: KASAN: null-ptr-deref in unlock_lock fs/dlm/lock.c:3333 [inline]
>   BUG: KASAN: null-ptr-deref in dlm_user_unlock+0x2ab/0x690 fs/dlm/lock.c:5956
>   Read of size 8 at addr 0000000000000050 by task syz.3.570/7893
>    rsb_flag fs/dlm/dlm_internal.h:386 [inline]
>    hold_rsb fs/dlm/lock.c:334 [inline]
>    unlock_lock fs/dlm/lock.c:3333 [inline]
>    dlm_user_unlock+0x2ab/0x690 fs/dlm/lock.c:5956
>    device_user_unlock+0x1ca/0x260 fs/dlm/user.c:321
>    device_write+0x905/0xed0 fs/dlm/user.c:590
>
> Reject an unattached lkb in find_lkb() rather than in each caller. Every
> find_lkb() caller dereferences lkb->lkb_resource -- convert_lock(),
> unlock_lock() and cancel_lock() through the r = lkb->lkb_resource above,
> dlm_recover_process_copy() the same way, add_to_waiters() via
> lkb->lkb_resource->res_ls -- so none of them wants a half-built lkb, and
> a lkb without an rsb is not a lock anyone outside can name yet. Callers
> already handle find_lkb() failing.
>
> Testing lkb_resource under ls_lkbxa_lock next to the existing kref_read()
> check is enough. The value is not stable under that lock, as attach_lkb()
> does not take it, but it does not need to be: once a non-NULL rsb has
> been observed it stays attached for the life of the reference taken here,
> because detach_lkb() only runs from __put_lkb() on the last reference.
> Observing NULL while attach_lkb() races is the case being rejected, and
> the thread still inside request_lock() has not returned the lkid to
> anyone at that point.
>
> This is the null-ptr-deref only. The refcount warning syzbot reports in
> dlm_user_request() itself, where hold_lkb() runs on a lkb whose count
> already reached zero, is a separate race on an lkb that is past
> attach_lkb() and is not addressed here.
>
> The unchecked r = lkb->lkb_resource goes back to the original DLM
> import, but an untrusted lkid only became possible once the userspace
> device interface was added, so that is the tag below.

I agree that the one design flaws of DLM is actually to have lkbs
around that does not have rsbs (lkb->lkb_resource) set, which makes
troubel all over the place if somebody forgets about this case...

It should not be possible to have lkbs around, e.g., in xarray of
"ls->ls_lkbxa" and the rsb is not set, however this is not what your
patch is doing. Your patch is simply to not return a lkb when there is
no lkb->lkb_resource set. It seems we can run into this case as the
above KASAN syzbot shows, although I am worried about cases where we
know it is not set and we handle it.
I would more suggest changing the patch so that it's impossible to
have lkbs around without an rsb being set.

- Alex