Received-SPF: pass (google.com: domain of linux-kernel+bounces-36760-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240123-b4-hid-bpf-fixes-v1-0-aa1fac734377@kernel.org>
 <20240123-b4-hid-bpf-fixes-v1-2-aa1fac734377@kernel.org> <CAEf4BzbE-YHR1FuXfG7ryi_77H=nzF0XBoppqrbG4Uh1uPz8Lg@mail.gmail.com>
In-Reply-To: <CAEf4BzbE-YHR1FuXfG7ryi_77H=nzF0XBoppqrbG4Uh1uPz8Lg@mail.gmail.com>
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date: Wed, 24 Jan 2024 10:58:53 +0100
Message-ID: <CAO-hwJL4UMQMOC-CQKXZ_Np2b7H2rSnAji=Yypc62k_+GejwcA@mail.gmail.com>
Subject: Re: [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Benjamin Tissoires <bentiss@kernel.org>, Jiri Kosina <jikos@kernel.org>, 
	Dan Carpenter <dan.carpenter@linaro.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, 
	bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 23, 2024 at 8:57=E2=80=AFPM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 23, 2024 at 8:46=E2=80=AFAM Benjamin Tissoires <bentiss@kerne=
l.org> wrote:
> >
> > Follow the docs at Documentation/bpf/kfuncs.rst:
> > - declare the function with `__bpf_kfunc`
> > - disables missing prototype warnings, which allows to remove them from
> >   include/linux/hid-bpf.h
> >
> > Removing the prototypes is not an issue because we currently have to
> > redeclare them when writing the BPF program. They will eventually be
> > generated by bpftool directly AFAIU.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> >  drivers/hid/bpf/hid_bpf_dispatch.c | 22 +++++++++++++++++-----
> >  include/linux/hid_bpf.h            | 11 -----------
> >  2 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_b=
pf_dispatch.c
> > index 5111d1fef0d3..119e4f03df55 100644
> > --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> > @@ -143,6 +143,11 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hd=
ev, u8 *rdesc, unsigned int *s
> >  }
> >  EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
> >
> > +/* Disables missing prototype warnings */
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +                 "Global kfuncs as their definitions will be in BTF");
> > +
>
> would it be possible to use __bpf_kfunc_start_defs() and
> __bpf_kfunc_end_defs() macros instead of explicit __diag push/pop
> pairs? It's defined in include/linux/btf.h

Sure, I'll use them in v2.

I also realized that I had some memory leaks because I never called
put_device() after bus_find_device(), so I need to add another fix to
this series.

Cheers,
Benjamin

>
> >  /**
> >   * hid_bpf_get_data - Get the kernel memory pointer associated with th=
e context @ctx
> >   *
> > @@ -152,7 +157,7 @@ EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
> >   *
> >   * @returns %NULL on error, an %__u8 memory pointer on success
> >   */
> > -noinline __u8 *
> > +__bpf_kfunc __u8 *
> >  hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const s=
ize_t rdwr_buf_size)
> >  {
> >         struct hid_bpf_ctx_kern *ctx_kern;
> > @@ -167,6 +172,7 @@ hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned =
int offset, const size_t rdwr
> >
> >         return ctx_kern->data + offset;
> >  }
> > +__diag_pop(); /* missing prototype warnings */
> >
> >  /*
> >   * The following set contains all functions we agree BPF programs
> > @@ -274,6 +280,11 @@ static int do_hid_bpf_attach_prog(struct hid_devic=
e *hdev, int prog_fd, struct b
> >         return fd;
> >  }
> >
> > +/* Disables missing prototype warnings */
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +                 "Global kfuncs as their definitions will be in BTF");
> > +
> >  /**
> >   * hid_bpf_attach_prog - Attach the given @prog_fd to the given HID de=
vice
> >   *
> > @@ -286,7 +297,7 @@ static int do_hid_bpf_attach_prog(struct hid_device=
 *hdev, int prog_fd, struct b
> >   * is pinned to the BPF file system).
> >   */
> >  /* called from syscall */
> > -noinline int
> > +__bpf_kfunc int
> >  hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
> >  {
> >         struct hid_device *hdev;
> > @@ -328,7 +339,7 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_f=
d, __u32 flags)
> >   *
> >   * @returns A pointer to &struct hid_bpf_ctx on success, %NULL on erro=
r.
> >   */
> > -noinline struct hid_bpf_ctx *
> > +__bpf_kfunc struct hid_bpf_ctx *
> >  hid_bpf_allocate_context(unsigned int hid_id)
> >  {
> >         struct hid_device *hdev;
> > @@ -359,7 +370,7 @@ hid_bpf_allocate_context(unsigned int hid_id)
> >   * @ctx: the HID-BPF context to release
> >   *
> >   */
> > -noinline void
> > +__bpf_kfunc void
> >  hid_bpf_release_context(struct hid_bpf_ctx *ctx)
> >  {
> >         struct hid_bpf_ctx_kern *ctx_kern;
> > @@ -380,7 +391,7 @@ hid_bpf_release_context(struct hid_bpf_ctx *ctx)
> >   *
> >   * @returns %0 on success, a negative error code otherwise.
> >   */
> > -noinline int
> > +__bpf_kfunc int
> >  hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
> >                    enum hid_report_type rtype, enum hid_class_request r=
eqtype)
> >  {
> > @@ -448,6 +459,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *b=
uf, size_t buf__sz,
> >         kfree(dma_data);
> >         return ret;
> >  }
> > +__diag_pop(); /* missing prototype warnings */
> >
> >  /* our HID-BPF entrypoints */
> >  BTF_SET8_START(hid_bpf_fmodret_ids)
> > diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
> > index 840cd254172d..7118ac28d468 100644
> > --- a/include/linux/hid_bpf.h
> > +++ b/include/linux/hid_bpf.h
> > @@ -77,17 +77,6 @@ enum hid_bpf_attach_flags {
> >  int hid_bpf_device_event(struct hid_bpf_ctx *ctx);
> >  int hid_bpf_rdesc_fixup(struct hid_bpf_ctx *ctx);
> >
> > -/* Following functions are kfunc that we export to BPF programs */
> > -/* available everywhere in HID-BPF */
> > -__u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, c=
onst size_t __sz);
> > -
> > -/* only available in syscall */
> > -int hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)=
;
> > -int hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf_=
_sz,
> > -                      enum hid_report_type rtype, enum hid_class_reque=
st reqtype);
> > -struct hid_bpf_ctx *hid_bpf_allocate_context(unsigned int hid_id);
> > -void hid_bpf_release_context(struct hid_bpf_ctx *ctx);
> > -
> >  /*
> >   * Below is HID internal
> >   */
> >
> > --
> > 2.43.0
> >
> >
>