Re: [PATCH 06/11] dlm: Free dlm_ls in kobject release callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dlm: sysfs UAF of ls_recoverd_task during lockspace teardown
- [High] dlm: module unload UAF in dlm_ls_release under DEBUG_KOBJECT_RELEASE
--

commit 5dce188281954f951472555cf7284cda831b20a7
Author: Ian Rogers <irogers@google.com>

dlm: Free dlm_ls in kobject release callback

This commit adds a kobject release callback, dlm_ls_release(), to properly
free the dlm_ls structure when the last kobject reference is dropped. This
fixes a use-after-free issue when CONFIG_DEBUG_KOBJECT_RELEASE is enabled.

> diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
> index a9c98b4f378f3..1d226b27d8e61 100644
> --- a/fs/dlm/lockspace.c
> +++ b/fs/dlm/lockspace.c

[ ... ]

> @@ -179,7 +179,15 @@ static const struct sysfs_ops dlm_attr_ops = {
>  	.show  = dlm_attr_show,
>  	.store = dlm_attr_store,
>  };
>  
> +static void dlm_ls_release(struct kobject *kobj)
> +{
> +	struct dlm_ls *ls = container_of(kobj, struct dlm_ls, ls_kobj);
> +
> +	kfree(ls);
> +}

[Severity: High]
Can this callback trigger an instruction page fault if the module is unloaded?

Under CONFIG_DEBUG_KOBJECT_RELEASE=y, kobject release is delayed by 1-4
seconds. If the dlm module is unloaded during this window, this callback
will point to unmapped memory when the delayed work executes.

[ ... ]

> @@ -615,7 +623,6 @@ static int new_lockspace(const char *name, const char *cluster,
>  	rhashtable_destroy(&ls->ls_rsbtbl);
>   out_lsfree:
>  	kobject_put(&ls->ls_kobj);
> -	kfree(ls);
>   out:
>  	module_put(THIS_MODULE);
>  	return error;

[Severity: High]
In the error path of new_lockspace(), we drop the kobject reference and then
immediately drop the module reference via module_put(). Doesn't this allow
the module to be unloaded while the delayed kobject release is still pending?

Additionally, could this sequence open a use-after-free race window?

Earlier in the error path of new_lockspace(), we call dlm_recoverd_stop():

out_recoverd:
        dlm_recoverd_stop(ls);
out_callback:
        ...
out_lsfree:
        kobject_put(&ls->ls_kobj);

The recovery task is stopped and freed in dlm_recoverd_stop(), but the sysfs
entries aren't removed until kobject_put() is called much later. A concurrent
sysfs write to the control file could invoke dlm_ls_start() in fs/dlm/member.c
and wake up the freed ls->ls_recoverd_task.

[ ... ]

> @@ -759,7 +766,7 @@ static int release_lockspace(struct dlm_ls *ls, unsigned int release_option)
>  
>  	dlm_delete_debug_file(ls);
>  
> -	kobject_put(&ls->ls_kobj);
> +	kobject_del(&ls->ls_kobj);
>  
>  	xa_destroy(&ls->ls_recover_xa);
>  	kfree(ls->ls_recover_buf);

[Severity: High]
Is there a similar use-after-free risk involving the recovery thread here?

Looking at the context in release_lockspace(), we stop the recovery thread
earlier:

release_lockspace() {
    ...
    dlm_recoverd_stop(ls);
    ...
    clear_bit(LSFL_RUNNING, &ls->ls_flags);
    ...
    kobject_del(&ls->ls_kobj);
    ...
    module_put(THIS_MODULE);
}

Since the sysfs kobject isn't deleted until after the recovery thread is
stopped and its task_struct is freed, the sysfs entries remain active in the
interim. A concurrent sysfs write to the control file could invoke
dlm_ls_start() which calls wake_up_process(ls->ls_recoverd_task) on the freed
task_struct.

Also, module_put() is called at the end of release_lockspace() while the
kobject delayed release is pending, repeating the module unload race mentioned
above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013223946.1836757-1-irogers@google.com?part=6