On Wed, Oct 2, 2024 at 9:51 PM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > > On 10/2/24 18:55, Alexei Starovoitov wrote: > > On Tue, Oct 1, 2024 at 11:12 PM Viktor Malik <vmalik@xxxxxxxxxx> wrote: > >> > >> On 10/1/24 19:40, Andrii Nakryiko wrote: > >>> On Tue, Oct 1, 2024 at 10:34 AM Alexei Starovoitov > >>> <alexei.starovoitov@xxxxxxxxx> wrote: > >>>> > >>>> On Tue, Oct 1, 2024 at 10:04 AM Andrii Nakryiko > >>>> <andrii.nakryiko@xxxxxxxxx> wrote: > >>>>> > >>>>> On Tue, Oct 1, 2024 at 7:48 AM Alexei Starovoitov > >>>>> <alexei.starovoitov@xxxxxxxxx> wrote: > >>>>>> > >>>>>> On Tue, Oct 1, 2024 at 4:26 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > >>>>>>> > >>>>>>> On Mon, 2024-09-30 at 15:00 -0700, Andrii Nakryiko wrote: > >>>>>>> > >>>>>>> [...] > >>>>>>> > >>>>>>>> Right now, the only way to pass dynamically sized anything is through > >>>>>>>> dynptr, AFAIU. > >>>>>>> > >>>>>>> But we do have 'is_kfunc_arg_mem_size()' that checks for __sz suffix, > >>>>>>> e.g. used for bpf_copy_from_user_str(): > >>>>>>> > >>>>>>> /** > >>>>>>> * bpf_copy_from_user_str() - Copy a string from an unsafe user address > >>>>>>> * @dst: Destination address, in kernel space. This buffer must be > >>>>>>> * at least @dst__sz bytes long. > >>>>>>> * @dst__sz: Maximum number of bytes to copy, includes the trailing NUL. > >>>>>>> * ... > >>>>>>> */ > >>>>>>> __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user *unsafe_ptr__ign, u64 flags) > >>>>>>> > >>>>>>> However, this suffix won't work for strnstr because of the arguments order. > >>>>>> > >>>>>> Stating the obvious... we don't need to keep the order exactly the same. > >>>>>> > >>>>>> Regarding all of these kfuncs... as Andrii pointed out 'const char *s' > >>>>>> means that the verifier will check that 's' points to a valid byte. > >>>>>> I think we can do a hybrid static + dynamic safety scheme here. > >>>>>> All of the kfunc signatures can stay the same, but we'd have to > >>>>>> open code all string helpers with __get_kernel_nofault() instead of > >>>>>> direct memory access. > >>>>>> Since the first byte is guaranteed to be valid by the verifier > >>>>>> we only need to make sure that the s+N bytes won't cause page faults > >>>>> > >>>>> You mean to just check that s[N-1] can be read? Given a large enough > >>>>> N, couldn't it be that some page between s[0] and s[N-1] still can be > >>>>> unmapped, defeating this check? > >>>> > >>>> Just checking s[0] and s[N-1] is not enough, obviously, and especially, > >>>> since the logic won't know where nul byte is, so N is unknown. > >>>> I meant to that all of str* kfuncs will be reading all bytes > >>>> via __get_kernel_nofault() until they find \0. > >>> > >>> Ah, ok, I see what you mean now. > >>> > >>>> It can be optimized to 8 byte access. > >>>> The open coding (aka copy-paste) is unfortunate, of course. > >>> > >>> Yep, this sucks. > >> > >> Yeah, that's quite annoying. I really wanted to avoid doing that. Also, > >> we won't be able to use arch-optimized versions of the functions. > >> > >> Just to make sure I understand things correctly - can we do what Eduard > >> suggested and add explicit sizes for all arguments using the __sz > >> suffix? So something like: > >> > >> const char *bpf_strnstr(const char *s1, u32 s1__sz, const char *s2, u32 s2__sz); > > > > That's ok-ish, but you probably want: > > > > const char *bpf_strnstr(void *s1, u32 s1__sz, void *s2, u32 s2__sz); > > > > and then to call strnstr() you still need to strnlen(s2, s2__sz). > > > > But a more general question... how always passing size will work > > for bpftrace ? Does it always know the upper bound of storage where > > strings are stored? > > Yes, it does. The strings must be read via the str() call (which > internally calls bpf_probe_read_str) and there's an upper bound on the > size of each string. which sounds like a bpftrace current limitation and not something to depend on from kfunc design pov. Wouldn't you want strings to be arbitrary length? > > I would think __get_kernel_nofault() approach is user friendlier. > > That's probably true but isn't there still the problem that strings are > not necessarily null-terminated? And in such case, unbounded string > functions may not terminate which is not allowed in BPF? kfuncs that are searching for nul via loop with __get_kernel_nofault() would certainly need an upper bound. Something like PATH_MAX (4k) or XATTR_SIZE_MAX (64k) would cover 99.99% of use cases.
