Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
Hi KP,
I love your patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
[cannot apply to bpf/master linus/master linux/master v5.7 next-20200602]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/KP-Singh/Generalizing-bpf_local_storage/20200527-011230
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: c6x-randconfig-r016-20200602 (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
In file included from net/core/sock.c:133:
>> include/linux/bpf_local_storage.h:41:20: error: two or more data types in
>> declaration specifiers
41 | static inline void void bpf_inode_storage_free(struct inode *inode)
|^~~~
--
In file included from net/core/filter.c:75:
>> include/linux/bpf_local_storage.h:41:20: error: two or more data types in
>> declaration specifiers
41 | static inline void void bpf_inode_storage_free(struct inode *inode)
|^~~~
In file included from include/asm-generic/atomic.h:12,
from ./arch/c6x/include/generated/asm/atomic.h:1,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/c6x/include/asm/bitops.h:87,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/module.h:12,
from net/core/filter.c:20:
net/core/filter.c: In function 'bpf_clear_redirect_map':
arch/c6x/include/asm/cmpxchg.h:55:3: warning: value computed is not used
[-Wunused-value]
55 | ((__typeof__(*(ptr)))__cmpxchg_local_generic((ptr), |
~^
56 |(unsigned long)(o), |~
57 |(unsigned long)(n), |~
58 |sizeof(*(ptr
|
include/asm-generic/cmpxchg.h:106:28: note: in expansion of macro
'cmpxchg_local'
106 | #define cmpxchg(ptr, o, n) cmpxchg_local((ptr), (o), (n))
|^
net/core/filter.c:3529:4: note: in expansion of macro 'cmpxchg'
3529 |cmpxchg(>map, map, NULL);
|^~~
vim +41 include/linux/bpf_local_storage.h
19
20 #ifdef CONFIG_BPF_SYSCALL
21 void bpf_inode_storage_free(struct inode *inode);
22 int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk);
23 struct bpf_sk_storage_diag *
24 bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs);
25 void bpf_sk_storage_diag_free(struct bpf_sk_storage_diag *diag);
26 int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
27 struct sock *sk, struct sk_buff *skb,
28 int stg_array_type,
29 unsigned int *res_diag_size);
30 #else
31 static inline int bpf_sk_storage_clone(const struct sock *sk,
32 struct sock *newsk)
33 {
34 return 0;
35 }
36 static inline struct bpf_sk_storage_diag *
37 bpf_sk_storage_diag_alloc(const struct nlattr *nla)
38 {
39 return NULL;
40 }
> 41 static inline void void bpf_inode_storage_free(struct inode *inode)
42 {
43 }
44 static inline void bpf_sk_storage_diag_free(struct bpf_sk_storage_diag
*diag)
45 {
46 }
47 static inline int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag
*diag,
48struct sock *sk, struct
sk_buff *skb,
49int stg_array_type,
50unsigned int *res_diag_size)
51 {
52 return 0;
53 }
54 #endif
55
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
On Wed, May 27, 2020 at 6:41 PM Casey Schaufler wrote: > > On 5/27/2020 5:38 AM, KP Singh wrote: > > On 26-May 22:08, Christoph Hellwig wrote: > >> On Tue, May 26, 2020 at 06:33:34PM +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. > >>> > >>> Since, the intention is to use this in LSM programs, the destruction is > >>> done after security_inode_free in __destroy_inode. > >> NAK onbloating the inode structure. Please find an out of line way > >> to store your information. > > The other alternative is to use lbs_inode (security blobs) and we can > > do this without adding fields to struct inode. > > This is the correct approach, and always has been. This isn't the > first ( or second :( ) case where the correct behavior for an LSM > has been pretty darn obvious, but you've taken a different approach > for no apparent reason. > > > Here is a rough diff (only illustrative, won't apply cleanly) of the > > changes needed to this patch: > > > > https://gist.github.com/sinkap/1d213d17fb82a5e8ffdc3f320ec37d79 > > To do just a little nit-picking, please use bpf_inode() instead of > bpf_inode_storage(). This is in keeping with the convention used by > the other security modules. Sticking with the existing convention > makes it easier for people (and tools) that work with multiple > security modules. > > > Once tracing has gets a whitelist based access to inode storage, I > > guess it, too, can use bpf_local_storage for inodes > > Only within the BPF module. Your sentence above is slightly garbled, > so I'm not really sure what you're saying, but if you're suggesting > that tracing code outside of the BPF security module can use the > BPF inode data, the answer is a resounding "no". This is why I wanted to add a separate pointer in struct inode so that we could share the implementation with tracing. bpf_local_storage is managed (per-program+per-type of storage) with separate BPF maps. So, it can be easily shared between two programs (and program types) without them clobbering over each other. I guess we can have separate pointers for tracing, use the pointer in the security blob for the LSM and discuss this separately if and when we use this for tracing and keep this series patches scoped to BPF_PROG_TYPE_LSM. - KP > > > if CONFIG_BPF_LSM > > is enabled. Does this sound reasonable to the BPF folks? > > > > - KP > > > > >
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
On 5/27/2020 5:38 AM, KP Singh wrote: > On 26-May 22:08, Christoph Hellwig wrote: >> On Tue, May 26, 2020 at 06:33:34PM +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. >>> >>> Since, the intention is to use this in LSM programs, the destruction is >>> done after security_inode_free in __destroy_inode. >> NAK onbloating the inode structure. Please find an out of line way >> to store your information. > The other alternative is to use lbs_inode (security blobs) and we can > do this without adding fields to struct inode. This is the correct approach, and always has been. This isn't the first ( or second :( ) case where the correct behavior for an LSM has been pretty darn obvious, but you've taken a different approach for no apparent reason. > Here is a rough diff (only illustrative, won't apply cleanly) of the > changes needed to this patch: > > https://gist.github.com/sinkap/1d213d17fb82a5e8ffdc3f320ec37d79 To do just a little nit-picking, please use bpf_inode() instead of bpf_inode_storage(). This is in keeping with the convention used by the other security modules. Sticking with the existing convention makes it easier for people (and tools) that work with multiple security modules. > Once tracing has gets a whitelist based access to inode storage, I > guess it, too, can use bpf_local_storage for inodes Only within the BPF module. Your sentence above is slightly garbled, so I'm not really sure what you're saying, but if you're suggesting that tracing code outside of the BPF security module can use the BPF inode data, the answer is a resounding "no". > if CONFIG_BPF_LSM > is enabled. Does this sound reasonable to the BPF folks? > > - KP > >
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
On 26-May 22:08, Christoph Hellwig wrote: > On Tue, May 26, 2020 at 06:33:34PM +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. > > > > Since, the intention is to use this in LSM programs, the destruction is > > done after security_inode_free in __destroy_inode. > > NAK onbloating the inode structure. Please find an out of line way > to store your information. The other alternative is to use lbs_inode (security blobs) and we can do this without adding fields to struct inode. Here is a rough diff (only illustrative, won't apply cleanly) of the changes needed to this patch: https://gist.github.com/sinkap/1d213d17fb82a5e8ffdc3f320ec37d79 Once tracing has gets a whitelist based access to inode storage, I guess it, too, can use bpf_local_storage for inodes if CONFIG_BPF_LSM is enabled. Does this sound reasonable to the BPF folks? - KP
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
On Tue, May 26, 2020 at 06:33:34PM +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. > > Since, the intention is to use this in LSM programs, the destruction is > done after security_inode_free in __destroy_inode. NAK onbloating the inode structure. Please find an out of line way to store your information.
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
Thanks for taking a look!
On 26-May 17:49, Alexei Starovoitov wrote:
> On Tue, May 26, 2020 at 06:33:34PM +0200, KP Singh wrote:
> >
> > +static struct bpf_local_storage_data *inode_storage_update(
> > + struct inode *inode, struct bpf_map *map, void *value, u64 map_flags)
> > +{
> > + struct bpf_local_storage_data *old_sdata = NULL;
> > + struct bpf_local_storage_elem *selem;
> > + struct bpf_local_storage *local_storage;
> > + struct bpf_local_storage_map *smap;
> > + int err;
> > +
> > + err = check_update_flags(map, map_flags);
> > + if (err)
> > + return ERR_PTR(err);
> > +
> > + smap = (struct bpf_local_storage_map *)map;
> > + local_storage = rcu_dereference(inode->inode_bpf_storage);
> > +
> > + if (!local_storage || hlist_empty(_storage->list)) {
> > + /* Very first elem for this inode */
> > + err = check_flags(NULL, map_flags);
> > + if (err)
> > + return ERR_PTR(err);
> > +
> > + selem = selem_alloc(smap, value);
> > + if (!selem)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + err = inode_storage_alloc(inode, smap, selem);
>
> inode_storage_update looks like big copy-paste except above one line.
> pls consolidate.
Sure.
>
> > +BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *,
> > inode,
> > + void *, value, u64, flags)
> > +{
> > + struct bpf_local_storage_data *sdata;
> > +
> > + if (flags > BPF_LOCAL_STORAGE_GET_F_CREATE)
> > + return (unsigned long)NULL;
> > +
> > + sdata = inode_storage_lookup(inode, map, true);
> > + if (sdata)
> > + return (unsigned long)sdata->data;
> > +
> > + if (flags == BPF_LOCAL_STORAGE_GET_F_CREATE &&
> > + atomic_inc_not_zero(>i_count)) {
> > + sdata = inode_storage_update(inode, map, value, BPF_NOEXIST);
> > + iput(inode);
> > + return IS_ERR(sdata) ?
> > + (unsigned long)NULL : (unsigned long)sdata->data;
> > + }
>
> This is wrong. You cannot just copy paste the refcounting logic
> from bpf_sk_storage_get(). sk->sk_refcnt is very different from
> inode->i_count.
> To start, the inode->i_count cannot be incremented without lock.
Good catch! Agreed, Jann pointed out that this can lead to bugs
similar to https://crbug.com/project-zero/2015.
> If you really need to do it you need igrab().
> Secondly, the iput() is not possible to call from bpf prog yet, since
> progs are not sleepable and iput() may call iput_final() which may sleep.
Agreed, I will send a separate patch to add a might_sleep call to
iput() which currently only has a "Consequently, iput() can sleep."
warning in the comments so that this can be caught by
CONFIG_DEBUG_ATOMIC_SLEEP.
> But considering that only lsm progs from lsm hooks will call
> bpf_inode_storage_get()
> the inode is not going to disappear while this function is running.
If the inode pointer is an argument to the LSM hook, it won't
disappear and yes this does hold generally true for the other
use-cases as well.
> So why touch i_count ?
>
> > +
> > + return (unsigned long)NULL;
> > +}
> > +
> > BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *, map, struct sock *, sk)
> > {
> > if (refcount_inc_not_zero(>sk_refcnt)) {
> > @@ -957,6 +1229,20 @@ BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *,
> > map, struct sock *, sk)
> > return -ENOENT;
> > }
> >
> > +BPF_CALL_2(bpf_inode_storage_delete,
> > + struct bpf_map *, map, struct inode *, inode)
> > +{
> > + int err;
> > +
> > + if (atomic_inc_not_zero(>i_count)) {
> > + err = inode_storage_delete(inode, map);
> > + iput(inode);
> > + return err;
> > + }
>
> ditto.
>
> > +
> > + return inode_storage_delete(inode, map);
>
> bad copy-paste from bpf_sk_storage_delete?
> or what is this logic suppose to do?
The former :) fixed...
- KP
Re: [PATCH bpf-next 2/4] bpf: Implement bpf_local_storage for inodes
On Tue, May 26, 2020 at 06:33:34PM +0200, KP Singh wrote:
>
> +static struct bpf_local_storage_data *inode_storage_update(
> + struct inode *inode, struct bpf_map *map, void *value, u64 map_flags)
> +{
> + struct bpf_local_storage_data *old_sdata = NULL;
> + struct bpf_local_storage_elem *selem;
> + struct bpf_local_storage *local_storage;
> + struct bpf_local_storage_map *smap;
> + int err;
> +
> + err = check_update_flags(map, map_flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + smap = (struct bpf_local_storage_map *)map;
> + local_storage = rcu_dereference(inode->inode_bpf_storage);
> +
> + if (!local_storage || hlist_empty(_storage->list)) {
> + /* Very first elem for this inode */
> + err = check_flags(NULL, map_flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + selem = selem_alloc(smap, value);
> + if (!selem)
> + return ERR_PTR(-ENOMEM);
> +
> + err = inode_storage_alloc(inode, smap, selem);
inode_storage_update looks like big copy-paste except above one line.
pls consolidate.
> +BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *,
> inode,
> +void *, value, u64, flags)
> +{
> + struct bpf_local_storage_data *sdata;
> +
> + if (flags > BPF_LOCAL_STORAGE_GET_F_CREATE)
> + return (unsigned long)NULL;
> +
> + sdata = inode_storage_lookup(inode, map, true);
> + if (sdata)
> + return (unsigned long)sdata->data;
> +
> + if (flags == BPF_LOCAL_STORAGE_GET_F_CREATE &&
> + atomic_inc_not_zero(>i_count)) {
> + sdata = inode_storage_update(inode, map, value, BPF_NOEXIST);
> + iput(inode);
> + return IS_ERR(sdata) ?
> + (unsigned long)NULL : (unsigned long)sdata->data;
> + }
This is wrong. You cannot just copy paste the refcounting logic
from bpf_sk_storage_get(). sk->sk_refcnt is very different from inode->i_count.
To start, the inode->i_count cannot be incremented without lock.
If you really need to do it you need igrab().
Secondly, the iput() is not possible to call from bpf prog yet, since
progs are not sleepable and iput() may call iput_final() which may sleep.
But considering that only lsm progs from lsm hooks will call
bpf_inode_storage_get()
the inode is not going to disappear while this function is running.
So why touch i_count ?
> +
> + return (unsigned long)NULL;
> +}
> +
> BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *, map, struct sock *, sk)
> {
> if (refcount_inc_not_zero(>sk_refcnt)) {
> @@ -957,6 +1229,20 @@ BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *,
> map, struct sock *, sk)
> return -ENOENT;
> }
>
> +BPF_CALL_2(bpf_inode_storage_delete,
> +struct bpf_map *, map, struct inode *, inode)
> +{
> + int err;
> +
> + if (atomic_inc_not_zero(>i_count)) {
> + err = inode_storage_delete(inode, map);
> + iput(inode);
> + return err;
> + }
ditto.
> +
> + return inode_storage_delete(inode, map);
bad copy-paste from bpf_sk_storage_delete?
or what is this logic suppose to do?
[PATCH bpf-next 2/4] 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.
Since, the intention is to use this in LSM programs, the destruction is
done after security_inode_free in __destroy_inode.
Signed-off-by: KP Singh
---
fs/inode.c| 3 +
include/linux/bpf_local_storage.h | 6 +
include/linux/bpf_types.h | 1 +
include/linux/fs.h| 5 +
include/uapi/linux/bpf.h | 41 +++-
kernel/bpf/bpf_local_storage.c| 321 +-
kernel/bpf/syscall.c | 3 +-
kernel/bpf/verifier.c | 10 +
tools/bpf/bpftool/map.c | 1 +
tools/include/uapi/linux/bpf.h| 41 +++-
tools/lib/bpf/libbpf_probes.c | 5 +-
11 files changed, 431 insertions(+), 6 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index cc6e701b7e5d..91c81644f33d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include "internal.h"
/*
@@ -257,6 +258,8 @@ void __destroy_inode(struct inode *inode)
security_inode_free(inode);
fsnotify_inode_delete(inode);
locks_free_lock_context(inode);
+ bpf_inode_storage_free(inode);
+
if (!inode->i_nlink) {
WARN_ON(atomic_long_read(>i_sb->s_remove_count) == 0);
atomic_long_dec(>i_sb->s_remove_count);
diff --git a/include/linux/bpf_local_storage.h
b/include/linux/bpf_local_storage.h
index 85524f18cd91..c6837e7838fc 100644
--- a/include/linux/bpf_local_storage.h
+++ b/include/linux/bpf_local_storage.h
@@ -9,6 +9,8 @@ void bpf_sk_storage_free(struct sock *sk);
extern const struct bpf_func_proto bpf_sk_storage_get_proto;
extern const struct bpf_func_proto bpf_sk_storage_delete_proto;
+extern const struct bpf_func_proto bpf_inode_storage_get_proto;
+extern const struct bpf_func_proto bpf_inode_storage_delete_proto;
struct bpf_sk_storage_diag;
struct sk_buff;
@@ -16,6 +18,7 @@ struct nlattr;
struct sock;
#ifdef CONFIG_BPF_SYSCALL
+void bpf_inode_storage_free(struct inode *inode);
int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk);
struct bpf_sk_storage_diag *
bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs);
@@ -35,6 +38,9 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla)
{
return NULL;
}
+static inline void void bpf_inode_storage_free(struct inode *inode)
+{
+}
static inline void bpf_sk_storage_diag_free(struct bpf_sk_storage_diag *diag)
{
}
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 29d22752fc87..07181fb89bdd 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -101,6 +101,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS,
htab_of_maps_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_INODE_STORAGE, inode_storage_map_ops)
#if defined(CONFIG_BPF_STREAM_PARSER)
BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5ee9e583bde2..23a6b8fbd381 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -624,6 +624,7 @@ is_uncached_acl(struct posix_acl *acl)
#define IOP_DEFAULT_READLINK 0x0010
struct fsnotify_mark_connector;
+struct bpf_local_storage;
/*
* Keep mostly read-only and often accessed (especially for
@@ -740,6 +741,10 @@ struct inode {
struct fsverity_info*i_verity_info;
#endif
+#ifdef CONFIG_BPF_SYSCALL
+ struct bpf_local_storage __rcu *inode_bpf_storage;
+#endif
+
void*i_private; /* fs or device private pointer */
} __randomize_layout;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 4a202eea15c0..410fb2e5fdd4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -147,6 +147,7 @@ enum bpf_map_type {
BPF_MAP_TYPE_SK_STORAGE,
BPF_MAP_TYPE_DEVMAP_HASH,
BPF_MAP_TYPE_STRUCT_OPS,
+ BPF_MAP_TYPE_INODE_STORAGE,
};
/* Note that tracing related programs such as
@@ -3157,6 +3158,42 @@ union bpf_attr {
* **bpf_sk_cgroup_id**\ ().
* Return
* The id is returned or 0 in case the id could not be retrieved.
+ *
+ * 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
