On Fri, Aug 27, 2021 at 05:43:41AM IST, KP Singh wrote:

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

quoted

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!

quoted

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.

quoted

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.

Thank you both of you for the comments. I will revisit this and inode_storage
and get back to you, soon.

--
Kartikeya