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.
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
case to make sure socket doesn't disappear.
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.
