Re: [PATCH bpf-next v8 5/7] bpf: Implement bpf_local_storage for inodes
On 8/18/20 5:23 PM, Martin KaFai Lau wrote: > On Tue, Aug 18, 2020 at 05:10:34PM +0200, KP Singh wrote: >> >> >> On 8/18/20 3:27 AM, Martin KaFai Lau wrote:>>> On Mon, Aug 03, 2020 at >> 06:46:53PM +0200, KP Singh wrote: [...] a get_file >> rather fcheck followed by get_file_rcu: >> >> #define get_file_rcu_many(x, cnt)\ >> atomic_long_add_unless(&(x)->f_count, (cnt), 0) >> #define get_file_rcu(x) get_file_rcu_many((x), 1) >> #define file_count(x)atomic_long_read(&(x)->f_count) >> >> But there is an easier way than all of this and this is to use >> fget_raw which also calls get_file_rcu_many >> and ensures a non-zero count before getting a reference. > ic. Make sense. > > There are fdget() and fdput() also which are used in bpf/syscall.c. Yeah we could use fdget_raw but we don't really need the struct fd but just the struct file. he non-raw versions masks away FMODE_PATH (O_PATH) files, we should still be able to access blobs on the O_PATH files, thus the _raw version here. - KP >
Re: [PATCH bpf-next v8 5/7] bpf: Implement bpf_local_storage for inodes
On Tue, Aug 18, 2020 at 05:10:34PM +0200, KP Singh wrote: > > > On 8/18/20 3:27 AM, Martin KaFai Lau wrote: > > On Mon, Aug 03, 2020 at 06:46:53PM +0200, KP Singh wrote: > >> From: KP Singh > >> > >> 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. > >> > >> Signed-off-by: KP Singh > >> --- > >> include/linux/bpf_local_storage.h | 10 + > >> include/linux/bpf_lsm.h | 21 ++ > >> include/linux/bpf_types.h | 3 + > >> include/uapi/linux/bpf.h | 38 +++ > >> kernel/bpf/Makefile | 1 + > > [...] > > ata *inode_storage_lookup(struct inode *inode, > >> + struct bpf_map *map, > >> + bool cacheit_lockit) > >> +{ > >> + struct bpf_local_storage *inode_storage; > >> + struct bpf_local_storage_map *smap; > >> + struct bpf_storage_blob *bsb; > >> + > >> + bsb = bpf_inode(inode); > >> + if (!bsb) > >> + return ERR_PTR(-ENOENT); > > ERR_PTR is returned here... > > > >> + > >> + inode_storage = rcu_dereference(bsb->storage); > >> + if (!inode_storage) > >> + return NULL; > >> + > > [...] > > >> + kfree_rcu(local_storage, rcu); > >> +} > >> + > >> + > >> +static void *bpf_fd_inode_storage_lookup_elem(struct bpf_map *map, void > >> *key) > >> +{ > >> + struct bpf_local_storage_data *sdata; > >> + struct file *f; > >> + int fd; > >> + > >> + fd = *(int *)key; > >> + f = fcheck(fd); > >> + if (!f) > >> + return ERR_PTR(-EINVAL); > >> + > >> + get_file(f); > >> + sdata = inode_storage_lookup(f->f_inode, map, true); > >> + fput(f); > >> + return sdata ? sdata->data : NULL; > > sdata can be ERR_PTR here and a few other cases below. > > > > May be inode_storage_lookup() should just return NULL. > > I think returning NULL is a better option. Thanks! > > > > >> +} > >> + > >> +static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void > >> *key, > >> + void *value, u64 map_flags) > >> +{ > >> + struct bpf_local_storage_data *sdata; > >> + struct file *f; > >> + int fd; > >> + > >> + fd = *(int *)key; > >> + f = fcheck(fd); > >> + if (!f) > >> + return -EINVAL; > >> + > >> + get_file(f);> get_file() does atomic_long_inc() instead of > >> atomic_long_inc_not_zero(). > > I don't see how that helps here. Am I missing something? > > You are right, this should not not be an fcheck followed by a get_file > rather fcheck followed by get_file_rcu: > > #define get_file_rcu_many(x, cnt) \ > atomic_long_add_unless(&(x)->f_count, (cnt), 0) > #define get_file_rcu(x) get_file_rcu_many((x), 1) > #define file_count(x) atomic_long_read(&(x)->f_count) > > But there is an easier way than all of this and this is to use > fget_raw which also calls get_file_rcu_many > and ensures a non-zero count before getting a reference. ic. Make sense. There are fdget() and fdput() also which are used in bpf/syscall.c.
Re: [PATCH bpf-next v8 5/7] bpf: Implement bpf_local_storage for inodes
On 8/18/20 3:27 AM, Martin KaFai Lau wrote: > On Mon, Aug 03, 2020 at 06:46:53PM +0200, KP Singh wrote: >> From: KP Singh >> >> 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. >> >> Signed-off-by: KP Singh >> --- >> include/linux/bpf_local_storage.h | 10 + >> include/linux/bpf_lsm.h | 21 ++ >> include/linux/bpf_types.h | 3 + >> include/uapi/linux/bpf.h | 38 +++ >> kernel/bpf/Makefile | 1 + [...] ata *inode_storage_lookup(struct inode *inode, >> + struct bpf_map *map, >> + bool cacheit_lockit) >> +{ >> +struct bpf_local_storage *inode_storage; >> +struct bpf_local_storage_map *smap; >> +struct bpf_storage_blob *bsb; >> + >> +bsb = bpf_inode(inode); >> +if (!bsb) >> +return ERR_PTR(-ENOENT); > ERR_PTR is returned here... > >> + >> +inode_storage = rcu_dereference(bsb->storage); >> +if (!inode_storage) >> +return NULL; >> + [...] >> +kfree_rcu(local_storage, rcu); >> +} >> + >> + >> +static void *bpf_fd_inode_storage_lookup_elem(struct bpf_map *map, void >> *key) >> +{ >> +struct bpf_local_storage_data *sdata; >> +struct file *f; >> +int fd; >> + >> +fd = *(int *)key; >> +f = fcheck(fd); >> +if (!f) >> +return ERR_PTR(-EINVAL); >> + >> +get_file(f); >> +sdata = inode_storage_lookup(f->f_inode, map, true); >> +fput(f); >> +return sdata ? sdata->data : NULL; > sdata can be ERR_PTR here and a few other cases below. > > May be inode_storage_lookup() should just return NULL. I think returning NULL is a better option. Thanks! > >> +} >> + >> +static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key, >> + void *value, u64 map_flags) >> +{ >> +struct bpf_local_storage_data *sdata; >> +struct file *f; >> +int fd; >> + >> +fd = *(int *)key; >> +f = fcheck(fd); >> +if (!f) >> +return -EINVAL; >> + >> +get_file(f);> get_file() does atomic_long_inc() instead of >> atomic_long_inc_not_zero(). > I don't see how that helps here. Am I missing something? You are right, this should not not be an fcheck followed by a get_file rather fcheck followed by get_file_rcu: #define get_file_rcu_many(x, cnt) \ atomic_long_add_unless(&(x)->f_count, (cnt), 0) #define get_file_rcu(x) get_file_rcu_many((x), 1) #define file_count(x) atomic_long_read(&(x)->f_count) But there is an easier way than all of this and this is to use fget_raw which also calls get_file_rcu_many and ensures a non-zero count before getting a reference. - KP > >> +sdata = bpf_local_storage_update(f->f_inode, map, value, map_flags); >> +fput(f); >> +return PTR_ERR_OR_ZERO(sdata); >> +} >> +
Re: [PATCH bpf-next v8 5/7] bpf: Implement bpf_local_storage for inodes
On Mon, Aug 03, 2020 at 06:46:53PM +0200, KP Singh wrote: > From: KP Singh > > 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. > > Signed-off-by: KP Singh > --- > include/linux/bpf_local_storage.h | 10 + > include/linux/bpf_lsm.h | 21 ++ > include/linux/bpf_types.h | 3 + > include/uapi/linux/bpf.h | 38 +++ > kernel/bpf/Makefile | 1 + > kernel/bpf/bpf_inode_storage.c| 265 ++ > kernel/bpf/syscall.c | 3 +- > kernel/bpf/verifier.c | 10 + > security/bpf/hooks.c | 7 + > .../bpf/bpftool/Documentation/bpftool-map.rst | 2 +- > tools/bpf/bpftool/bash-completion/bpftool | 3 +- > tools/bpf/bpftool/map.c | 3 +- > tools/include/uapi/linux/bpf.h| 38 +++ > tools/lib/bpf/libbpf_probes.c | 5 +- > 14 files changed, 403 insertions(+), 6 deletions(-) > create mode 100644 kernel/bpf/bpf_inode_storage.c > > diff --git a/include/linux/bpf_local_storage.h > b/include/linux/bpf_local_storage.h > index 3685f681a7fc..75c76847fad5 100644 > --- a/include/linux/bpf_local_storage.h > +++ b/include/linux/bpf_local_storage.h > @@ -160,4 +160,14 @@ struct bpf_local_storage_data * > bpf_local_storage_update(void *owner, struct bpf_map *map, void *value, >u64 map_flags); > > +#ifdef CONFIG_BPF_LSM > +extern const struct bpf_func_proto bpf_inode_storage_get_proto; > +extern const struct bpf_func_proto bpf_inode_storage_delete_proto; > +void bpf_inode_storage_free(struct inode *inode); > +#else > +static inline void bpf_inode_storage_free(struct inode *inode) > +{ > +} > +#endif /* CONFIG_BPF_LSM */ This is LSM specific. Can they be moved to bpf_lsm.h? > + > #endif /* _BPF_LOCAL_STORAGE_H */ > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h > index af74712af585..d0683ada1e49 100644 > --- a/include/linux/bpf_lsm.h > +++ b/include/linux/bpf_lsm.h > @@ -17,9 +17,24 @@ > #include > #undef LSM_HOOK > > +struct bpf_storage_blob { > + struct bpf_local_storage __rcu *storage; > +}; > + > +extern struct lsm_blob_sizes bpf_lsm_blob_sizes; > + > int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > const struct bpf_prog *prog); > > +static inline struct bpf_storage_blob *bpf_inode( > + const struct inode *inode) > +{ > + if (unlikely(!inode->i_security)) > + return NULL; > + > + return inode->i_security + bpf_lsm_blob_sizes.lbs_inode; > +} > + > #else /* !CONFIG_BPF_LSM */ > > static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > @@ -28,6 +43,12 @@ static inline int bpf_lsm_verify_prog(struct > bpf_verifier_log *vlog, > return -EOPNOTSUPP; > } > > +static inline struct bpf_storage_blob *bpf_inode( > + const struct inode *inode) > +{ > + return NULL; > +} > + > #endif /* CONFIG_BPF_LSM */ > > #endif /* _LINUX_BPF_LSM_H */ [ ... ] > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c > new file mode 100644 > index ..a51a6328c02d > --- /dev/null > +++ b/kernel/bpf/bpf_inode_storage.c > @@ -0,0 +1,265 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2019 Facebook > + * Copyright 2020 Google LLC. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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; > + return >storage; > +} > + > +static struct bpf_local_storage_data *inode_storage_lookup(struct inode > *inode, > +struct bpf_map *map, > +bool cacheit_lockit) > +{ > + struct bpf_local_storage *inode_storage; > + struct bpf_local_storage_map *smap; > + struct bpf_storage_blob *bsb; > + > + bsb = bpf_inode(inode); > + if (!bsb) > + return ERR_PTR(-ENOENT); ERR_PTR is returned here... > + > + inode_storage = rcu_dereference(bsb->storage); > + if (!inode_storage) > + return NULL; > + > + smap = (struct bpf_local_storage_map *)map; > + return bpf_local_storage_lookup(inode_storage, smap, cacheit_lockit); > +} > + > +void
[PATCH bpf-next v8 5/7] bpf: Implement bpf_local_storage for inodes
From: KP Singh 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. Signed-off-by: KP Singh --- include/linux/bpf_local_storage.h | 10 + include/linux/bpf_lsm.h | 21 ++ include/linux/bpf_types.h | 3 + include/uapi/linux/bpf.h | 38 +++ kernel/bpf/Makefile | 1 + kernel/bpf/bpf_inode_storage.c| 265 ++ kernel/bpf/syscall.c | 3 +- kernel/bpf/verifier.c | 10 + security/bpf/hooks.c | 7 + .../bpf/bpftool/Documentation/bpftool-map.rst | 2 +- tools/bpf/bpftool/bash-completion/bpftool | 3 +- tools/bpf/bpftool/map.c | 3 +- tools/include/uapi/linux/bpf.h| 38 +++ tools/lib/bpf/libbpf_probes.c | 5 +- 14 files changed, 403 insertions(+), 6 deletions(-) create mode 100644 kernel/bpf/bpf_inode_storage.c diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h index 3685f681a7fc..75c76847fad5 100644 --- a/include/linux/bpf_local_storage.h +++ b/include/linux/bpf_local_storage.h @@ -160,4 +160,14 @@ struct bpf_local_storage_data * bpf_local_storage_update(void *owner, struct bpf_map *map, void *value, u64 map_flags); +#ifdef CONFIG_BPF_LSM +extern const struct bpf_func_proto bpf_inode_storage_get_proto; +extern const struct bpf_func_proto bpf_inode_storage_delete_proto; +void bpf_inode_storage_free(struct inode *inode); +#else +static inline void bpf_inode_storage_free(struct inode *inode) +{ +} +#endif /* CONFIG_BPF_LSM */ + #endif /* _BPF_LOCAL_STORAGE_H */ diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index af74712af585..d0683ada1e49 100644 --- a/include/linux/bpf_lsm.h +++ b/include/linux/bpf_lsm.h @@ -17,9 +17,24 @@ #include #undef LSM_HOOK +struct bpf_storage_blob { + struct bpf_local_storage __rcu *storage; +}; + +extern struct lsm_blob_sizes bpf_lsm_blob_sizes; + int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, const struct bpf_prog *prog); +static inline struct bpf_storage_blob *bpf_inode( + const struct inode *inode) +{ + if (unlikely(!inode->i_security)) + return NULL; + + return inode->i_security + bpf_lsm_blob_sizes.lbs_inode; +} + #else /* !CONFIG_BPF_LSM */ static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, @@ -28,6 +43,12 @@ static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, return -EOPNOTSUPP; } +static inline struct bpf_storage_blob *bpf_inode( + const struct inode *inode) +{ + return NULL; +} + #endif /* CONFIG_BPF_LSM */ #endif /* _LINUX_BPF_LSM_H */ diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h index a52a5688418e..2e6f568377f1 100644 --- a/include/linux/bpf_types.h +++ b/include/linux/bpf_types.h @@ -107,6 +107,9 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops) BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops) BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops) #endif +#ifdef CONFIG_BPF_LSM +BPF_MAP_TYPE(BPF_MAP_TYPE_INODE_STORAGE, inode_storage_map_ops) +#endif BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops) #if defined(CONFIG_XDP_SOCKETS) BPF_MAP_TYPE(BPF_MAP_TYPE_XSKMAP, xsk_map_ops) diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 35629752cec8..e17c00eea5d8 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -149,6 +149,7 @@ enum bpf_map_type { BPF_MAP_TYPE_DEVMAP_HASH, BPF_MAP_TYPE_STRUCT_OPS, BPF_MAP_TYPE_RINGBUF, + BPF_MAP_TYPE_INODE_STORAGE, }; /* Note that tracing related programs such as @@ -3394,6 +3395,41 @@ union bpf_attr { * A non-negative value equal to or less than *size* on success, * or a negative error in case of failure. * + * void *bpf_inode_storage_get(struct bpf_map *map, void *inode, void *value, u64 flags) + * Description + * Get a bpf_local_storage from an *inode*. + * + * Logically, it could be thought of as getting the value from + * a *map* with *inode* as the **key**. From this + * perspective, the usage is not much different from + * **bpf_map_lookup_elem**\ (*map*, **&**\ *inode*) except this + * helper enforces the key must be an inode and the map must also + * be a **BPF_MAP_TYPE_INODE_STORAGE**. + * + * Underneath, the value is stored locally at *inode* instead of + * the *map*. The *map* is used as the