* [PATCH 0/2] HID: bpf: couple of upstream fixes
@ 2024-01-23 16:40 Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 1/2] HID: bpf: remove double fdget() Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline Benjamin Tissoires
0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2024-01-23 16:40 UTC (permalink / raw
To: Jiri Kosina, Benjamin Tissoires, Dan Carpenter, Daniel Borkmann
Cc: linux-input, linux-kernel, bpf, Benjamin Tissoires, stable
Hi,
these are a couple of fixes for hid-bpf. The first one should
probably go in ASAP, after the reviews, and the second one is nice
to have and doesn't hurt much.
Thanks Dan for finding out the issue with bpf_prog_get()
Cheers,
Benjamin
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (2):
HID: bpf: remove double fdget()
HID: bpf: use __bpf_kfunc instead of noinline
drivers/hid/bpf/hid_bpf_dispatch.c | 88 +++++++++++++++++++++++++------------
drivers/hid/bpf/hid_bpf_dispatch.h | 4 +-
drivers/hid/bpf/hid_bpf_jmp_table.c | 20 ++-------
include/linux/hid_bpf.h | 11 -----
4 files changed, 66 insertions(+), 57 deletions(-)
---
base-commit: fef018d8199661962b5fc0f0d1501caa54b2b533
change-id: 20240123-b4-hid-bpf-fixes-662908fe2234
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] HID: bpf: remove double fdget()
2024-01-23 16:40 [PATCH 0/2] HID: bpf: couple of upstream fixes Benjamin Tissoires
@ 2024-01-23 16:40 ` Benjamin Tissoires
2024-01-23 17:19 ` Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline Benjamin Tissoires
1 sibling, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2024-01-23 16:40 UTC (permalink / raw
To: Jiri Kosina, Benjamin Tissoires, Dan Carpenter, Daniel Borkmann
Cc: linux-input, linux-kernel, bpf, Benjamin Tissoires, stable
When the kfunc hid_bpf_attach_prog() is called, we called twice fdget():
one for fetching the type of the bpf program, and one for actually
attaching the program to the device.
The problem is that between those two calls, we have no guarantees that
the prog_fd is still the same file descriptor for the given program.
Solve this by calling bpf_prog_get() earlier, and use this to fetch the
program type.
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Link: https://lore.kernel.org/bpf/CAO-hwJJ8vh8JD3-P43L-_CLNmPx0hWj44aom0O838vfP4=_1CA@mail.gmail.com/T/#t
Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
drivers/hid/bpf/hid_bpf_dispatch.c | 66 ++++++++++++++++++++++++-------------
drivers/hid/bpf/hid_bpf_dispatch.h | 4 +--
drivers/hid/bpf/hid_bpf_jmp_table.c | 20 ++---------
3 files changed, 49 insertions(+), 41 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index d9ef45fcaeab..5111d1fef0d3 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -241,6 +241,39 @@ int hid_bpf_reconnect(struct hid_device *hdev)
return 0;
}
+static int do_hid_bpf_attach_prog(struct hid_device *hdev, int prog_fd, struct bpf_prog *prog,
+ __u32 flags)
+{
+ int fd, err, prog_type;
+
+ prog_type = hid_bpf_get_prog_attach_type(prog);
+ if (prog_type < 0)
+ return prog_type;
+
+ if (prog_type >= HID_BPF_PROG_TYPE_MAX)
+ return -EINVAL;
+
+ if (prog_type == HID_BPF_PROG_TYPE_DEVICE_EVENT) {
+ err = hid_bpf_allocate_event_data(hdev);
+ if (err)
+ return err;
+ }
+
+ fd = __hid_bpf_attach_prog(hdev, prog_type, prog_fd, prog, flags);
+ if (fd < 0)
+ return fd;
+
+ if (prog_type == HID_BPF_PROG_TYPE_RDESC_FIXUP) {
+ err = hid_bpf_reconnect(hdev);
+ if (err) {
+ close_fd(fd);
+ return err;
+ }
+ }
+
+ return fd;
+}
+
/**
* hid_bpf_attach_prog - Attach the given @prog_fd to the given HID device
*
@@ -257,18 +290,13 @@ noinline int
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, err, prog_type = hid_bpf_get_prog_attach_type(prog_fd);
+ int fd;
if (!hid_bpf_ops)
return -EINVAL;
- if (prog_type < 0)
- return prog_type;
-
- if (prog_type >= HID_BPF_PROG_TYPE_MAX)
- return -EINVAL;
-
if ((flags & ~HID_BPF_FLAG_MASK))
return -EINVAL;
@@ -278,23 +306,17 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
hdev = to_hid_device(dev);
- if (prog_type == HID_BPF_PROG_TYPE_DEVICE_EVENT) {
- err = hid_bpf_allocate_event_data(hdev);
- if (err)
- return err;
- }
+ /*
+ * take a ref on the prog itself, it will be released
+ * on errors or when it'll be detached
+ */
+ prog = bpf_prog_get(prog_fd);
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
- fd = __hid_bpf_attach_prog(hdev, prog_type, prog_fd, flags);
+ fd = do_hid_bpf_attach_prog(hdev, prog_fd, prog, flags);
if (fd < 0)
- return fd;
-
- if (prog_type == HID_BPF_PROG_TYPE_RDESC_FIXUP) {
- err = hid_bpf_reconnect(hdev);
- if (err) {
- close_fd(fd);
- return err;
- }
- }
+ bpf_prog_put(prog);
return fd;
}
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
index 63dfc8605cd2..fbe0639d09f2 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.h
+++ b/drivers/hid/bpf/hid_bpf_dispatch.h
@@ -12,9 +12,9 @@ struct hid_bpf_ctx_kern {
int hid_bpf_preload_skel(void);
void hid_bpf_free_links_and_skel(void);
-int hid_bpf_get_prog_attach_type(int prog_fd);
+int hid_bpf_get_prog_attach_type(struct bpf_prog *prog);
int __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type, int prog_fd,
- __u32 flags);
+ struct bpf_prog *prog, __u32 flags);
void __hid_bpf_destroy_device(struct hid_device *hdev);
int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
struct hid_bpf_ctx_kern *ctx_kern);
diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
index eca34b7372f9..12f7cebddd73 100644
--- a/drivers/hid/bpf/hid_bpf_jmp_table.c
+++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
@@ -333,15 +333,10 @@ static int hid_bpf_insert_prog(int prog_fd, struct bpf_prog *prog)
return err;
}
-int hid_bpf_get_prog_attach_type(int prog_fd)
+int hid_bpf_get_prog_attach_type(struct bpf_prog *prog)
{
- struct bpf_prog *prog = NULL;
- int i;
int prog_type = HID_BPF_PROG_TYPE_UNDEF;
-
- prog = bpf_prog_get(prog_fd);
- if (IS_ERR(prog))
- return PTR_ERR(prog);
+ int i;
for (i = 0; i < HID_BPF_PROG_TYPE_MAX; i++) {
if (hid_bpf_btf_ids[i] == prog->aux->attach_btf_id) {
@@ -350,8 +345,6 @@ int hid_bpf_get_prog_attach_type(int prog_fd)
}
}
- bpf_prog_put(prog);
-
return prog_type;
}
@@ -388,19 +381,13 @@ static const struct bpf_link_ops hid_bpf_link_lops = {
/* called from syscall */
noinline int
__hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
- int prog_fd, __u32 flags)
+ int prog_fd, struct bpf_prog *prog, __u32 flags)
{
struct bpf_link_primer link_primer;
struct hid_bpf_link *link;
- struct bpf_prog *prog = NULL;
struct hid_bpf_prog_entry *prog_entry;
int cnt, err = -EINVAL, prog_table_idx = -1;
- /* take a ref on the prog itself */
- prog = bpf_prog_get(prog_fd);
- if (IS_ERR(prog))
- return PTR_ERR(prog);
-
mutex_lock(&hid_bpf_attach_lock);
link = kzalloc(sizeof(*link), GFP_USER);
@@ -467,7 +454,6 @@ __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
err_unlock:
mutex_unlock(&hid_bpf_attach_lock);
- bpf_prog_put(prog);
kfree(link);
return err;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline
2024-01-23 16:40 [PATCH 0/2] HID: bpf: couple of upstream fixes Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 1/2] HID: bpf: remove double fdget() Benjamin Tissoires
@ 2024-01-23 16:40 ` Benjamin Tissoires
2024-01-23 19:57 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2024-01-23 16:40 UTC (permalink / raw
To: Jiri Kosina, Benjamin Tissoires, Dan Carpenter, Daniel Borkmann
Cc: linux-input, linux-kernel, bpf, Benjamin Tissoires
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_bpf_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 *hdev, 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");
+
/**
* hid_bpf_get_data - Get the kernel memory pointer associated with the 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 size_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_device *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 device
*
@@ -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_fd, __u32 flags)
*
* @returns A pointer to &struct hid_bpf_ctx on success, %NULL on error.
*/
-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 reqtype)
{
@@ -448,6 +459,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, 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, const 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_request 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] HID: bpf: remove double fdget()
2024-01-23 16:40 ` [PATCH 1/2] HID: bpf: remove double fdget() Benjamin Tissoires
@ 2024-01-23 17:19 ` Benjamin Tissoires
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2024-01-23 17:19 UTC (permalink / raw
To: Benjamin Tissoires
Cc: Jiri Kosina, Dan Carpenter, Daniel Borkmann, linux-input,
linux-kernel, bpf, stable
On Tue, Jan 23, 2024 at 5:41 PM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> When the kfunc hid_bpf_attach_prog() is called, we called twice fdget():
> one for fetching the type of the bpf program, and one for actually
> attaching the program to the device.
>
> The problem is that between those two calls, we have no guarantees that
> the prog_fd is still the same file descriptor for the given program.
>
> Solve this by calling bpf_prog_get() earlier, and use this to fetch the
> program type.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Link: https://lore.kernel.org/bpf/CAO-hwJJ8vh8JD3-P43L-_CLNmPx0hWj44aom0O838vfP4=_1CA@mail.gmail.com/T/#t
> Cc: stable@vger.kernel.org
Sigh, I forgot:
Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
Cheers,
Benjamin
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> drivers/hid/bpf/hid_bpf_dispatch.c | 66 ++++++++++++++++++++++++-------------
> drivers/hid/bpf/hid_bpf_dispatch.h | 4 +--
> drivers/hid/bpf/hid_bpf_jmp_table.c | 20 ++---------
> 3 files changed, 49 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index d9ef45fcaeab..5111d1fef0d3 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -241,6 +241,39 @@ int hid_bpf_reconnect(struct hid_device *hdev)
> return 0;
> }
>
> +static int do_hid_bpf_attach_prog(struct hid_device *hdev, int prog_fd, struct bpf_prog *prog,
> + __u32 flags)
> +{
> + int fd, err, prog_type;
> +
> + prog_type = hid_bpf_get_prog_attach_type(prog);
> + if (prog_type < 0)
> + return prog_type;
> +
> + if (prog_type >= HID_BPF_PROG_TYPE_MAX)
> + return -EINVAL;
> +
> + if (prog_type == HID_BPF_PROG_TYPE_DEVICE_EVENT) {
> + err = hid_bpf_allocate_event_data(hdev);
> + if (err)
> + return err;
> + }
> +
> + fd = __hid_bpf_attach_prog(hdev, prog_type, prog_fd, prog, flags);
> + if (fd < 0)
> + return fd;
> +
> + if (prog_type == HID_BPF_PROG_TYPE_RDESC_FIXUP) {
> + err = hid_bpf_reconnect(hdev);
> + if (err) {
> + close_fd(fd);
> + return err;
> + }
> + }
> +
> + return fd;
> +}
> +
> /**
> * hid_bpf_attach_prog - Attach the given @prog_fd to the given HID device
> *
> @@ -257,18 +290,13 @@ noinline int
> 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, err, prog_type = hid_bpf_get_prog_attach_type(prog_fd);
> + int fd;
>
> if (!hid_bpf_ops)
> return -EINVAL;
>
> - if (prog_type < 0)
> - return prog_type;
> -
> - if (prog_type >= HID_BPF_PROG_TYPE_MAX)
> - return -EINVAL;
> -
> if ((flags & ~HID_BPF_FLAG_MASK))
> return -EINVAL;
>
> @@ -278,23 +306,17 @@ hid_bpf_attach_prog(unsigned int hid_id, int prog_fd, __u32 flags)
>
> hdev = to_hid_device(dev);
>
> - if (prog_type == HID_BPF_PROG_TYPE_DEVICE_EVENT) {
> - err = hid_bpf_allocate_event_data(hdev);
> - if (err)
> - return err;
> - }
> + /*
> + * take a ref on the prog itself, it will be released
> + * on errors or when it'll be detached
> + */
> + prog = bpf_prog_get(prog_fd);
> + if (IS_ERR(prog))
> + return PTR_ERR(prog);
>
> - fd = __hid_bpf_attach_prog(hdev, prog_type, prog_fd, flags);
> + fd = do_hid_bpf_attach_prog(hdev, prog_fd, prog, flags);
> if (fd < 0)
> - return fd;
> -
> - if (prog_type == HID_BPF_PROG_TYPE_RDESC_FIXUP) {
> - err = hid_bpf_reconnect(hdev);
> - if (err) {
> - close_fd(fd);
> - return err;
> - }
> - }
> + bpf_prog_put(prog);
>
> return fd;
> }
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
> index 63dfc8605cd2..fbe0639d09f2 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.h
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.h
> @@ -12,9 +12,9 @@ struct hid_bpf_ctx_kern {
>
> int hid_bpf_preload_skel(void);
> void hid_bpf_free_links_and_skel(void);
> -int hid_bpf_get_prog_attach_type(int prog_fd);
> +int hid_bpf_get_prog_attach_type(struct bpf_prog *prog);
> int __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type, int prog_fd,
> - __u32 flags);
> + struct bpf_prog *prog, __u32 flags);
> void __hid_bpf_destroy_device(struct hid_device *hdev);
> int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
> struct hid_bpf_ctx_kern *ctx_kern);
> diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
> index eca34b7372f9..12f7cebddd73 100644
> --- a/drivers/hid/bpf/hid_bpf_jmp_table.c
> +++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
> @@ -333,15 +333,10 @@ static int hid_bpf_insert_prog(int prog_fd, struct bpf_prog *prog)
> return err;
> }
>
> -int hid_bpf_get_prog_attach_type(int prog_fd)
> +int hid_bpf_get_prog_attach_type(struct bpf_prog *prog)
> {
> - struct bpf_prog *prog = NULL;
> - int i;
> int prog_type = HID_BPF_PROG_TYPE_UNDEF;
> -
> - prog = bpf_prog_get(prog_fd);
> - if (IS_ERR(prog))
> - return PTR_ERR(prog);
> + int i;
>
> for (i = 0; i < HID_BPF_PROG_TYPE_MAX; i++) {
> if (hid_bpf_btf_ids[i] == prog->aux->attach_btf_id) {
> @@ -350,8 +345,6 @@ int hid_bpf_get_prog_attach_type(int prog_fd)
> }
> }
>
> - bpf_prog_put(prog);
> -
> return prog_type;
> }
>
> @@ -388,19 +381,13 @@ static const struct bpf_link_ops hid_bpf_link_lops = {
> /* called from syscall */
> noinline int
> __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
> - int prog_fd, __u32 flags)
> + int prog_fd, struct bpf_prog *prog, __u32 flags)
> {
> struct bpf_link_primer link_primer;
> struct hid_bpf_link *link;
> - struct bpf_prog *prog = NULL;
> struct hid_bpf_prog_entry *prog_entry;
> int cnt, err = -EINVAL, prog_table_idx = -1;
>
> - /* take a ref on the prog itself */
> - prog = bpf_prog_get(prog_fd);
> - if (IS_ERR(prog))
> - return PTR_ERR(prog);
> -
> mutex_lock(&hid_bpf_attach_lock);
>
> link = kzalloc(sizeof(*link), GFP_USER);
> @@ -467,7 +454,6 @@ __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
> err_unlock:
> mutex_unlock(&hid_bpf_attach_lock);
>
> - bpf_prog_put(prog);
> kfree(link);
>
> return err;
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline
2024-01-23 16:40 ` [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline Benjamin Tissoires
@ 2024-01-23 19:57 ` Andrii Nakryiko
2024-01-24 9:58 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2024-01-23 19:57 UTC (permalink / raw
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Dan Carpenter, Daniel Borkmann,
linux-input, linux-kernel, bpf
On Tue, Jan 23, 2024 at 8:46 AM Benjamin Tissoires <bentiss@kernel.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_bpf_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 *hdev, 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
> /**
> * hid_bpf_get_data - Get the kernel memory pointer associated with the 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 size_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_device *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 device
> *
> @@ -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_fd, __u32 flags)
> *
> * @returns A pointer to &struct hid_bpf_ctx on success, %NULL on error.
> */
> -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 reqtype)
> {
> @@ -448,6 +459,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, 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, const 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_request 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline
2024-01-23 19:57 ` Andrii Nakryiko
@ 2024-01-24 9:58 ` Benjamin Tissoires
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2024-01-24 9:58 UTC (permalink / raw
To: Andrii Nakryiko
Cc: Benjamin Tissoires, Jiri Kosina, Dan Carpenter, Daniel Borkmann,
linux-input, linux-kernel, bpf
On Tue, Jan 23, 2024 at 8:57 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 23, 2024 at 8:46 AM Benjamin Tissoires <bentiss@kernel.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_bpf_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 *hdev, 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 the 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 size_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_device *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 device
> > *
> > @@ -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_fd, __u32 flags)
> > *
> > * @returns A pointer to &struct hid_bpf_ctx on success, %NULL on error.
> > */
> > -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 reqtype)
> > {
> > @@ -448,6 +459,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, 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, const 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_request 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
> >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-24 9:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-23 16:40 [PATCH 0/2] HID: bpf: couple of upstream fixes Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 1/2] HID: bpf: remove double fdget() Benjamin Tissoires
2024-01-23 17:19 ` Benjamin Tissoires
2024-01-23 16:40 ` [PATCH 2/2] HID: bpf: use __bpf_kfunc instead of noinline Benjamin Tissoires
2024-01-23 19:57 ` Andrii Nakryiko
2024-01-24 9:58 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).