On Fri, Aug 27, 2021 at 05:43:41AM IST, KP Singh wrote:
> On Fri, Aug 27, 2021 at 12:23 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> 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.
>
Thank you both of you for the comments. I will revisit this and inode_storage
and get back to you, soon.
--
Kartikeya
