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

On Fri, Aug 27, 2021 at 12:23 AM Alexei Starovoitov
[off-list ref] wrote:

On Thu, Aug 26, 2021 at 07:09:09PM +0530, Kumar Kartikeya Dwivedi wrote:

quoted

+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.