On 5/8/25 11:41, Matt Bobrowski wrote: > On Wed, May 07, 2025 at 08:40:38AM +0200, Viktor Malik wrote: >> String operations are commonly used so this exposes the most common ones >> to BPF programs. For now, we limit ourselves to operations which do not >> copy memory around. >> >> Unfortunately, most in-kernel implementations assume that strings are >> %NUL-terminated, which is not necessarily true, and therefore we cannot >> use them directly in the BPF context. Instead, we open-code them using >> __get_kernel_nofault instead of plain dereference to make them safe and >> limit the strings length to XATTR_SIZE_MAX to make sure the functions >> terminate. When __get_kernel_nofault fails, functions return -EFAULT. >> Similarly, when the size bound is reached, the functions return -E2BIG. > > Curious, why was XATTR_SIZE_MAX chosen as the upper bounds here? Just > an arbitrary value that felt large enough? Yes, exactly that. > >> At the moment, strings can be passed to the kfuncs in three forms: >> - string literals (i.e. pointers to read-only maps) >> - global variables (i.e. pointers to read-write maps) >> - stack-allocated buffers >> >> Note that currently, it is not possible to pass strings from the BPF >> program context (like function args) as the verifier doesn't treat them >> as neither PTR_TO_MEM nor PTR_TO_BTF_ID. >> >> Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx> >> Signed-off-by: Viktor Malik <vmalik@xxxxxxxxxx> >> --- >> kernel/bpf/helpers.c | 440 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 440 insertions(+) >> >> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c >> index e3a2662f4e33..8fb7c2ca7ac0 100644 >> --- a/kernel/bpf/helpers.c >> +++ b/kernel/bpf/helpers.c >> @@ -23,6 +23,7 @@ >> #include <linux/btf_ids.h> >> #include <linux/bpf_mem_alloc.h> >> #include <linux/kasan.h> >> +#include <linux/uaccess.h> >> >> #include "../../lib/kstrtox.h" >> >> @@ -3194,6 +3195,433 @@ __bpf_kfunc void bpf_local_irq_restore(unsigned long *flags__irq_flag) >> local_irq_restore(*flags__irq_flag); >> } >> >> +/* Kfuncs for string operations. >> + * >> + * Since strings are not necessarily %NUL-terminated, we cannot directly call >> + * in-kernel implementations. Instead, we open-code the implementations using >> + * __get_kernel_nofault instead of plain dereference to make them safe. >> + */ > > Returning an -EFAULT error code for supplied arguments which are > deemed to be invalid is just a very weird semantic IMO. As a BPF > program author, I totally wouldn't expect a BPF kfunc to return > -EINVAL if I had supplied it something that it couldn't quite possibly > handle to begin with. Look at the prior art, being pre-existing BPF > kfuncs, and you'll see how they handle invalidly supplied arguments. I > totally understand that attempting to dereference a NULL-pointer would > ultimately result in a fault being raised, so it may feel rather > natural to also report an -EFAULT error code upon doing some > NULL-pointer checks that hold true, but from an API usability POV it > just seems awkward and wrong. > > Another thing that I noticed was that none of these BPF kfunc > arguments make use of the parameter suffixes i.e. __str, __sz. Why is > that exactly? Will leaning on those break you in some way? The reason is that they both require literal values to be passed which is a limitation that we don't want in these APIs. Viktor
