Re: [PATCH v2 1/2] HID: bpf: synchronize hardware operations with device destruction

["Alexei Starovoitov" <alexei.starovoitov@gmail.com>]

On Mon, Nov 02, 2026 at 06:45 PM Yifan Zhang <yfzhang23@stu.pku.edu.cn> wrote:
> Add a lifetime lock that serializes admission of HID-BPF hardware
> operations with device destruction.  Take an SRCU read lock while holding
> the lifetime lock before accessing the report tables or the low-level
> driver.  Destruction marks the device destroyed under the same lock and
> waits for the SRCU readers before cleaning up the SRCU state.

The lifetime lock is needed only because hid_bpf_destroy_device() calls
cleanup_srcu_struct(). After that a reference to hid_device is not
enough to call srcu_read_lock().
The splat in patch 2 has the same cause. So does
commit 578e1b96fad7 ("HID: bpf: abort dispatch if device destroyed").

Lets keep hdev->bpf.srcu valid until hiddev_free().
Then the check of 'destroyed' goes inside the SRCU section.
No need for the 2nd mutex.

The dispatchers have the same race and this patch leaves it.
hid_bpf_destroy_device() runs before hidraw_disconnect():

  hidraw ioctl                        hid_destroy_device()
  dispatch_hid_bpf_raw_requests()
    destroyed == false
                                      destroyed = true
                                      synchronize_srcu()
                                      cleanup_srcu_struct()
    srcu_read_lock()   <- use after free

hid_close_report() also runs on unbind and on device_reprobe().
SRCU doesn't cover that. Neither this patch nor the diff below fixes it.

Instead of patches 1 and 2:

and Pls wait for Benjamin before sending v3.

diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index 6b467c7..6c2b136 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -99,13 +99,15 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev,
 	struct hid_bpf_ops *e;
 	int ret, idx;

-	if (unlikely(hdev->bpf.destroyed))
-		return -ENODEV;
-
 	if (rtype >= HID_REPORT_TYPES)
 		return -EINVAL;

 	idx = srcu_read_lock(&hdev->bpf.srcu);
+	if (unlikely(hdev->bpf.destroyed)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	list_for_each_entry_srcu(e, &hdev->bpf.prog_list, list,
 				 srcu_read_lock_held(&hdev->bpf.srcu)) {
 		if (!e->hid_hw_request)
@@ -139,10 +141,12 @@ int dispatch_hid_bpf_output_report(struct hid_device *hdev,
 	struct hid_bpf_ops *e;
 	int ret, idx;

-	if (unlikely(hdev->bpf.destroyed))
-		return -ENODEV;
-
 	idx = srcu_read_lock(&hdev->bpf.srcu);
+	if (unlikely(hdev->bpf.destroyed)) {
+		ret = -ENODEV;
+		goto out;
+	}
+
 	list_for_each_entry_srcu(e, &hdev->bpf.prog_list, list,
 				 srcu_read_lock_held(&hdev->bpf.srcu)) {
 		if (!e->hid_hw_output_report)
@@ -423,6 +427,11 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
 	if (ctx_kern->from_bpf)
 		return -EDEADLOCK;

+	/* hid_bpf_destroy_device() waits for us before the transport goes away */
+	guard(srcu)(&ctx->hid->bpf.srcu);
+	if (ctx->hid->bpf.destroyed)
+		return -ENODEV;
+
 	/* check arguments */
 	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
 	if (ret)
@@ -483,6 +492,10 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
 	if (ctx_kern->from_bpf)
 		return -EDEADLOCK;

+	guard(srcu)(&ctx->hid->bpf.srcu);
+	if (ctx->hid->bpf.destroyed)
+		return -ENODEV;
+
 	/* check arguments */
 	ret = __hid_bpf_hw_check_params(ctx, buf, &size, HID_OUTPUT_REPORT, true);
 	if (ret)
@@ -641,13 +654,9 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
 	if (!hdev)
 		return;

-	/* mark the device as destroyed in bpf so we don't reattach it */
-	hdev->bpf.destroyed = true;
-
 	__hid_bpf_ops_destroy_device(hdev);

 	synchronize_srcu(&hdev->bpf.srcu);
-	cleanup_srcu_struct(&hdev->bpf.srcu);
 }
 EXPORT_SYMBOL_GPL(hid_bpf_destroy_device);

@@ -659,6 +668,13 @@ int hid_bpf_device_init(struct hid_device *hdev)
 }
 EXPORT_SYMBOL_GPL(hid_bpf_device_init);

+/* A reference to hdev is enough to use hdev->bpf.srcu */
+void hid_bpf_device_free(struct hid_device *hdev)
+{
+	cleanup_srcu_struct(&hdev->bpf.srcu);
+}
+EXPORT_SYMBOL_GPL(hid_bpf_device_free);
+
 static int __init hid_bpf_init(void)
 {
 	int err;
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
index c90b689..f3bbe11 100644
--- a/drivers/hid/bpf/hid_bpf_struct_ops.c
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -197,10 +197,13 @@ static int hid_bpf_reg(void *kdata, struct bpf_link *link)
 	if (IS_ERR(hdev))
 		return PTR_ERR(hdev);

-	ops->hdev = hdev;
-
 	mutex_lock(&hdev->bpf.prog_list_lock);

+	if (hdev->bpf.destroyed) {
+		err = -ENODEV;
+		goto out_unlock;
+	}
+
 	count = list_count_nodes(&hdev->bpf.prog_list);
 	if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
 		err = -E2BIG;
@@ -222,6 +225,7 @@ static int hid_bpf_reg(void *kdata, struct bpf_link *link)
 			goto out_unlock;
 	}

+	ops->hdev = hdev;
 	if (ops->flags & BPF_F_BEFORE)
 		list_add_rcu(&ops->list, &hdev->bpf.prog_list);
 	else
@@ -324,6 +328,8 @@ void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
 	int count = 0;

 	mutex_lock(&hdev->bpf.prog_list_lock);
+	/* mark the device as destroyed in bpf so we don't reattach it */
+	hdev->bpf.destroyed = true;
 	list_for_each_entry(e, &hdev->bpf.prog_list, list) {
 		e->hdev = NULL;
 		count++;
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b4fd482..827a0ee 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -768,6 +768,7 @@ void hiddev_free(struct kref *ref)

 	hid_close_report(hid);
 	hid_free_bpf_rdesc(hid);
+	hid_bpf_device_free(hid);
 	kfree(hid->dev_rdesc);
 	kfree(hid);
 }
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 19fffa4..0db7f67 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -213,6 +213,7 @@ int hid_bpf_connect_device(struct hid_device *hdev);
 void hid_bpf_disconnect_device(struct hid_device *hdev);
 void hid_bpf_destroy_device(struct hid_device *hid);
 int hid_bpf_device_init(struct hid_device *hid);
+void hid_bpf_device_free(struct hid_device *hid);
 const u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, const u8 *rdesc, unsigned int *size);
 #else /* CONFIG_HID_BPF */
 static inline u8 *dispatch_hid_bpf_device_event(struct hid_device *hid, enum hid_report_type type,
@@ -232,6 +233,7 @@ static inline int hid_bpf_connect_device(struct hid_device *hdev) { return 0; }
 static inline void hid_bpf_disconnect_device(struct hid_device *hdev) {}
 static inline void hid_bpf_destroy_device(struct hid_device *hid) {}
 static inline int hid_bpf_device_init(struct hid_device *hid) { return 0; }
+static inline void hid_bpf_device_free(struct hid_device *hid) {}
 static inline const u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, const u8 *rdesc,
 						 unsigned int *size) { return rdesc; }


pw-bot: cr