Re: [PATCH v2] dlm: don't publish a lkb in ls_lkbxa before it has an rsb

[Alexander Aring]

Hi,

On Sun, Oct 4, 2026 at 1:19 AM Yogesh Gaur <yogeshgaur.83@xxxxxxxxx> wrote:
>
> _create_lkb() puts a new lkb into ls_lkbxa straight away, which is also
> what assigns its lkb_id, but the lkb only gets an rsb later, when
> request_lock(), receive_request(), dlm_recover_master_copy() or
> dlm_debug_add_lkb() call attach_lkb(). In between, find_lkb() hands the
> lkb out with lkb_resource still NULL, and its callers go straight for
> the rsb:
>
>         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 another thread is
> still building, and hold_rsb() 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
>
> Make it impossible for ls_lkbxa to hold a lkb without an rsb instead of
> filtering such lkbs in find_lkb(). _create_lkb() now only reserves the
> lkb_id, by passing a NULL entry to xa_alloc(), so xa_load() returns NULL
> for it; attach_lkb() stores the lkb once lkb_resource is set. All four
> attach_lkb() callers hold the rsb lock, and nothing takes an rsb lock
> under ls_lkbxa_lock, so taking it there adds no new lock ordering.
>
> An lkb that never gets an rsb is still released by __put_lkb(), whose
> xa_erase() frees the reserved id as before, and detach_lkb() already
> copes with a NULL lkb_resource. The lkb_id is still assigned at create
> time, so it is available to tracing and to the caller as before.
>
> The xa_for_each() walks in lockspace.c skip reserved entries. For
> lockspace_busy() that means an lkb still between create and attach no
> longer makes the lockspace busy. That is the same as the request
> arriving just after the check, which can already happen; the caller
> holds a lockspace reference, which remove_lockspace() waits for.
>
> 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.
>
> Fixes: 597d0cae0f99 ("[DLM] dlm: user locks")
> Reported-by: syzbot+da6dc573ce5e6624f505@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=da6dc573ce5e6624f505
> Suggested-by: Alexander Aring <aahringo@xxxxxxxxxx>
> Assisted-by: LLM
>
> ---
> v2: Following review, stop ls_lkbxa from ever holding
>     a lkb without an rsb, rather than filtering such lkbs in find_lkb().
>     _create_lkb() now only reserves the id; attach_lkb() publishes the
>     lkb. Subject changed to match.
> v1: https://lore.kernel.org/all/20260909112108.2281-1-yogeshgaur.83@xxxxxxxxx/
>
> Not runtime-tested; syzbot has no reproducer for this report. Built
> with W=1.
>
> ---
>  fs/dlm/lock.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/dlm/lock.c b/fs/dlm/lock.c
> index 2609e4fdeba8..2b7f3ff94310 100644
> --- a/fs/dlm/lock.c
> +++ b/fs/dlm/lock.c
> @@ -1488,10 +1488,22 @@ void free_inactive_rsb(struct dlm_rsb *r)
>  /* Attaching/detaching lkb's from rsb's is for rsb reference counting.
>     The rsb must exist as long as any lkb's for it do. */
>
> +/*
> + * An lkb only becomes visible to find_lkb() here, once it has an rsb.
> + * _create_lkb() just reserves its lkb_id in ls_lkbxa.
> + */
>  static void attach_lkb(struct dlm_rsb *r, struct dlm_lkb *lkb)
>  {
> +       struct dlm_ls *ls = r->res_ls;
> +       void *old;
> +
>         hold_rsb(r);
>         lkb->lkb_resource = r;
> +
> +       write_lock_bh(&ls->ls_lkbxa_lock);
> +       old = xa_store(&ls->ls_lkbxa, lkb->lkb_id, lkb, GFP_ATOMIC);
> +       write_unlock_bh(&ls->ls_lkbxa_lock);
> +       WARN_ON_ONCE(old);
>  }
>
>  static void detach_lkb(struct dlm_lkb *lkb)
> @@ -1525,8 +1537,9 @@ static int _create_lkb(struct dlm_ls *ls, struct dlm_lkb **lkb_ret,
>         INIT_LIST_HEAD(&lkb->lkb_ownqueue);
>         INIT_LIST_HEAD(&lkb->lkb_rsb_lookup);
>
> +       /* reserve the id only, attach_lkb() publishes the lkb */
>         write_lock_bh(&ls->ls_lkbxa_lock);
> -       rv = xa_alloc(&ls->ls_lkbxa, &lkb->lkb_id, lkb, limit, GFP_ATOMIC);
> +       rv = xa_alloc(&ls->ls_lkbxa, &lkb->lkb_id, NULL, limit, GFP_ATOMIC);

Now, all potential iterators need to check if there are reserved lkbs
(being NULL) in ls_lkbxa which makes it not better.

What I meant what ls_lkbxa should contain only valid lkbs (not
reserved as being NULL) that have a lkb_resource set up and not being
NULL. An lkb should always have an rsb.

- Alex