Re: [PATCH v3 bpf-next 1/3] bpf: introduce new VFS based BPF kfuncs

[Matt Bobrowski <mattbobrowski@google.com>]

On Fri, Jul 26, 2024 at 03:48:45PM -0700, Song Liu wrote:
> On Fri, Jul 26, 2024 at 2:49 PM Matt Bobrowski <mattbobrowski@google.com> wrote:
> >
> > On Fri, Jul 26, 2024 at 02:25:26PM -0700, Song Liu wrote:
> > > On Fri, Jul 26, 2024 at 1:56 AM Matt Bobrowski <mattbobrowski@google.com> wrote:
> > > >
> > > [...]
> > > > +       len = buf + buf__sz - ret;
> > > > +       memmove(buf, ret, len);
> > > > +       return len;
> > > > +}
> > > > +__bpf_kfunc_end_defs();
> > > > +
> > > > +BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
> > > > +BTF_ID_FLAGS(func, bpf_get_task_exe_file,
> > > > +            KF_ACQUIRE | KF_TRUSTED_ARGS | KF_SLEEPABLE | KF_RET_NULL)
> > > > +BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE | KF_SLEEPABLE)
> > >
> > > Do we really need KF_SLEEPABLE for bpf_put_file?
> >
> > Well, the guts of fput() is annotated w/ might_sleep(), so the calling
> > thread may presumably be involuntarily put to sleep? You can also see
> > the guts of fput() invoking various indirect function calls
> > i.e. ->release(), and depending on the implementation of those, they
> > could be initiating resource release related actions which
> > consequently could result in waiting for some I/O to be done? fput()
> > also calls dput() and mntput() and these too can also do a bunch of
> > teardown.
> >
> > Please correct me if I've misunderstood something.
> 
> __fput() is annotated with might_sleep(). However, fput() doesn't not
> call __fput() directly. Instead, it schedules a worker to call __fput().
> Therefore, it is safe to call fput() from a non-sleepable context.

Oh, yes, you're absolutely right. I failed to realize that, so my
apologies. In that case, yes, technically bpf_put_file() does not need
to be annotated w/ KF_SLEEPABLE. Now that I also think of it, one of
the other and only reasons why we made this initially sleepable is
because bpf_put_file() at the time was meant to be used exclusively
within the same context as bpf_path_d_path(), and that is currently
marked as sleepable. Although technically speaking, I think we could
also make bpf_path_d_path() not restricted to only sleepable BPF LSM
program types, and in turn that could mean that
bpf_get_task_exe_file() also doesn't need to be restricted to
sleepable BPF LSM programs.

Alexei, what do you think about relaxing the sleepable annotation
across this entire set of BPF kfuncs? From a technical perspective I
think it's OK, but I'd also like someone like Christian to confirm
that d_path() can't actually end up sleeping. Glancing over it, I
believe this to be true, but I may also be naively missing something.

/M