Re: [PATCH bpf-next v9 5/7] bpf: Implement bpf_local_storage for inodes

[Martin KaFai Lau]

On Tue, Aug 25, 2020 at 04:10:26PM +0200, KP Singh wrote:
> 
> 
> On 8/25/20 2:52 AM, Martin KaFai Lau wrote:
> > On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
> >> From: KP Singh <kpsingh@xxxxxxxxxx>
> >>
> >> Similar to bpf_local_storage for sockets, add local storage for inodes.
> >> The life-cycle of storage is managed with the life-cycle of the inode.
> >> i.e. the storage is destroyed along with the owning inode.
> >>
> >> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> >> security blob which are now stackable and can co-exist with other LSMs.
> >>
> > [ ... ]
> > 
> >> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> >> new file mode 100644
> >> index 000000000000..b0b283c224c1
> >> --- /dev/null
> >> +++ b/kernel/bpf/bpf_inode_storage.c
> 
> [...]
> 
> >> +
> >> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
> >> +
> >> +static struct bpf_local_storage __rcu **
> >> +inode_storage_ptr(void *owner)
> >> +{
> >> +	struct inode *inode = owner;
> >> +	struct bpf_storage_blob *bsb;
> >> +
> >> +	bsb = bpf_inode(inode);
> >> +	if (!bsb)
> >> +		return NULL;
> > just noticed this one.  NULL could be returned here.  When will it happen?
> 
> This can happen if CONFIG_BPF_LSM is enabled but "bpf" is not in the list of
> active LSMs.
> 
> > 
> >> +	return &bsb->storage;
> >> +}
> >> +
> >> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
> >> +							   struct bpf_map *map,
> >> +							   bool cacheit_lockit)
> >> +{
> 
> [...]
> 
> > path first before calling the bpf_local_storage_update() since
> > this case is specific to inode local storage.
> > 
> > Same for the other bpf_local_storage_update() cases.
> 
> If you're okay with this I can send a new series with the following updates.
> 
> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> index b0b283c224c1..74546cee814d 100644
> --- a/kernel/bpf/bpf_inode_storage.c
> +++ b/kernel/bpf/bpf_inode_storage.c
> @@ -125,7 +125,7 @@ static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
>  
>         fd = *(int *)key;
>         f = fget_raw(fd);
> -       if (!f)
> +       if (!f || !inode_storage_ptr(f->f_inode))
>                 return -EBADF;
>  
>         sdata = bpf_local_storage_update(f->f_inode,
> @@ -171,6 +171,14 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
>         if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
>                 return (unsigned long)NULL;
>  
> +       /* explicitly check that the inode_storage_ptr is not
> +        * NULL as inode_storage_lookup returns NULL in this case and
> +        * and bpf_local_storage_update expects the owner to have a
> +        * valid storage pointer.
> +        */
> +       if (!inode_storage_ptr(inode))
> +               return (unsigned long)NULL;
LGTM