[PATCH bpf-next v2 1/5] bpf: Implement file local storage

[KP Singh]

On Fri, Aug 27, 2021 at 12:23 AM Alexei Starovoitov
<alexei.starovoitov at gmail.com> wrote:
>
> On Thu, Aug 26, 2021 at 07:09:09PM +0530, Kumar Kartikeya Dwivedi wrote:
> > +BPF_CALL_2(bpf_file_storage_delete, struct bpf_map *, map, struct file *, file)
> > +{
> > +     if (!file)
> > +             return -EINVAL;
> > +
> > +     return file_storage_delete(file, map);
> > +}
>
> It's exciting to see that file local storage is coming to life.
>

+1 Thanks for your work!

> What is the reason you've copy pasted inode_local_storage implementation,
> but didn't copy any comments?
> They were relevant there and just as relevant here.
> For example in the above *_storage_delete, the inode version would say:
>
> /* This helper must only called from where the inode is guaranteed
>  * to have a refcount and cannot be freed.
>  */
>
> That comment highlights the important restriction.
> The 'file' pointer should have similar restriction, right?
> But files are trickier than inodes in terms of refcnt.
> They are more similar to sockets,
> the socket_local_storage is doing refcount_inc_not_zero() in similar

Even the task_local_storage checks if the task is refcounted and going to
be around while we do a get / delete.

> case to make sure socket doesn't disappear.
>

Agreed, I would prefer if we also revisit inode_local_storage
in this respect pretty much because of what Alexei said.
One could end up with an inode (e.g. by walking pointers) in an LSM hook
whose life-cycle is not guaranteed in the current context.

This is generally not that big a deal with inodes because they are
not as ephemeral as tasks, sockets and files.

e.g. your userspace "_fd_" version of the helper does the right thing
by grabbing a
reference to the file and then dropping it once the storage is updated.

> May be socket_local_storage implementation should have been a base
> of copy-paste instead of inode_local_storage?
> Not paying attention to comments leads to this fundamental question:
> What analysis have you done to prove that this approach is correct vs
> life time of the file object?
>
> The selftest hooks into lsm/file_open and lsm/file_fcntl.
> In these cases the file pointer is valid, but the file ptr
> can be accessed via walking pointers of other objects.
>
> See commit cf28f3bbfca0 ("bpf: Use get_file_rcu() instead of get_file() for task_file iterator")
> that fixes a tricky issue with file iterator.
> It highlights that it's pretty difficult to implement 'struct file' access
> correctly. Let's double down on the safety analysis of the file local storage.