Hi,
This is a new version of the ALLOW_ERROR_INJECTION hack removal.
Compared to v2, I followed the review from Alexei which cleaned up the
code a little bit.
I also got a kbuild test bot complaining[3] so add a fix for that too.
For reference, here is the previous cover letter:
So this patch series aims at solving both [0] and [1].
The first one is bpf related and concerns the ALLOW_ERROR_INJECTION API.
It is considered as a hack to begin with, so introduce a proper kernel
API to declare when a BPF hook can have its return value changed.
The second one is related to the fact that
DYNAMIC_FTRACE_WITH_DIRECT_CALLS is currently not enabled on arm64, and
that means that the current HID-BPF implementation doesn't work there
for now.
The first patch actually touches the bpf core code, but it would be
easier if we could merge it through the hid tree in the for-6.2/hid-bpf
branch once we get the proper acks.
Cheers,
Benjamin
[0] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/r/CABRcYmKyRchQhabi1Vd9RcMQFCcb=EtWyEbFDFRTc-L-U8WhgA@mail.gmail.com
[3] https://lore.kernel.org/all/[email protected]/
Benjamin Tissoires (5):
bpf: do not rely on ALLOW_ERROR_INJECTION for fmod_ret
HID: bpf: do not rely on ALLOW_ERROR_INJECTION
HID: bpf: enforce HID_BPF dependencies
selftests: hid: ensures we have the proper requirements in config
kselftests: hid: fix missing headers_install step
drivers/hid/bpf/Kconfig | 3 ++-
drivers/hid/bpf/hid_bpf_dispatch.c | 20 +++++++++++++++++--
drivers/hid/bpf/hid_bpf_jmp_table.c | 1 -
include/linux/btf.h | 2 ++
kernel/bpf/btf.c | 30 +++++++++++++++++++++++-----
kernel/bpf/verifier.c | 17 ++++++++++++++--
net/bpf/test_run.c | 14 ++++++++++---
tools/testing/selftests/hid/Makefile | 26 +++++++++++-------------
tools/testing/selftests/hid/config | 1 +
9 files changed, 86 insertions(+), 28 deletions(-)
--
2.38.1
Now that we have aproper non debug API to declare which function is
fmodret, we can rely on it.
Link: https://lore.kernel.org/all/[email protected]/
Suggested-by: Alexei Starovoitov <[email protected]>
Signed-off-by: Benjamin Tissoires <[email protected]>
---
no changes in v3
changes in v2:
- new, split from https://lore.kernel.org/all/[email protected]/
and use the new API
---
drivers/hid/bpf/hid_bpf_dispatch.c | 20 ++++++++++++++++++--
drivers/hid/bpf/hid_bpf_jmp_table.c | 1 -
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index c3920c7964dc..58e608ebf0fa 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -44,7 +44,6 @@ __weak noinline int hid_bpf_device_event(struct hid_bpf_ctx *ctx)
{
return 0;
}
-ALLOW_ERROR_INJECTION(hid_bpf_device_event, ERRNO);
u8 *
dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data,
@@ -105,7 +104,6 @@ __weak noinline int hid_bpf_rdesc_fixup(struct hid_bpf_ctx *ctx)
{
return 0;
}
-ALLOW_ERROR_INJECTION(hid_bpf_rdesc_fixup, ERRNO);
u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size)
{
@@ -429,6 +427,18 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
return ret;
}
+/* our HID-BPF entrypoints */
+BTF_SET8_START(hid_bpf_fmodret_ids)
+BTF_ID_FLAGS(func, hid_bpf_device_event)
+BTF_ID_FLAGS(func, hid_bpf_rdesc_fixup)
+BTF_ID_FLAGS(func, __hid_bpf_tail_call)
+BTF_SET8_END(hid_bpf_fmodret_ids)
+
+static const struct btf_kfunc_id_set hid_bpf_fmodret_set = {
+ .owner = THIS_MODULE,
+ .set = &hid_bpf_fmodret_ids,
+};
+
/* for syscall HID-BPF */
BTF_SET8_START(hid_bpf_syscall_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_attach_prog)
@@ -495,6 +505,12 @@ static int __init hid_bpf_init(void)
* will not be available, so nobody will be able to use the functionality.
*/
+ err = register_btf_fmodret_id_set(&hid_bpf_fmodret_set);
+ if (err) {
+ pr_warn("error while registering fmodret entrypoints: %d", err);
+ return 0;
+ }
+
err = register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &hid_bpf_kfunc_set);
if (err) {
pr_warn("error while setting HID BPF tracing kfuncs: %d", err);
diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
index 579a6c06906e..207972b028d9 100644
--- a/drivers/hid/bpf/hid_bpf_jmp_table.c
+++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
@@ -103,7 +103,6 @@ __weak noinline int __hid_bpf_tail_call(struct hid_bpf_ctx *ctx)
{
return 0;
}
-ALLOW_ERROR_INJECTION(__hid_bpf_tail_call, ERRNO);
int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
struct hid_bpf_ctx_kern *ctx_kern)
--
2.38.1
The current way of expressing that a non-bpf kernel component is willing
to accept that bpf programs can be attached to it and that they can change
the return value is to abuse ALLOW_ERROR_INJECTION.
This is debated in the link below, and the result is that it is not a
reasonable thing to do.
Reuse the kfunc declaration structure to also tag the kernel functions
we want to be fmodret. This way we can control from any subsystem which
functions are being modified by bpf without touching the verifier.
Link: https://lore.kernel.org/all/[email protected]/
Suggested-by: Alexei Starovoitov <[email protected]>
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v3:
- renamed btf_kern_func_is_modify_return() into btf_kfunc_is_modify_return()
- removed copy/paste in register_btf_fmodret_id_set()
changes in v2:
- compared to https://lore.kernel.org/all/[email protected]/
- make use of register_btf_kfunc_id_set() so we don't have to include
hid-bpf.h in bpf core.
---
include/linux/btf.h | 2 ++
kernel/bpf/btf.c | 30 +++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 17 +++++++++++++++--
net/bpf/test_run.c | 14 +++++++++++---
4 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index f9aababc5d78..6a0808e7845c 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -412,8 +412,10 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
u32 *btf_kfunc_id_set_contains(const struct btf *btf,
enum bpf_prog_type prog_type,
u32 kfunc_btf_id);
+u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id);
int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
const struct btf_kfunc_id_set *s);
+int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset);
s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id);
int register_btf_id_dtor_kfuncs(const struct btf_id_dtor_kfunc *dtors, u32 add_cnt,
struct module *owner);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 35c07afac924..c1df506293e4 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -204,6 +204,7 @@ enum btf_kfunc_hook {
BTF_KFUNC_HOOK_STRUCT_OPS,
BTF_KFUNC_HOOK_TRACING,
BTF_KFUNC_HOOK_SYSCALL,
+ BTF_KFUNC_HOOK_FMODRET,
BTF_KFUNC_HOOK_MAX,
};
@@ -7448,11 +7449,14 @@ u32 *btf_kfunc_id_set_contains(const struct btf *btf,
return __btf_kfunc_id_set_contains(btf, hook, kfunc_btf_id);
}
-/* This function must be invoked only from initcalls/module init functions */
-int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
- const struct btf_kfunc_id_set *kset)
+u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id)
+{
+ return __btf_kfunc_id_set_contains(btf, BTF_KFUNC_HOOK_FMODRET, kfunc_btf_id);
+}
+
+static int __register_btf_kfunc_id_set(enum btf_kfunc_hook hook,
+ const struct btf_kfunc_id_set *kset)
{
- enum btf_kfunc_hook hook;
struct btf *btf;
int ret;
@@ -7471,13 +7475,29 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
if (IS_ERR(btf))
return PTR_ERR(btf);
- hook = bpf_prog_type_to_kfunc_hook(prog_type);
ret = btf_populate_kfunc_set(btf, hook, kset->set);
btf_put(btf);
return ret;
}
+
+/* This function must be invoked only from initcalls/module init functions */
+int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
+ const struct btf_kfunc_id_set *kset)
+{
+ enum btf_kfunc_hook hook;
+
+ hook = bpf_prog_type_to_kfunc_hook(prog_type);
+ return __register_btf_kfunc_id_set(hook, kset);
+}
EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set);
+/* This function must be invoked only from initcalls/module init functions */
+int register_btf_fmodret_id_set(const struct btf_kfunc_id_set *kset)
+{
+ return __register_btf_kfunc_id_set(BTF_KFUNC_HOOK_FMODRET, kset);
+}
+EXPORT_SYMBOL_GPL(register_btf_fmodret_id_set);
+
s32 btf_find_dtor_kfunc(struct btf *btf, u32 btf_id)
{
struct btf_id_dtor_kfunc_tab *tab = btf->dtor_kfunc_tab;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 225666307bba..ec2e6ec7309e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15021,12 +15021,22 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
ret = -EINVAL;
switch (prog->type) {
case BPF_PROG_TYPE_TRACING:
- /* fentry/fexit/fmod_ret progs can be sleepable only if they are
+
+ /* fentry/fexit/fmod_ret progs can be sleepable if they are
* attached to ALLOW_ERROR_INJECTION and are not in denylist.
*/
if (!check_non_sleepable_error_inject(btf_id) &&
within_error_injection_list(addr))
ret = 0;
+ /* fentry/fexit/fmod_ret progs can also be sleepable if they are
+ * in the fmodret id set with the KF_SLEEPABLE flag.
+ */
+ else {
+ u32 *flags = btf_kfunc_is_modify_return(btf, btf_id);
+
+ if (flags && (*flags & KF_SLEEPABLE))
+ ret = 0;
+ }
break;
case BPF_PROG_TYPE_LSM:
/* LSM progs check that they are attached to bpf_lsm_*() funcs.
@@ -15047,7 +15057,10 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
bpf_log(log, "can't modify return codes of BPF programs\n");
return -EINVAL;
}
- ret = check_attach_modify_return(addr, tname);
+ ret = -EINVAL;
+ if (btf_kfunc_is_modify_return(btf, btf_id) ||
+ !check_attach_modify_return(addr, tname))
+ ret = 0;
if (ret) {
bpf_log(log, "%s() is not modifiable\n", tname);
return ret;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 13d578ce2a09..5079fb089b5f 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -489,7 +489,6 @@ int noinline bpf_fentry_test1(int a)
return a + 1;
}
EXPORT_SYMBOL_GPL(bpf_fentry_test1);
-ALLOW_ERROR_INJECTION(bpf_fentry_test1, ERRNO);
int noinline bpf_fentry_test2(int a, u64 b)
{
@@ -733,7 +732,15 @@ noinline void bpf_kfunc_call_test_destructive(void)
__diag_pop();
-ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
+BTF_SET8_START(bpf_test_modify_return_ids)
+BTF_ID_FLAGS(func, bpf_modify_return_test)
+BTF_ID_FLAGS(func, bpf_fentry_test1, KF_SLEEPABLE)
+BTF_SET8_END(bpf_test_modify_return_ids)
+
+static const struct btf_kfunc_id_set bpf_test_modify_return_set = {
+ .owner = THIS_MODULE,
+ .set = &bpf_test_modify_return_ids,
+};
BTF_SET8_START(test_sk_check_kfunc_ids)
BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -1668,7 +1675,8 @@ static int __init bpf_prog_test_run_init(void)
};
int ret;
- ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set);
+ ret = register_btf_fmodret_id_set(&bpf_test_modify_return_set);
+ ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_prog_test_kfunc_set);
return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
--
2.38.1
The Makefile was assuming that headers_install was already done in
the top source directory, and was searching for installed uapi headers
there.
Unfortunately this is not the case and we need to manually call that step.
To do so, reorder the declaration of the variables, and reuses top_srcdir
provided by lib.mk
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/#t
Signed-off-by: Benjamin Tissoires <[email protected]>
---
new in v3
---
tools/testing/selftests/hid/Makefile | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 693f1cb6e47a..f6fc5cfff770 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -7,17 +7,9 @@ include ../../../scripts/Makefile.include
CXX ?= $(CROSS_COMPILE)g++
-CURDIR := $(abspath .)
-TOOLSDIR := $(abspath ../../..)
-TOP_SRCDIR = $(CURDIR)/../../../..
-KHDR_INCLUDES := $(TOP_SRCDIR)/usr/include
-LIBDIR := $(TOOLSDIR)/lib
-BPFDIR := $(LIBDIR)/bpf
-TOOLSINCDIR := $(TOOLSDIR)/include
-BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
HOSTPKG_CONFIG := pkg-config
-CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(KHDR_INCLUDES)
+CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(KHDR_INCLUDES) -I$(OUTPUT)
LDLIBS += -lelf -lz -lrt -lpthread
# Silence some warnings when compiled with clang
@@ -53,9 +45,15 @@ endef
include ../lib.mk
+TOOLSDIR := $(top_srcdir)/tools
+LIBDIR := $(TOOLSDIR)/lib
+BPFDIR := $(LIBDIR)/bpf
+TOOLSINCDIR := $(TOOLSDIR)/include
+BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
SCRATCH_DIR := $(OUTPUT)/tools
BUILD_DIR := $(SCRATCH_DIR)/build
INCLUDE_DIR := $(SCRATCH_DIR)/include
+KHDR_INCLUDES := $(SCRATCH_DIR)/uapi/include
BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
ifneq ($(CROSS_COMPILE),)
HOST_BUILD_DIR := $(BUILD_DIR)/host
@@ -122,7 +120,6 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install-bin
$(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
- $(KHDR_INCLUDES)/linux/bpf.h \
| $(BUILD_DIR)/libbpf
$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
EXTRA_CFLAGS='-g -O0' \
@@ -130,7 +127,6 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
$(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
- $(KHDR_INCLUDES)/linux/bpf.h \
| $(HOST_BUILD_DIR)/libbpf
$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) \
EXTRA_CFLAGS='-g -O0' ARCH= CROSS_COMPILE= \
@@ -147,6 +143,9 @@ else
$(Q)cp "$(VMLINUX_H)" $@
endif
+$(KHDR_INCLUDES)/linux/hid.h: $(top_srcdir)/include/uapi/linux/hid.h
+ $(MAKE) -C $(top_srcdir) INSTALL_HDR_PATH=$(SCRATCH_DIR)/uapi headers_install
+
$(RESOLVE_BTFIDS): $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/resolve_btfids \
$(TOOLSDIR)/bpf/resolve_btfids/main.c \
$(TOOLSDIR)/lib/rbtree.c \
@@ -178,8 +177,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \
- -I$(INCLUDE_DIR) -I$(CURDIR) -I$(KHDR_INCLUDES) \
- -I$(abspath $(OUTPUT)/../usr/include)
+ -I$(INCLUDE_DIR)
CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \
-Wno-compare-distinct-pointer-types
@@ -225,7 +223,7 @@ $(BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(OUTPUT)
$(Q)$(BPFTOOL) gen object $(<:.o=.linked1.o) $<
$(Q)$(BPFTOOL) gen skeleton $(<:.o=.linked1.o) name $(notdir $(<:.bpf.o=)) > $@
-$(OUTPUT)/%:%.c $(BPF_SKELS)
+$(OUTPUT)/%:%.c $(BPF_SKELS) $(KHDR_INCLUDES)/linux/hid.h
$(call msg,BINARY,,$@)
$(Q)$(LINK.c) $^ $(LDLIBS) -o $@
--
2.38.1
DYNAMIC_FTRACE_WITH_DIRECT_CALLS is implicit on x86_64 but is still a
WIP on aarm64. Ensure we get it selected to not have any surprises.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
no changes in v3
changes in v2:
- dropped ALLOW_ERROR_INJECTION requirement
---
tools/testing/selftests/hid/config | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/hid/config b/tools/testing/selftests/hid/config
index d4130489c1b1..9c5a55abca6b 100644
--- a/tools/testing/selftests/hid/config
+++ b/tools/testing/selftests/hid/config
@@ -11,6 +11,7 @@ CONFIG_BPF_SYSCALL=y
CONFIG_BPF=y
CONFIG_CGROUP_BPF=y
CONFIG_DEBUG_INFO_BTF=y
+CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS=y
CONFIG_FPROBE=y
CONFIG_FTRACE_SYSCALLS=y
CONFIG_FUNCTION_TRACER=y
--
2.38.1
As mentioned in the link below, having JIT and BPF is not enough to
have fentry/fexit/fmod_ret APIs. This resolves the error that
happens on a system without tracing enabled when hid-bpf tries to
load itself.
Link: https://lore.kernel.org/r/CABRcYmKyRchQhabi1Vd9RcMQFCcb=EtWyEbFDFRTc-L-U8WhgA@mail.gmail.com
Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
Signed-off-by: Benjamin Tissoires <[email protected]>
---
no changes in v3
changes in v2:
- dropped ALLOW_ERROR_INJECTION requirement
---
drivers/hid/bpf/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/bpf/Kconfig b/drivers/hid/bpf/Kconfig
index 298634fc3335..03f52145b83b 100644
--- a/drivers/hid/bpf/Kconfig
+++ b/drivers/hid/bpf/Kconfig
@@ -4,7 +4,8 @@ menu "HID-BPF support"
config HID_BPF
bool "HID-BPF support"
default HID_SUPPORT
- depends on BPF && BPF_SYSCALL
+ depends on BPF && BPF_SYSCALL && \
+ DYNAMIC_FTRACE_WITH_DIRECT_CALLS
help
This option allows to support eBPF programs on the HID subsystem.
eBPF programs can fix HID devices in a lighter way than a full
--
2.38.1
On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires
<[email protected]> wrote:
>
> The current way of expressing that a non-bpf kernel component is willing
> to accept that bpf programs can be attached to it and that they can change
> the return value is to abuse ALLOW_ERROR_INJECTION.
> This is debated in the link below, and the result is that it is not a
> reasonable thing to do.
>
> Reuse the kfunc declaration structure to also tag the kernel functions
> we want to be fmodret. This way we can control from any subsystem which
> functions are being modified by bpf without touching the verifier.
>
>
> Link: https://lore.kernel.org/all/[email protected]/
> Suggested-by: Alexei Starovoitov <[email protected]>
> Signed-off-by: Benjamin Tissoires <[email protected]>
BPF CI couldn't do its job because of a merge conflict.
CI only tries to apply the whole series.
But I tested the patch 1 manually.
Everything is green on x86-64 and the patch looks good.
Acked-by: Alexei Starovoitov <[email protected]>
Please send the set during the merge window.
If not we can take just this patch,
since the series from Viktor Malik would need this patch too.
On Tue, Dec 6, 2022 at 9:48 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > The current way of expressing that a non-bpf kernel component is willing
> > to accept that bpf programs can be attached to it and that they can change
> > the return value is to abuse ALLOW_ERROR_INJECTION.
> > This is debated in the link below, and the result is that it is not a
> > reasonable thing to do.
> >
> > Reuse the kfunc declaration structure to also tag the kernel functions
> > we want to be fmodret. This way we can control from any subsystem which
> > functions are being modified by bpf without touching the verifier.
> >
> >
> > Link: https://lore.kernel.org/all/[email protected]/
> > Suggested-by: Alexei Starovoitov <[email protected]>
> > Signed-off-by: Benjamin Tissoires <[email protected]>
>
> BPF CI couldn't do its job because of a merge conflict.
> CI only tries to apply the whole series.
> But I tested the patch 1 manually.
> Everything is green on x86-64 and the patch looks good.
>
> Acked-by: Alexei Starovoitov <[email protected]>
>
> Please send the set during the merge window.
> If not we can take just this patch,
> since the series from Viktor Malik would need this patch too.
>
Thanks a lot for the quick review/tests Alexei.
I have now taken this patch and the next into the hid tree.
I actually took this patch through a branch attached to our hid.git
master branch so compared to Linus, it only has this one patch. I also
tagged (and signed) that very same branch with "for-alexei-2022120701"
in case you also want to bring this one in through the bpf tree too.
Full path is at
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/tree/?h=for-alexei-2022120701
Cheers,
Benjamin
On Wed, Dec 7, 2022 at 3:58 PM Benjamin Tissoires
<[email protected]> wrote:
>
> On Tue, Dec 6, 2022 at 3:59 PM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > As mentioned in the link below, having JIT and BPF is not enough to
> > have fentry/fexit/fmod_ret APIs. This resolves the error that
> > happens on a system without tracing enabled when hid-bpf tries to
> > load itself.
> >
> > Link: https://lore.kernel.org/r/CABRcYmKyRchQhabi1Vd9RcMQFCcb=EtWyEbFDFRTc-L-U8WhgA@mail.gmail.com
> > Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
> > Signed-off-by: Benjamin Tissoires <[email protected]>
> >
> > ---
> >
> > no changes in v3
> >
> > changes in v2:
> > - dropped ALLOW_ERROR_INJECTION requirement
>
> Florent, can I keep your reviewed-by on this patch?
Yes! :)
Reviewed-by: Florent Revest <[email protected]>
Thank you for the updated series, I think it's cleaner than relying on
error injection indeed.
I still believe that, in the future, BPF should offer a proxy config
to expose if BPF tracing is supported because 1- the implementation of
BPF tracing could someday change 2- to be exactly correct, ftrace
direct call isn't _really_ a sufficient condition either: the BPF JIT
also needs to implement the arch_prepare_bpf_trampoline function.
Currently, there is no config to check if that feature is available.
But as agreed in a previous thread, that consolidation can be done
separately. For now your patch looks good enough to me already.
On Tue, Dec 6, 2022 at 3:59 PM Benjamin Tissoires
<[email protected]> wrote:
>
> As mentioned in the link below, having JIT and BPF is not enough to
> have fentry/fexit/fmod_ret APIs. This resolves the error that
> happens on a system without tracing enabled when hid-bpf tries to
> load itself.
>
> Link: https://lore.kernel.org/r/CABRcYmKyRchQhabi1Vd9RcMQFCcb=EtWyEbFDFRTc-L-U8WhgA@mail.gmail.com
> Fixes: f5c27da4e3c8 ("HID: initial BPF implementation")
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
> ---
>
> no changes in v3
>
> changes in v2:
> - dropped ALLOW_ERROR_INJECTION requirement
Florent, can I keep your reviewed-by on this patch?
Jon, may I ask you to do one more testing with the full v3 applied on
top of for-next?
Cheers,
Benjamin
> ---
> drivers/hid/bpf/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/bpf/Kconfig b/drivers/hid/bpf/Kconfig
> index 298634fc3335..03f52145b83b 100644
> --- a/drivers/hid/bpf/Kconfig
> +++ b/drivers/hid/bpf/Kconfig
> @@ -4,7 +4,8 @@ menu "HID-BPF support"
> config HID_BPF
> bool "HID-BPF support"
> default HID_SUPPORT
> - depends on BPF && BPF_SYSCALL
> + depends on BPF && BPF_SYSCALL && \
> + DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> help
> This option allows to support eBPF programs on the HID subsystem.
> eBPF programs can fix HID devices in a lighter way than a full
> --
> 2.38.1
>
Hello:
This series was applied to bpf/bpf-next.git (master)
by Benjamin Tissoires <[email protected]>:
On Tue, 6 Dec 2022 15:59:31 +0100 you wrote:
> Hi,
>
> This is a new version of the ALLOW_ERROR_INJECTION hack removal.
>
> Compared to v2, I followed the review from Alexei which cleaned up the
> code a little bit.
>
> [...]
Here is the summary with links:
- [HID,for-next,v3,1/5] bpf: do not rely on ALLOW_ERROR_INJECTION for fmod_ret
https://git.kernel.org/bpf/bpf-next/c/5b481acab4ce
- [HID,for-next,v3,2/5] HID: bpf: do not rely on ALLOW_ERROR_INJECTION
(no matching commit)
- [HID,for-next,v3,3/5] HID: bpf: enforce HID_BPF dependencies
(no matching commit)
- [HID,for-next,v3,4/5] selftests: hid: ensures we have the proper requirements in config
(no matching commit)
- [HID,for-next,v3,5/5] kselftests: hid: fix missing headers_install step
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Wed, Dec 7, 2022 at 6:57 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Tue, Dec 6, 2022 at 9:48 PM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Tue, Dec 6, 2022 at 6:59 AM Benjamin Tissoires
> > <[email protected]> wrote:
> > >
> > > The current way of expressing that a non-bpf kernel component is willing
> > > to accept that bpf programs can be attached to it and that they can change
> > > the return value is to abuse ALLOW_ERROR_INJECTION.
> > > This is debated in the link below, and the result is that it is not a
> > > reasonable thing to do.
> > >
> > > Reuse the kfunc declaration structure to also tag the kernel functions
> > > we want to be fmodret. This way we can control from any subsystem which
> > > functions are being modified by bpf without touching the verifier.
> > >
> > >
> > > Link: https://lore.kernel.org/all/[email protected]/
> > > Suggested-by: Alexei Starovoitov <[email protected]>
> > > Signed-off-by: Benjamin Tissoires <[email protected]>
> >
> > BPF CI couldn't do its job because of a merge conflict.
> > CI only tries to apply the whole series.
> > But I tested the patch 1 manually.
> > Everything is green on x86-64 and the patch looks good.
> >
> > Acked-by: Alexei Starovoitov <[email protected]>
> >
> > Please send the set during the merge window.
> > If not we can take just this patch,
> > since the series from Viktor Malik would need this patch too.
> >
>
> Thanks a lot for the quick review/tests Alexei.
>
> I have now taken this patch and the next into the hid tree.
>
> I actually took this patch through a branch attached to our hid.git
> master branch so compared to Linus, it only has this one patch. I also
> tagged (and signed) that very same branch with "for-alexei-2022120701"
> in case you also want to bring this one in through the bpf tree too.
I didn't find such a branch in your tree, but found that tag
and pulled into bpf-next.
Thanks!
On Tue, 6 Dec 2022, Benjamin Tissoires wrote:
> Compared to v2, I followed the review from Alexei which cleaned up the
> code a little bit.
>
> I also got a kbuild test bot complaining[3] so add a fix for that too.
>
> For reference, here is the previous cover letter:
>
> So this patch series aims at solving both [0] and [1].
>
> The first one is bpf related and concerns the ALLOW_ERROR_INJECTION API.
> It is considered as a hack to begin with, so introduce a proper kernel
> API to declare when a BPF hook can have its return value changed.
>
> The second one is related to the fact that
> DYNAMIC_FTRACE_WITH_DIRECT_CALLS is currently not enabled on arm64, and
> that means that the current HID-BPF implementation doesn't work there
> for now.
>
> The first patch actually touches the bpf core code, but it would be
> easier if we could merge it through the hid tree in the for-6.2/hid-bpf
> branch once we get the proper acks.
For the series:
Reviewed-by: Jiri Kosina <[email protected]>
Thanks,
--
Jiri Kosina
SUSE Labs
On Sat, Dec 10, 2022 at 10:07 AM Jiri Kosina <[email protected]> wrote:
>
> On Tue, 6 Dec 2022, Benjamin Tissoires wrote:
>
> > Compared to v2, I followed the review from Alexei which cleaned up the
> > code a little bit.
> >
> > I also got a kbuild test bot complaining[3] so add a fix for that too.
> >
> > For reference, here is the previous cover letter:
> >
> > So this patch series aims at solving both [0] and [1].
> >
> > The first one is bpf related and concerns the ALLOW_ERROR_INJECTION API.
> > It is considered as a hack to begin with, so introduce a proper kernel
> > API to declare when a BPF hook can have its return value changed.
> >
> > The second one is related to the fact that
> > DYNAMIC_FTRACE_WITH_DIRECT_CALLS is currently not enabled on arm64, and
> > that means that the current HID-BPF implementation doesn't work there
> > for now.
> >
> > The first patch actually touches the bpf core code, but it would be
> > easier if we could merge it through the hid tree in the for-6.2/hid-bpf
> > branch once we get the proper acks.
>
> For the series:
>
> Reviewed-by: Jiri Kosina <[email protected]>
Thanks. I have applied the rest of the series to for-6.2/hid-bpf.
Cheers,
Benjamin
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>