On Wed, Jan 24, 2024 at 12:27 PM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
>
> Turns out that I got my reference counts wrong and each successful
> bus_find_device() actually calls get_device(), and we need to manually
> call put_device().
>
> Ensure each bus_find_device() gets a matching put_device() when releasing
> the bpf programs and fix all the error paths.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
> Signed-off-by: Benjamin Tissoires <bentiss@xxxxxxxxxx>
>
> ---
>
> new in v2
> ---
> drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++------
> drivers/hid/bpf/hid_bpf_jmp_table.c | 19 ++++++++++++++++---
> 2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 5111d1fef0d3..7903c8638e81 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -292,7 +292,7 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
> struct hid_device *hdev;
> struct bpf_prog *prog;
> struct device *dev;
> - int fd;
> + int err, fd;
>
> if (!hid_bpf_ops)
> return -EINVAL;
> @@ -311,14 +311,24 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
> * on errors or when it'll be detached
> */
> prog = bpf_prog_get(prog_fd);
> - if (IS_ERR(prog))
> - return PTR_ERR(prog);
> + if (IS_ERR(prog)) {
> + err = PTR_ERR(prog);
> + goto out_dev_put;
> + }
>
> fd = do_hid_bpf_attach_prog(hdev, prog_fd, prog, flags);
> - if (fd < 0)
> - bpf_prog_put(prog);
> + if (fd < 0) {
> + err = fd;
> + goto out_prog_put;
> + }
>
> return fd;
> +
> + out_prog_put:
> + bpf_prog_put(prog);
> + out_dev_put:
> + put_device(dev);
> + return err;
> }
>
> /**
> @@ -345,8 +355,10 @@ hid_bpf_allocate_context(unsigned int hid_id)
> hdev = to_hid_device(dev);
>
> ctx_kern = kzalloc(sizeof(*ctx_kern), GFP_KERNEL);
> - if (!ctx_kern)
> + if (!ctx_kern) {
> + put_device(dev);
> return NULL;
> + }
>
> ctx_kern->ctx.hid = hdev;
>
> @@ -363,10 +375,15 @@ noinline void
> hid_bpf_release_context(struct hid_bpf_ctx *ctx)
> {
> struct hid_bpf_ctx_kern *ctx_kern;
> + struct hid_device *hid;
>
> ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
> + hid = (struct hid_device *)ctx_kern->ctx.hid; /* ignore const */
>
> kfree(ctx_kern);
> +
> + /* get_device() is called by bus_find_device() */
> + put_device(&hid->dev);
> }
>
> /**
> diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
> index 12f7cebddd73..85a24bc0ea25 100644
> --- a/drivers/hid/bpf/hid_bpf_jmp_table.c
> +++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
> @@ -196,6 +196,7 @@ static void __hid_bpf_do_release_prog(int map_fd, unsigned int idx)
> static void hid_bpf_release_progs(struct work_struct *work)
> {
> int i, j, n, map_fd = -1;
> + bool hdev_destroyed;
>
> if (!jmp_table.map)
> return;
> @@ -220,6 +221,12 @@ static void hid_bpf_release_progs(struct work_struct *work)
> if (entry->hdev) {
> hdev = entry->hdev;
> type = entry->type;
> + /*
> + * hdev is still valid, even if we are called after hid_destroy_device():
> + * when hid_bpf_attach() gets called, it takes a ref on the dev through
> + * bus_find_device()
> + */
> + hdev_destroyed = hdev->bpf.destroyed;
>
> hid_bpf_populate_hdev(hdev, type);
>
> @@ -232,12 +239,18 @@ static void hid_bpf_release_progs(struct work_struct *work)
> if (test_bit(next->idx, jmp_table.enabled))
> continue;
>
> - if (next->hdev == hdev && next->type == type)
> + if (next->hdev == hdev && next->type == type) {
> + /*
> + * clear the hdev reference and decrement the device ref
> + * that was taken during bus_find_device() while calling
> + * hid_bpf_attach()
> + */
> next->hdev = NULL;
> + put_device(&hdev->dev);
sigh... I can't make a correct patch these days... Missing a '}' here
to match the open bracket added above :(
I had some debug information put there to check if the device was
actually freed, and the closing bracket got lost while cleaning this
up.
Cheers,
Benjamin
> }
>
> - /* if type was rdesc fixup, reconnect device */
> - if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP)
> + /* if type was rdesc fixup and the device is not gone, reconnect device */
> + if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP && !hdev_destroyed)
> hid_bpf_reconnect(hdev);
> }
> }
>
> --
> 2.43.0
>
