Re: [PATCH] HID: core: fix device cleanup on allocation failure

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

> hid_allocate_device() calls hid_destroy_device() in its error path when
> hid_bpf_device_init() fails.  hid_destroy_device() in turn calls
> hid_bpf_destroy_device() which invokes synchronize_srcu() and
> cleanup_srcu_struct() on the SRCU structure.  However, at this point
> init_srcu_struct() has not been called (or has failed), so the SRCU
> internal pointers are all zeroed from kzalloc.  Dereferencing them
> causes a general protection fault:

Is the reasoning here fully accurate?

Looking at hid_bpf_device_init() in drivers/hid/bpf/hid_bpf_dispatch.c,
it returns the result of init_srcu_struct(&hdev->bpf.srcu), so
init_srcu_struct() is always called before the error path is taken.

When init_srcu_struct() fails (specifically init_srcu_struct_fields() in
kernel/rcu/srcutree.c), the failure paths explicitly free and NULL the
pointers:

kernel/rcu/srcutree.c:init_srcu_struct_fields() {
    ...
err_free_sda:
    free_percpu(ssp->sda);
    ssp->sda = NULL;
err_free_sup:
    kfree(ssp->srcu_sup);
    ssp->srcu_sup = NULL;
    ...
}

So the SRCU internal pointers are NULL due to init_srcu_struct()'s
rollback on failure, not because init_srcu_struct() was never called or
because they are still zeroed from kzalloc.

Could the commit message be more precise? Something like:
"init_srcu_struct() was called but failed, and it rolled the SRCU
internal pointers back to NULL"

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index a3ff0514f9cd..d8df23f67f1f 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -3115,7 +3115,12 @@ struct hid_device *hid_allocate_device(void)
>  	return hdev;
>
>  out_err:
> -	hid_destroy_device(hdev);
> +	/*
> +	 * hid_destroy_device() cannot be used here because
> +	 * hid_bpf_device_init() failed and the SRCU struct was never
> +	 * initialized.  Release the device directly.
> +	 */
> +	put_device(&hdev->dev);
>  	return ERR_PTR(ret);
>  }

Should the comment say "was never successfully initialized" rather than
"was never initialized"?

As shown above, hid_bpf_device_init() returns init_srcu_struct()'s
result, so init_srcu_struct() is always called before reaching this
error path. It failed after partially initializing, then rolled back.

The distinction matters for future readers trying to understand the
ordering inside hid_bpf_device_init().


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33923149618