Re: [PATCH bpf-next 1/2] bpf: Add kfuncs for read-only string operations

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, Oct 1, 2024 at 11:12 PM Viktor Malik <vmalik@redhat.com> wrote:
>
> On 10/1/24 19:40, Andrii Nakryiko wrote:
> > On Tue, Oct 1, 2024 at 10:34 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >>
> >> On Tue, Oct 1, 2024 at 10:04 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Tue, Oct 1, 2024 at 7:48 AM Alexei Starovoitov
> >>> <alexei.starovoitov@gmail.com> wrote:
> >>>>
> >>>> On Tue, Oct 1, 2024 at 4:26 AM Eduard Zingerman <eddyz87@gmail.com> 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?

I would think __get_kernel_nofault() approach is user friendlier.