Received-SPF: pass (google.com: domain of linux-kernel+bounces-40034-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240124-b4-hid-bpf-fixes-v2-0-052520b1e5e6@kernel.org> <20240124-b4-hid-bpf-fixes-v2-2-052520b1e5e6@kernel.org>
In-Reply-To: <20240124-b4-hid-bpf-fixes-v2-2-052520b1e5e6@kernel.org>
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date: Fri, 26 Jan 2024 12:20:23 +0100
Message-ID: <CAO-hwJ+xOF=GH115_KcWKjXLqeKU-BzXmNY0bvOzhZQb0WkEDg@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] HID: bpf: actually free hdev memory after
 attaching a HID-BPF program
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>, Dan Carpenter <dan.carpenter@linaro.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii.nakryiko@gmail.com>, 
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, 
	bpf@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 24, 2024 at 12:27=E2=80=AFPM Benjamin Tissoires <bentiss@kernel=
org> 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@vger.kernel.org
> Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> 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_f=
d, __u32 flags)
>          * on errors or when it'll be detached
>          */
>         prog =3D bpf_prog_get(prog_fd);
> -       if (IS_ERR(prog))
> -               return PTR_ERR(prog);
> +       if (IS_ERR(prog)) {
> +               err =3D PTR_ERR(prog);
> +               goto out_dev_put;
> +       }
>
>         fd =3D do_hid_bpf_attach_prog(hdev, prog_fd, prog, flags);
> -       if (fd < 0)
> -               bpf_prog_put(prog);
> +       if (fd < 0) {
> +               err =3D 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 =3D to_hid_device(dev);
>
>         ctx_kern =3D kzalloc(sizeof(*ctx_kern), GFP_KERNEL);
> -       if (!ctx_kern)
> +       if (!ctx_kern) {
> +               put_device(dev);
>                 return NULL;
> +       }
>
>         ctx_kern->ctx.hid =3D 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 =3D container_of(ctx, struct hid_bpf_ctx_kern, ctx);
> +       hid =3D (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_bp=
f_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, uns=
igned int idx)
>  static void hid_bpf_release_progs(struct work_struct *work)
>  {
>         int i, j, n, map_fd =3D -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 =3D entry->hdev;
>                         type =3D entry->type;
> +                       /*
> +                        * hdev is still valid, even if we are called aft=
er hid_destroy_device():
> +                        * when hid_bpf_attach() gets called, it takes a =
ref on the dev through
> +                        * bus_find_device()
> +                        */
> +                       hdev_destroyed =3D hdev->bpf.destroyed;
>
>                         hid_bpf_populate_hdev(hdev, type);
>
> @@ -232,12 +239,18 @@ static void hid_bpf_release_progs(struct work_struc=
t *work)
>                                 if (test_bit(next->idx, jmp_table.enabled=
))
>                                         continue;
>
> -                               if (next->hdev =3D=3D hdev && next->type =
=3D=3D type)
> +                               if (next->hdev =3D=3D hdev && next->type =
=3D=3D type) {
> +                                       /*
> +                                        * clear the hdev reference and d=
ecrement the device ref
> +                                        * that was taken during bus_find=
_device() while calling
> +                                        * hid_bpf_attach()
> +                                        */
>                                         next->hdev =3D 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 =3D=3D HID_BPF_PROG_TYPE_RDESC_FIXUP)
> +                       /* if type was rdesc fixup and the device is not =
gone, reconnect device */
> +                       if (type =3D=3D HID_BPF_PROG_TYPE_RDESC_FIXUP && =
!hdev_destroyed)
>                                 hid_bpf_reconnect(hdev);
>                 }
>         }
>
> --
> 2.43.0
>