[PATCH v8 bpf-next 00/13] bpf: Add d_path helper

[PATCH v8 bpf-next 00/13] bpf: Add d_path helper

[Jiri Olsa]

hi,
adding d_path helper function that returns full path for
given 'struct path' object, which needs to be the kernel
BTF 'path' object. The path is returned in buffer provided
'buf' of size 'sz' and is zero terminated.

  int bpf_d_path(struct path *path, char *buf, u32 sz);

The helper calls directly d_path function, so there's only
limited set of function it can be called from.

The patchset also adds support to add set of BTF IDs for
a helper to define functions that the helper is allowed
to be called from.

Also available at:
  https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  bpf/d_path

v8 changes:
  - rebased on Yonghong's latest changes
  - factored btf_struct_access function [Andrii]
  - fixed leftovers fro mthe preparation changes

thanks,
jirka


---
Jiri Olsa (13):
      selftests/bpf: Fix resolve_btfids test
      tools resolve_btfids: Add support for set symbols
      bpf: Move btf_resolve_size into __btf_resolve_size
      bpf: Add elem_id pointer as argument to __btf_resolve_size
      bpf: Add type_id pointer as argument to __btf_resolve_size
      bpf: Factor btf_struct_access function
      bpf: Add btf_struct_ids_match function
      bpf: Add BTF_SET_START/END macros
      bpf: Add d_path helper
      bpf: Update .BTF_ids section in btf.rst with sets info
      selftests/bpf: Add verifier test for d_path helper
      selftests/bpf: Add test for d_path helper
      selftests/bpf: Add set test to resolve_btfids

 Documentation/bpf/btf.rst                               |  25 +++++++++++++++
 include/linux/bpf.h                                     |   6 ++++
 include/linux/btf.h                                     |   3 +-
 include/linux/btf_ids.h                                 |  43 ++++++++++++++++++++++++-
 include/uapi/linux/bpf.h                                |  13 ++++++++
 kernel/bpf/bpf_struct_ops.c                             |   6 ++--
 kernel/bpf/btf.c                                        | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------
 kernel/bpf/verifier.c                                   |  23 ++++++++++----
 kernel/trace/bpf_trace.c                                |  48 ++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py                              |   2 ++
 tools/bpf/resolve_btfids/main.c                         |  15 ++++++++-
 tools/include/linux/btf_ids.h                           |  43 ++++++++++++++++++++++++-
 tools/include/uapi/linux/bpf.h                          |  13 ++++++++
 tools/testing/selftests/bpf/prog_tests/d_path.c         | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/prog_tests/resolve_btfids.c |  34 ++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_d_path.c         |  64 ++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/test_verifier.c             |  19 +++++++++++-
 tools/testing/selftests/bpf/verifier/d_path.c           |  37 ++++++++++++++++++++++
 18 files changed, 667 insertions(+), 37 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c
 create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c

[PATCH v8 bpf-next 01/13] selftests/bpf: Fix resolve_btfids test

[Jiri Olsa]

The linux/btf_ids.h header is now using CONFIG_DEBUG_INFO_BTF
config, so we need to have it defined when it's available.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/resolve_btfids.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
index 3b127cab4864..101785b49f7e 100644
--- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
+++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include "autoconf.h"
 #include <linux/err.h>
 #include <string.h>
 #include <bpf/btf.h>
-- 
2.25.4

Re: [PATCH v8 bpf-next 01/13] selftests/bpf: Fix resolve_btfids test

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The linux/btf_ids.h header is now using CONFIG_DEBUG_INFO_BTF
> config, so we need to have it defined when it's available.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

sure, why not

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/prog_tests/resolve_btfids.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> index 3b127cab4864..101785b49f7e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> +#include "autoconf.h"
>  #include <linux/err.h>
>  #include <string.h>
>  #include <bpf/btf.h>
> --
> 2.25.4
>

Re: [PATCH v8 bpf-next 01/13] selftests/bpf: Fix resolve_btfids test

[Jiri Olsa]

On Tue, Jul 28, 2020 at 01:27:49PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The linux/btf_ids.h header is now using CONFIG_DEBUG_INFO_BTF
> > config, so we need to have it defined when it's available.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> sure, why not

actually after rebase I realized Yonghong added
CONFIG_DEBUG_INFO_BTF define in:
  d8dfe5bfe856 ("tools/bpf: Sync btf_ids.h to tools")

I think including 'autoconf.h' is more clean,
on the other hand I don't think we'd get clean
selftest compile without CONFIG_DEBUG_INFO_BTF

should we keep the #define instead?

thanks,
jirka

> 
> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
> >  tools/testing/selftests/bpf/prog_tests/resolve_btfids.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > index 3b127cab4864..101785b49f7e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >
> > +#include "autoconf.h"
> >  #include <linux/err.h>
> >  #include <string.h>
> >  #include <bpf/btf.h>
> > --
> > 2.25.4
> >
> 

Re: [PATCH v8 bpf-next 01/13] selftests/bpf: Fix resolve_btfids test

[Andrii Nakryiko]

On Wed, Jul 29, 2020 at 1:07 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jul 28, 2020 at 01:27:49PM -0700, Andrii Nakryiko wrote:
> > On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > The linux/btf_ids.h header is now using CONFIG_DEBUG_INFO_BTF
> > > config, so we need to have it defined when it's available.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> >
> > sure, why not
>
> actually after rebase I realized Yonghong added
> CONFIG_DEBUG_INFO_BTF define in:
>   d8dfe5bfe856 ("tools/bpf: Sync btf_ids.h to tools")
>
> I think including 'autoconf.h' is more clean,
> on the other hand I don't think we'd get clean
> selftest compile without CONFIG_DEBUG_INFO_BTF
>
> should we keep the #define instead?
>

#define is fine with me

> thanks,
> jirka
>
> >
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
> >
> > >  tools/testing/selftests/bpf/prog_tests/resolve_btfids.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > index 3b127cab4864..101785b49f7e 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > > @@ -1,5 +1,6 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >
> > > +#include "autoconf.h"
> > >  #include <linux/err.h>
> > >  #include <string.h>
> > >  #include <bpf/btf.h>
> > > --
> > > 2.25.4
> > >
> >
>

[PATCH v8 bpf-next 02/13] tools resolve_btfids: Add support for set symbols

[Jiri Olsa]

The set symbol does not have the unique number suffix,
so we need to give it a special parsing function.

This was omitted in the first batch, because there was
no set support yet, so it slipped in the testing.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/bpf/resolve_btfids/main.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index 6956b6350cad..c28ab0401818 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -220,6 +220,19 @@ static char *get_id(const char *prefix_end)
 	return id;
 }
 
+static struct btf_id *add_set(struct object *obj, char *name)
+{
+	char *id;
+
+	id = strdup(name + sizeof(BTF_SET) + sizeof("__") - 2);
+	if (!id) {
+		pr_err("FAILED to parse cnt name: %s\n", name);
+		return NULL;
+	}
+
+	return btf_id__add(&obj->sets, id, true);
+}
+
 static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
 {
 	char *id;
@@ -376,7 +389,7 @@ static int symbols_collect(struct object *obj)
 			id = add_symbol(&obj->funcs, prefix, sizeof(BTF_FUNC) - 1);
 		/* set */
 		} else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
-			id = add_symbol(&obj->sets, prefix, sizeof(BTF_SET) - 1);
+			id = add_set(obj, prefix);
 			/*
 			 * SET objects store list's count, which is encoded
 			 * in symbol's size, together with 'cnt' field hence
-- 
2.25.4

Re: [PATCH v8 bpf-next 02/13] tools resolve_btfids: Add support for set symbols

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The set symbol does not have the unique number suffix,
> so we need to give it a special parsing function.
>
> This was omitted in the first batch, because there was
> no set support yet, so it slipped in the testing.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/bpf/resolve_btfids/main.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index 6956b6350cad..c28ab0401818 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -220,6 +220,19 @@ static char *get_id(const char *prefix_end)
>         return id;
>  }
>
> +static struct btf_id *add_set(struct object *obj, char *name)
> +{
> +       char *id;
> +
> +       id = strdup(name + sizeof(BTF_SET) + sizeof("__") - 2);

why strdup? you are not really managing memory carefully anyway,
letting OS clean everything up, so why bother strduping here?

Also if get invalid identifier, you can easily go past the string and
its ending zero byte. So check strlen first?

> +       if (!id) {
> +               pr_err("FAILED to parse cnt name: %s\n", name);
> +               return NULL;
> +       }
> +
> +       return btf_id__add(&obj->sets, id, true);
> +}
> +
>  static struct btf_id *add_symbol(struct rb_root *root, char *name, size_t size)
>  {
>         char *id;
> @@ -376,7 +389,7 @@ static int symbols_collect(struct object *obj)
>                         id = add_symbol(&obj->funcs, prefix, sizeof(BTF_FUNC) - 1);
>                 /* set */
>                 } else if (!strncmp(prefix, BTF_SET, sizeof(BTF_SET) - 1)) {
> -                       id = add_symbol(&obj->sets, prefix, sizeof(BTF_SET) - 1);
> +                       id = add_set(obj, prefix);
>                         /*
>                          * SET objects store list's count, which is encoded
>                          * in symbol's size, together with 'cnt' field hence
> --
> 2.25.4
>

Re: [PATCH v8 bpf-next 02/13] tools resolve_btfids: Add support for set symbols

[Jiri Olsa]

On Mon, Jul 27, 2020 at 05:53:01PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The set symbol does not have the unique number suffix,
> > so we need to give it a special parsing function.
> >
> > This was omitted in the first batch, because there was
> > no set support yet, so it slipped in the testing.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/bpf/resolve_btfids/main.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index 6956b6350cad..c28ab0401818 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -220,6 +220,19 @@ static char *get_id(const char *prefix_end)
> >         return id;
> >  }
> >
> > +static struct btf_id *add_set(struct object *obj, char *name)
> > +{
> > +       char *id;
> > +
> > +       id = strdup(name + sizeof(BTF_SET) + sizeof("__") - 2);
> 
> why strdup? you are not really managing memory carefully anyway,
> letting OS clean everything up, so why bother strduping here?

it copies the get_id logic, where we cut the unique ID part,
but we don't cut the string in here, so no reason for strdup
I'll remove it

> 
> Also if get invalid identifier, you can easily go past the string and
> its ending zero byte. So check strlen first?

right.. it's also missing in get_id funciton, will add

thanks,
jirka

> 
> > +       if (!id) {
> > +               pr_err("FAILED to parse cnt name: %s\n", name);
> > +               return NULL;
> > +       }
> > +
> > +       return btf_id__add(&obj->sets, id, true);
> > +}
> > +

SNIP

[PATCH v8 bpf-next 03/13] bpf: Move btf_resolve_size into __btf_resolve_size

[Jiri Olsa]

Moving btf_resolve_size into __btf_resolve_size and
keeping btf_resolve_size public with just first 3
arguments, because the rest of the arguments are not
used by outside callers.

Following changes are adding more arguments, which
are not useful to outside callers. They will be added
to the __btf_resolve_size function.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/btf.h         |  3 +--
 kernel/bpf/bpf_struct_ops.c |  6 ++----
 kernel/bpf/btf.c            | 21 ++++++++++++++-------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 8b81fbb4497c..a9af5e7a7ece 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -64,8 +64,7 @@ const struct btf_type *btf_type_resolve_func_ptr(const struct btf *btf,
 						 u32 id, u32 *res_id);
 const struct btf_type *
 btf_resolve_size(const struct btf *btf, const struct btf_type *type,
-		 u32 *type_size, const struct btf_type **elem_type,
-		 u32 *total_nelems);
+		 u32 *type_size);
 
 #define for_each_member(i, struct_type, member)			\
 	for (i = 0, member = btf_type_member(struct_type);	\
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 969c5d47f81f..4c3b543bb33b 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -298,8 +298,7 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 			return -EINVAL;
 
 		mtype = btf_type_by_id(btf_vmlinux, member->type);
-		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
-					 NULL, NULL);
+		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
 		if (IS_ERR(mtype))
 			return PTR_ERR(mtype);
 		prev_mend = moff + msize;
@@ -396,8 +395,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 			u32 msize;
 
 			mtype = btf_type_by_id(btf_vmlinux, member->type);
-			mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
-						 NULL, NULL);
+			mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
 			if (IS_ERR(mtype)) {
 				err = PTR_ERR(mtype);
 				goto reset_unlock;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ee36b7f60936..a68dee875a77 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1088,10 +1088,10 @@ static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
  * *elem_type: same as return type ("struct X")
  * *total_nelems: 1
  */
-const struct btf_type *
-btf_resolve_size(const struct btf *btf, const struct btf_type *type,
-		 u32 *type_size, const struct btf_type **elem_type,
-		 u32 *total_nelems)
+static const struct btf_type *
+__btf_resolve_size(const struct btf *btf, const struct btf_type *type,
+		   u32 *type_size, const struct btf_type **elem_type,
+		   u32 *total_nelems)
 {
 	const struct btf_type *array_type = NULL;
 	const struct btf_array *array;
@@ -1150,6 +1150,13 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 	return array_type ? : type;
 }
 
+const struct btf_type *
+btf_resolve_size(const struct btf *btf, const struct btf_type *type,
+		 u32 *type_size)
+{
+	return __btf_resolve_size(btf, type, type_size, NULL, NULL);
+}
+
 /* The input param "type_id" must point to a needs_resolve type */
 static const struct btf_type *btf_type_id_resolve(const struct btf *btf,
 						  u32 *type_id)
@@ -3963,8 +3970,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		mtype = btf_type_by_id(btf_vmlinux, member->type);
 		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
 
-		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
-					 &elem_type, &total_nelems);
+		mtype = __btf_resolve_size(btf_vmlinux, mtype, &msize,
+					   &elem_type, &total_nelems);
 		if (IS_ERR(mtype)) {
 			bpf_log(log, "field %s doesn't have size\n", mname);
 			return -EFAULT;
@@ -3978,7 +3985,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		if (btf_type_is_array(mtype)) {
 			u32 elem_idx;
 
-			/* btf_resolve_size() above helps to
+			/* __btf_resolve_size() above helps to
 			 * linearize a multi-dimensional array.
 			 *
 			 * The logic here is treating an array
-- 
2.25.4

Re: [PATCH v8 bpf-next 03/13] bpf: Move btf_resolve_size into __btf_resolve_size

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Moving btf_resolve_size into __btf_resolve_size and
> keeping btf_resolve_size public with just first 3
> arguments, because the rest of the arguments are not
> used by outside callers.
>
> Following changes are adding more arguments, which
> are not useful to outside callers. They will be added
> to the __btf_resolve_size function.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  include/linux/btf.h         |  3 +--
>  kernel/bpf/bpf_struct_ops.c |  6 ++----
>  kernel/bpf/btf.c            | 21 ++++++++++++++-------
>  3 files changed, 17 insertions(+), 13 deletions(-)
>

[...]

[PATCH v8 bpf-next 04/13] bpf: Add elem_id pointer as argument to __btf_resolve_size

[Jiri Olsa]

If the resolved type is array, make btf_resolve_size return also
ID of the elem type. It will be needed in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a68dee875a77..d9a723a8c040 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1079,6 +1079,7 @@ static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
  * *type_size: (x * y * sizeof(u32)).  Hence, *type_size always
  *             corresponds to the return type.
  * *elem_type: u32
+ * *elem_id: id of u32
  * *total_nelems: (x * y).  Hence, individual elem size is
  *                (*type_size / *total_nelems)
  *
@@ -1086,15 +1087,16 @@ static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
  * return type: type "struct X"
  * *type_size: sizeof(struct X)
  * *elem_type: same as return type ("struct X")
+ * *elem_id: 0
  * *total_nelems: 1
  */
 static const struct btf_type *
 __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		   u32 *type_size, const struct btf_type **elem_type,
-		   u32 *total_nelems)
+		   u32 *elem_id, u32 *total_nelems)
 {
 	const struct btf_type *array_type = NULL;
-	const struct btf_array *array;
+	const struct btf_array *array = NULL;
 	u32 i, size, nelems = 1;
 
 	for (i = 0; i < MAX_RESOLVE_DEPTH; i++) {
@@ -1146,6 +1148,8 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		*total_nelems = nelems;
 	if (elem_type)
 		*elem_type = type;
+	if (elem_id)
+		*elem_id = array ? array->type : 0;
 
 	return array_type ? : type;
 }
@@ -3971,7 +3975,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
 
 		mtype = __btf_resolve_size(btf_vmlinux, mtype, &msize,
-					   &elem_type, &total_nelems);
+					   &elem_type, NULL, &total_nelems);
 		if (IS_ERR(mtype)) {
 			bpf_log(log, "field %s doesn't have size\n", mname);
 			return -EFAULT;
-- 
2.25.4

Re: [PATCH v8 bpf-next 04/13] bpf: Add elem_id pointer as argument to __btf_resolve_size

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> If the resolved type is array, make btf_resolve_size return also
> ID of the elem type. It will be needed in following changes.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

it starts to feel like __btf_resolve_size does a bit too much, but
it's pretty contained, so I suppose it's ok

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  kernel/bpf/btf.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>

[...]

[PATCH v8 bpf-next 05/13] bpf: Add type_id pointer as argument to __btf_resolve_size

[Jiri Olsa]

Adding type_id pointer as argument to __btf_resolve_size
to return also BTF ID of the resolved type. It will be
used in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d9a723a8c040..841be6c49f11 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1082,6 +1082,7 @@ static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
  * *elem_id: id of u32
  * *total_nelems: (x * y).  Hence, individual elem size is
  *                (*type_size / *total_nelems)
+ * *type_id: id of type if it's changed within the function, 0 if not
  *
  * type: is not an array (e.g. const struct X)
  * return type: type "struct X"
@@ -1089,15 +1090,16 @@ static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
  * *elem_type: same as return type ("struct X")
  * *elem_id: 0
  * *total_nelems: 1
+ * *type_id: id of type if it's changed within the function, 0 if not
  */
 static const struct btf_type *
 __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		   u32 *type_size, const struct btf_type **elem_type,
-		   u32 *elem_id, u32 *total_nelems)
+		   u32 *elem_id, u32 *total_nelems, u32 *type_id)
 {
 	const struct btf_type *array_type = NULL;
 	const struct btf_array *array = NULL;
-	u32 i, size, nelems = 1;
+	u32 i, size, nelems = 1, id = 0;
 
 	for (i = 0; i < MAX_RESOLVE_DEPTH; i++) {
 		switch (BTF_INFO_KIND(type->info)) {
@@ -1118,6 +1120,7 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		case BTF_KIND_VOLATILE:
 		case BTF_KIND_CONST:
 		case BTF_KIND_RESTRICT:
+			id = type->type;
 			type = btf_type_by_id(btf, type->type);
 			break;
 
@@ -1150,6 +1153,8 @@ __btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		*elem_type = type;
 	if (elem_id)
 		*elem_id = array ? array->type : 0;
+	if (type_id && id)
+		*type_id = id;
 
 	return array_type ? : type;
 }
@@ -1158,7 +1163,7 @@ const struct btf_type *
 btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		 u32 *type_size)
 {
-	return __btf_resolve_size(btf, type, type_size, NULL, NULL);
+	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
 }
 
 /* The input param "type_id" must point to a needs_resolve type */
@@ -3975,7 +3980,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
 
 		mtype = __btf_resolve_size(btf_vmlinux, mtype, &msize,
-					   &elem_type, NULL, &total_nelems);
+					   &elem_type, NULL, &total_nelems, NULL);
 		if (IS_ERR(mtype)) {
 			bpf_log(log, "field %s doesn't have size\n", mname);
 			return -EFAULT;
-- 
2.25.4

[PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function

[Jiri Olsa]

Adding btf_struct_walk function that walks through the
struct type + given offset and returns following values:

  enum walk_return {
       /* < 0 error */
       walk_scalar = 0,
       walk_ptr,
       walk_struct,
  };

walk_scalar - when SCALAR_VALUE is found
walk_ptr    - when pointer value is found, its ID is stored
              in 'rid' output param
walk_struct - when nested struct object is found, its ID is stored
              in 'rid' output param

It will be used in following patches to get all nested
struct objects for given type and offset.

The btf_struct_access now calls btf_struct_walk function,
as long as it gets nested structs as return value.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 841be6c49f11..1ab5fd5bf992 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3873,16 +3873,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	return true;
 }
 
-int btf_struct_access(struct bpf_verifier_log *log,
-		      const struct btf_type *t, int off, int size,
-		      enum bpf_access_type atype,
-		      u32 *next_btf_id)
+enum walk_return {
+	/* < 0 error */
+	walk_scalar = 0,
+	walk_ptr,
+	walk_struct,
+};
+
+static int btf_struct_walk(struct bpf_verifier_log *log,
+			   const struct btf_type *t, int off, int size,
+			   u32 *rid)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
 	const struct btf_type *mtype, *elem_type = NULL;
 	const struct btf_member *member;
 	const char *tname, *mname;
-	u32 vlen;
+	u32 vlen, elem_id, mid;
 
 again:
 	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
@@ -3924,8 +3930,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			goto error;
 
 		off = (off - moff) % elem_type->size;
-		return btf_struct_access(log, elem_type, off, size, atype,
-					 next_btf_id);
+		return btf_struct_walk(log, elem_type, off, size, rid);
 
 error:
 		bpf_log(log, "access beyond struct %s at off %u size %u\n",
@@ -3954,7 +3959,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			 */
 			if (off <= moff &&
 			    BITS_ROUNDUP_BYTES(end_bit) <= off + size)
-				return SCALAR_VALUE;
+				return walk_scalar;
 
 			/* off may be accessing a following member
 			 *
@@ -3976,11 +3981,13 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			break;
 
 		/* type of the field */
+		mid = member->type;
 		mtype = btf_type_by_id(btf_vmlinux, member->type);
 		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
 
 		mtype = __btf_resolve_size(btf_vmlinux, mtype, &msize,
-					   &elem_type, NULL, &total_nelems, NULL);
+					   &elem_type, &elem_id, &total_nelems,
+					   &mid);
 		if (IS_ERR(mtype)) {
 			bpf_log(log, "field %s doesn't have size\n", mname);
 			return -EFAULT;
@@ -4042,6 +4049,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			elem_idx = (off - moff) / msize;
 			moff += elem_idx * msize;
 			mtype = elem_type;
+			mid = elem_id;
 		}
 
 		/* the 'off' we're looking for is either equal to start
@@ -4051,6 +4059,12 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			/* our field must be inside that union or struct */
 			t = mtype;
 
+			/* return if the offset matches the member offset */
+			if (off == moff) {
+				*rid = mid;
+				return walk_struct;
+			}
+
 			/* adjust offset we're looking for */
 			off -= moff;
 			goto again;
@@ -4066,11 +4080,10 @@ int btf_struct_access(struct bpf_verifier_log *log,
 					mname, moff, tname, off, size);
 				return -EACCES;
 			}
-
 			stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
 			if (btf_type_is_struct(stype)) {
-				*next_btf_id = id;
-				return PTR_TO_BTF_ID;
+				*rid = id;
+				return walk_ptr;
 			}
 		}
 
@@ -4087,12 +4100,46 @@ int btf_struct_access(struct bpf_verifier_log *log,
 			return -EACCES;
 		}
 
-		return SCALAR_VALUE;
+		return walk_scalar;
 	}
 	bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
 	return -EINVAL;
 }
 
+int btf_struct_access(struct bpf_verifier_log *log,
+		      const struct btf_type *t, int off, int size,
+		      enum bpf_access_type atype __maybe_unused,
+		      u32 *next_btf_id)
+{
+	int err;
+	u32 id;
+
+	do {
+		err = btf_struct_walk(log, t, off, size, &id);
+		if (err < 0)
+			return err;
+
+		/* We found the pointer or scalar on t+off,
+		 * we're done.
+		 */
+		if (err == walk_ptr) {
+			*next_btf_id = id;
+			return PTR_TO_BTF_ID;
+		}
+		if (err == walk_scalar)
+			return SCALAR_VALUE;
+
+		/* We found nested struct, so continue the search
+		 * by diving in it. At this point the offset is
+		 * aligned with the new type, so set it to 0.
+		 */
+		t = btf_type_by_id(btf_vmlinux, id);
+		off = 0;
+	} while (t);
+
+	return -EINVAL;
+}
+
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int arg)
 {
-- 
2.25.4

Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding btf_struct_walk function that walks through the
> struct type + given offset and returns following values:
>
>   enum walk_return {
>        /* < 0 error */
>        walk_scalar = 0,
>        walk_ptr,
>        walk_struct,
>   };
>
> walk_scalar - when SCALAR_VALUE is found
> walk_ptr    - when pointer value is found, its ID is stored
>               in 'rid' output param
> walk_struct - when nested struct object is found, its ID is stored
>               in 'rid' output param
>
> It will be used in following patches to get all nested
> struct objects for given type and offset.
>
> The btf_struct_access now calls btf_struct_walk function,
> as long as it gets nested structs as return value.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

It actually worked out to a pretty minimal changes to
btf_struct_access, I'm pleasantly surprised :))

Logic seems correct, just have few nits about naming and a bit more
safe handling in btf_struct_access, see below.


>  kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 60 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 841be6c49f11..1ab5fd5bf992 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3873,16 +3873,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>         return true;
>  }
>
> -int btf_struct_access(struct bpf_verifier_log *log,
> -                     const struct btf_type *t, int off, int size,
> -                     enum bpf_access_type atype,
> -                     u32 *next_btf_id)
> +enum walk_return {
> +       /* < 0 error */
> +       walk_scalar = 0,
> +       walk_ptr,
> +       walk_struct,
> +};

let's keep enum values in ALL_CAPS? walk_return is also a bit generic,
maybe something like bpf_struct_walk_result?

> +
> +static int btf_struct_walk(struct bpf_verifier_log *log,
> +                          const struct btf_type *t, int off, int size,
> +                          u32 *rid)
>  {
>         u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
>         const struct btf_type *mtype, *elem_type = NULL;
>         const struct btf_member *member;
>         const char *tname, *mname;
> -       u32 vlen;
> +       u32 vlen, elem_id, mid;
>
>  again:
>         tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> @@ -3924,8 +3930,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                         goto error;
>
>                 off = (off - moff) % elem_type->size;
> -               return btf_struct_access(log, elem_type, off, size, atype,
> -                                        next_btf_id);
> +               return btf_struct_walk(log, elem_type, off, size, rid);

oh, btw, this is a recursion in the kernel, let's fix that? I think it
could easily be just `goto again` here?
>
>  error:
>                 bpf_log(log, "access beyond struct %s at off %u size %u\n",
> @@ -3954,7 +3959,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                          */
>                         if (off <= moff &&
>                             BITS_ROUNDUP_BYTES(end_bit) <= off + size)
> -                               return SCALAR_VALUE;
> +                               return walk_scalar;
>
>                         /* off may be accessing a following member
>                          *

[...]

> @@ -4066,11 +4080,10 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                                         mname, moff, tname, off, size);
>                                 return -EACCES;
>                         }
> -
>                         stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
>                         if (btf_type_is_struct(stype)) {
> -                               *next_btf_id = id;
> -                               return PTR_TO_BTF_ID;
> +                               *rid = id;

nit: rid is a very opaque name, I find next_btf_id more appropriate
(even if it's meaning changes depending on walk_ptr vs walk_struct.

> +                               return walk_ptr;
>                         }
>                 }
>
> @@ -4087,12 +4100,46 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                         return -EACCES;
>                 }
>
> -               return SCALAR_VALUE;
> +               return walk_scalar;
>         }
>         bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
>         return -EINVAL;
>  }
>
> +int btf_struct_access(struct bpf_verifier_log *log,
> +                     const struct btf_type *t, int off, int size,
> +                     enum bpf_access_type atype __maybe_unused,
> +                     u32 *next_btf_id)
> +{
> +       int err;
> +       u32 id;
> +
> +       do {
> +               err = btf_struct_walk(log, t, off, size, &id);
> +               if (err < 0)
> +                       return err;
> +
> +               /* We found the pointer or scalar on t+off,
> +                * we're done.
> +                */
> +               if (err == walk_ptr) {
> +                       *next_btf_id = id;
> +                       return PTR_TO_BTF_ID;
> +               }
> +               if (err == walk_scalar)
> +                       return SCALAR_VALUE;
> +
> +               /* We found nested struct, so continue the search
> +                * by diving in it. At this point the offset is
> +                * aligned with the new type, so set it to 0.
> +                */
> +               t = btf_type_by_id(btf_vmlinux, id);
> +               off = 0;

It's very easy to miss that this case corresponds to walk_struct here.
If someone in the future adds a 4th special value, it will be too easy
to forget to update this piece of logic. So when dealing with enums, I
generally prefer this approach:

switch (err) {
case walk_ptr:
    ...
case walk_scalar:
    ...
case walk_struct:
    ...
default: /* complain loudly here */
}

WDYT?

> +       } while (t);
> +
> +       return -EINVAL;
> +}
> +
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int arg)
>  {
> --
> 2.25.4
>

Re: [PATCH v8 bpf-next 06/13] bpf: Factor btf_struct_access function

[Jiri Olsa]

On Tue, Jul 28, 2020 at 04:27:21PM -0700, Andrii Nakryiko wrote:

SNIP

> 
> >  kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 60 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 841be6c49f11..1ab5fd5bf992 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3873,16 +3873,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >         return true;
> >  }
> >
> > -int btf_struct_access(struct bpf_verifier_log *log,
> > -                     const struct btf_type *t, int off, int size,
> > -                     enum bpf_access_type atype,
> > -                     u32 *next_btf_id)
> > +enum walk_return {
> > +       /* < 0 error */
> > +       walk_scalar = 0,
> > +       walk_ptr,
> > +       walk_struct,
> > +};
> 
> let's keep enum values in ALL_CAPS? walk_return is also a bit generic,
> maybe something like bpf_struct_walk_result?

ok

> 
> > +
> > +static int btf_struct_walk(struct bpf_verifier_log *log,
> > +                          const struct btf_type *t, int off, int size,
> > +                          u32 *rid)
> >  {
> >         u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
> >         const struct btf_type *mtype, *elem_type = NULL;
> >         const struct btf_member *member;
> >         const char *tname, *mname;
> > -       u32 vlen;
> > +       u32 vlen, elem_id, mid;
> >
> >  again:
> >         tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> > @@ -3924,8 +3930,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
> >                         goto error;
> >
> >                 off = (off - moff) % elem_type->size;
> > -               return btf_struct_access(log, elem_type, off, size, atype,
> > -                                        next_btf_id);
> > +               return btf_struct_walk(log, elem_type, off, size, rid);
> 
> oh, btw, this is a recursion in the kernel, let's fix that? I think it
> could easily be just `goto again` here?

probably, I'll put it into separate change then

SNIP

> 
> > @@ -4066,11 +4080,10 @@ int btf_struct_access(struct bpf_verifier_log *log,
> >                                         mname, moff, tname, off, size);
> >                                 return -EACCES;
> >                         }
> > -
> >                         stype = btf_type_skip_modifiers(btf_vmlinux, mtype->type, &id);
> >                         if (btf_type_is_struct(stype)) {
> > -                               *next_btf_id = id;
> > -                               return PTR_TO_BTF_ID;
> > +                               *rid = id;
> 
> nit: rid is a very opaque name, I find next_btf_id more appropriate
> (even if it's meaning changes depending on walk_ptr vs walk_struct.

ok, will change

SNIP

> > +int btf_struct_access(struct bpf_verifier_log *log,
> > +                     const struct btf_type *t, int off, int size,
> > +                     enum bpf_access_type atype __maybe_unused,
> > +                     u32 *next_btf_id)
> > +{
> > +       int err;
> > +       u32 id;
> > +
> > +       do {
> > +               err = btf_struct_walk(log, t, off, size, &id);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               /* We found the pointer or scalar on t+off,
> > +                * we're done.
> > +                */
> > +               if (err == walk_ptr) {
> > +                       *next_btf_id = id;
> > +                       return PTR_TO_BTF_ID;
> > +               }
> > +               if (err == walk_scalar)
> > +                       return SCALAR_VALUE;
> > +
> > +               /* We found nested struct, so continue the search
> > +                * by diving in it. At this point the offset is
> > +                * aligned with the new type, so set it to 0.
> > +                */
> > +               t = btf_type_by_id(btf_vmlinux, id);
> > +               off = 0;
> 
> It's very easy to miss that this case corresponds to walk_struct here.
> If someone in the future adds a 4th special value, it will be too easy
> to forget to update this piece of logic. So when dealing with enums, I
> generally prefer this approach:
> 
> switch (err) {
> case walk_ptr:
>     ...
> case walk_scalar:
>     ...
> case walk_struct:
>     ...
> default: /* complain loudly here */
> }
> 
> WDYT?

right, I like it, make sense for future.. will change

thanks,
jirka

[PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function

[Jiri Olsa]

Adding btf_struct_ids_match function to check if given address provided
by BTF object + offset is also address of another nested BTF object.

This allows to pass an argument to helper, which is defined via parent
BTF object + offset, like for bpf_d_path (added in following changes):

  SEC("fentry/filp_close")
  int BPF_PROG(prog_close, struct file *file, void *id)
  {
    ...
    ret = bpf_d_path(&file->f_path, ...

The first bpf_d_path argument is hold by verifier as BTF file object
plus offset of f_path member.

The btf_struct_ids_match function will walk the struct file object and
check if there's nested struct path object on the given offset.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h   |  2 ++
 kernel/bpf/btf.c      | 29 +++++++++++++++++++++++++++++
 kernel/bpf/verifier.c | 18 ++++++++++++------
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bae557ff2da8..c981e258fed3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
 		      enum bpf_access_type atype,
 		      u32 *next_btf_id);
+bool btf_struct_ids_match(struct bpf_verifier_log *log,
+			  int off, u32 id, u32 mid);
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int);
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 1ab5fd5bf992..562d4453fad3 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log,
 	return -EINVAL;
 }
 
+bool btf_struct_ids_match(struct bpf_verifier_log *log,
+			  int off, u32 id, u32 mid)
+{
+	const struct btf_type *type;
+	u32 nid;
+	int err;
+
+	do {
+		type = btf_type_by_id(btf_vmlinux, id);
+		if (!type)
+			return false;
+		err = btf_struct_walk(log, type, off, 1, &nid);
+		if (err < 0)
+			return false;
+
+		/* We found nested struct object. If it matches
+		 * the requested ID, we're done. Otherwise let's
+		 * continue the search with offset 0 in the new
+		 * type.
+		 */
+		if (err == walk_struct && mid == nid)
+			return true;
+		off = 0;
+		id = nid;
+	} while (err == walk_struct);
+
+	return false;
+}
+
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int arg)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9a6703bc3f36..39922fa07154 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3887,16 +3887,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 				goto err_type;
 		}
 	} else if (arg_type == ARG_PTR_TO_BTF_ID) {
+		bool ids_match = false;
+
 		expected_type = PTR_TO_BTF_ID;
 		if (type != expected_type)
 			goto err_type;
 		if (!fn->check_btf_id) {
 			if (reg->btf_id != meta->btf_id) {
-				verbose(env, "Helper has type %s got %s in R%d\n",
-					kernel_type_name(meta->btf_id),
-					kernel_type_name(reg->btf_id), regno);
-
-				return -EACCES;
+				ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
+								 meta->btf_id);
+				if (!ids_match) {
+					verbose(env, "Helper has type %s got %s in R%d\n",
+						kernel_type_name(meta->btf_id),
+						kernel_type_name(reg->btf_id), regno);
+					return -EACCES;
+				}
 			}
 		} else if (!fn->check_btf_id(reg->btf_id, arg)) {
 			verbose(env, "Helper does not support %s in R%d\n",
@@ -3904,7 +3909,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 
 			return -EACCES;
 		}
-		if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) {
+		if (!ids_match &&
+		    (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) {
 			verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
 				regno);
 			return -EACCES;
-- 
2.25.4

Re: [PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding btf_struct_ids_match function to check if given address provided
> by BTF object + offset is also address of another nested BTF object.
>
> This allows to pass an argument to helper, which is defined via parent
> BTF object + offset, like for bpf_d_path (added in following changes):
>
>   SEC("fentry/filp_close")
>   int BPF_PROG(prog_close, struct file *file, void *id)
>   {
>     ...
>     ret = bpf_d_path(&file->f_path, ...
>
> The first bpf_d_path argument is hold by verifier as BTF file object
> plus offset of f_path member.
>
> The btf_struct_ids_match function will walk the struct file object and
> check if there's nested struct path object on the given offset.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h   |  2 ++
>  kernel/bpf/btf.c      | 29 +++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c | 18 ++++++++++++------
>  3 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index bae557ff2da8..c981e258fed3 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
>                       const struct btf_type *t, int off, int size,
>                       enum bpf_access_type atype,
>                       u32 *next_btf_id);
> +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> +                         int off, u32 id, u32 mid);
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int);
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 1ab5fd5bf992..562d4453fad3 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log,
>         return -EINVAL;
>  }
>
> +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> +                         int off, u32 id, u32 mid)
> +{
> +       const struct btf_type *type;
> +       u32 nid;
> +       int err;
> +

mid and nid are terrible names, especially as an input argument name.
mid == need_type_id? nid == cur_type_id or something along those
lines?

> +       do {
> +               type = btf_type_by_id(btf_vmlinux, id);
> +               if (!type)
> +                       return false;
> +               err = btf_struct_walk(log, type, off, 1, &nid);
> +               if (err < 0)
> +                       return false;
> +
> +               /* We found nested struct object. If it matches
> +                * the requested ID, we're done. Otherwise let's
> +                * continue the search with offset 0 in the new
> +                * type.
> +                */
> +               if (err == walk_struct && mid == nid)
> +                       return true;
> +               off = 0;
> +               id = nid;
> +       } while (err == walk_struct);

This seems like a slightly more obvious control flow:

again:

   ...

   if (err != walk_struct)
      return false;

   if (mid != nid) {
      off = 0;
      id = nid;
      goto again;
   }

   return true;

> +
> +       return false;
> +}
> +
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>                           const struct bpf_func_proto *fn, int arg)
>  {

[...]

Re: [PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function

[Jiri Olsa]

On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote:

SNIP

> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index bae557ff2da8..c981e258fed3 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
> >                       const struct btf_type *t, int off, int size,
> >                       enum bpf_access_type atype,
> >                       u32 *next_btf_id);
> > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > +                         int off, u32 id, u32 mid);
> >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> >                           const struct bpf_func_proto *fn, int);
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 1ab5fd5bf992..562d4453fad3 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log,
> >         return -EINVAL;
> >  }
> >
> > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > +                         int off, u32 id, u32 mid)
> > +{
> > +       const struct btf_type *type;
> > +       u32 nid;
> > +       int err;
> > +
> 
> mid and nid are terrible names, especially as an input argument name.
> mid == need_type_id? nid == cur_type_id or something along those
> lines?

'mid' was for matching id, 'nid' for nested id ;-)
need_type_id/cur_type_id sound good

> 
> > +       do {
> > +               type = btf_type_by_id(btf_vmlinux, id);
> > +               if (!type)
> > +                       return false;
> > +               err = btf_struct_walk(log, type, off, 1, &nid);
> > +               if (err < 0)
> > +                       return false;
> > +
> > +               /* We found nested struct object. If it matches
> > +                * the requested ID, we're done. Otherwise let's
> > +                * continue the search with offset 0 in the new
> > +                * type.
> > +                */
> > +               if (err == walk_struct && mid == nid)
> > +                       return true;
> > +               off = 0;
> > +               id = nid;
> > +       } while (err == walk_struct);
> 
> This seems like a slightly more obvious control flow:
> 
> again:
> 
>    ...
> 
>    if (err != walk_struct)
>       return false;

ok, and perhaps use in here the switch(err) as in the previous patch?

thanks,
jirka

> 
>    if (mid != nid) {
>       off = 0;
>       id = nid;
>       goto again;
>    }
> 
>    return true;
> 
> > +
> > +       return false;
> > +}
> > +
> >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> >                           const struct bpf_func_proto *fn, int arg)
> >  {
> 
> [...]
> 

Re: [PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function

[Andrii Nakryiko]

On Wed, Jul 29, 2020 at 9:04 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index bae557ff2da8..c981e258fed3 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > >                       const struct btf_type *t, int off, int size,
> > >                       enum bpf_access_type atype,
> > >                       u32 *next_btf_id);
> > > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > > +                         int off, u32 id, u32 mid);
> > >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > >                           const struct bpf_func_proto *fn, int);
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 1ab5fd5bf992..562d4453fad3 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > >         return -EINVAL;
> > >  }
> > >
> > > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > > +                         int off, u32 id, u32 mid)

just realized that if id == mid and off == 0, btf_struct_ids_match()
will return false. Right now verifier is careful to not call
btf_struct_ids_match in such case, but I wonder if it's better to make
that (common) case also work?

> > > +{
> > > +       const struct btf_type *type;
> > > +       u32 nid;
> > > +       int err;
> > > +
> >
> > mid and nid are terrible names, especially as an input argument name.
> > mid == need_type_id? nid == cur_type_id or something along those
> > lines?
>
> 'mid' was for matching id, 'nid' for nested id ;-)
> need_type_id/cur_type_id sound good

nested I guessed, mid was a mystery to me :))

>
> >
> > > +       do {
> > > +               type = btf_type_by_id(btf_vmlinux, id);
> > > +               if (!type)
> > > +                       return false;
> > > +               err = btf_struct_walk(log, type, off, 1, &nid);
> > > +               if (err < 0)
> > > +                       return false;
> > > +
> > > +               /* We found nested struct object. If it matches
> > > +                * the requested ID, we're done. Otherwise let's
> > > +                * continue the search with offset 0 in the new
> > > +                * type.
> > > +                */
> > > +               if (err == walk_struct && mid == nid)
> > > +                       return true;
> > > +               off = 0;
> > > +               id = nid;
> > > +       } while (err == walk_struct);
> >
> > This seems like a slightly more obvious control flow:
> >
> > again:
> >
> >    ...
> >
> >    if (err != walk_struct)
> >       return false;
>
> ok, and perhaps use in here the switch(err) as in the previous patch?

I think straightforward if is better than switch here, because
anything but walk_struct is not what we expect.

>
> thanks,
> jirka
>
> >
> >    if (mid != nid) {
> >       off = 0;
> >       id = nid;
> >       goto again;
> >    }
> >
> >    return true;
> >
> > > +
> > > +       return false;
> > > +}
> > > +
> > >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > >                           const struct bpf_func_proto *fn, int arg)
> > >  {
> >
> > [...]
> >
>

Re: [PATCH v8 bpf-next 07/13] bpf: Add btf_struct_ids_match function

[Jiri Olsa]

On Wed, Jul 29, 2020 at 10:51:26AM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 29, 2020 at 9:04 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Tue, Jul 28, 2020 at 04:35:16PM -0700, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index bae557ff2da8..c981e258fed3 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -1306,6 +1306,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > > >                       const struct btf_type *t, int off, int size,
> > > >                       enum bpf_access_type atype,
> > > >                       u32 *next_btf_id);
> > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > > > +                         int off, u32 id, u32 mid);
> > > >  int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > > >                           const struct bpf_func_proto *fn, int);
> > > >
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 1ab5fd5bf992..562d4453fad3 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -4140,6 +4140,35 @@ int btf_struct_access(struct bpf_verifier_log *log,
> > > >         return -EINVAL;
> > > >  }
> > > >
> > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log,
> > > > +                         int off, u32 id, u32 mid)
> 
> just realized that if id == mid and off == 0, btf_struct_ids_match()
> will return false. Right now verifier is careful to not call
> btf_struct_ids_match in such case, but I wonder if it's better to make
> that (common) case also work?

right, also we should call btf_struct_ids_match when
IDs are equal and off != 0, which we don't do now

jirka

[PATCH v8 bpf-next 08/13] bpf: Add BTF_SET_START/END macros

[Jiri Olsa]

Adding support to define sorted set of BTF ID values.

Following defines sorted set of BTF ID values:

  BTF_SET_START(btf_whitelist_d_path)
  BTF_ID(func, vfs_truncate)
  BTF_ID(func, vfs_fallocate)
  BTF_ID(func, dentry_open)
  BTF_ID(func, vfs_getattr)
  BTF_ID(func, filp_close)
  BTF_SET_END(btf_whitelist_d_path)

It defines following 'struct btf_id_set' variable to access
values and count:

  struct btf_id_set btf_whitelist_d_path;

Adding 'allowed' callback to struct bpf_func_proto, to allow
verifier the check on allowed callers.

Adding btf_id_set_contains function, which will be used by
allowed callbacks to verify the caller's BTF ID value is
within allowed set.

Also removing extra '\' in __BTF_ID_LIST macro.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h           |  4 ++++
 include/linux/btf_ids.h       | 43 ++++++++++++++++++++++++++++++++++-
 kernel/bpf/btf.c              | 14 ++++++++++++
 kernel/bpf/verifier.c         |  5 ++++
 tools/include/linux/btf_ids.h | 43 ++++++++++++++++++++++++++++++++++-
 5 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c981e258fed3..e5d81c31ed8e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -295,6 +295,7 @@ struct bpf_func_proto {
 						    * for this argument.
 						    */
 	int *ret_btf_id; /* return value btf_id */
+	bool (*allowed)(const struct bpf_prog *prog);
 };
 
 /* bpf_context is intentionally undefined structure. Pointer to bpf_context is
@@ -1787,4 +1788,7 @@ enum bpf_text_poke_type {
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		       void *addr1, void *addr2);
 
+struct btf_id_set;
+bool btf_id_set_contains(struct btf_id_set *set, u32 id);
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 4867d549e3c1..840c3e63cc13 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -3,6 +3,11 @@
 #ifndef _LINUX_BTF_IDS_H
 #define _LINUX_BTF_IDS_H
 
+struct btf_id_set {
+	u32 cnt;
+	u32 ids[];
+};
+
 #ifdef CONFIG_DEBUG_INFO_BTF
 
 #include <linux/compiler.h> /* for __PASTE */
@@ -62,7 +67,7 @@ asm(							\
 ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
 "." #scope " " #name ";                        \n"	\
 #name ":;                                      \n"	\
-".popsection;                                  \n");	\
+".popsection;                                  \n");
 
 #define BTF_ID_LIST(name)				\
 __BTF_ID_LIST(name, local)				\
@@ -88,12 +93,48 @@ asm(							\
 ".zero 4                                       \n"	\
 ".popsection;                                  \n");
 
+/*
+ * The BTF_SET_START/END macros pair defines sorted list of
+ * BTF IDs plus its members count, with following layout:
+ *
+ * BTF_SET_START(list)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ * BTF_SET_END(list)
+ *
+ * __BTF_ID__set__list:
+ * .zero 4
+ * list:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * __BTF_ID__type2__name2__4:
+ * .zero 4
+ *
+ */
+#define BTF_SET_START(name)				\
+__BTF_ID_LIST(name, local)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local __BTF_ID__set__" #name ";              \n"	\
+"__BTF_ID__set__" #name ":;                    \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");
+
+#define BTF_SET_END(name)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";      \n"	\
+".size __BTF_ID__set__" #name ", .-" #name "  \n"	\
+".popsection;                                 \n");	\
+extern struct btf_id_set name;
+
 #else
 
 #define BTF_ID_LIST(name) static u32 name[5];
 #define BTF_ID(prefix, name)
 #define BTF_ID_UNUSED
 #define BTF_ID_LIST_GLOBAL(name) u32 name[1];
+#define BTF_SET_START(name) static struct btf_id_set name = { 0 };
+#define BTF_SET_END(name)
 
 #endif /* CONFIG_DEBUG_INFO_BTF */
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 562d4453fad3..06714cdda0a9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -21,6 +21,8 @@
 #include <linux/btf_ids.h>
 #include <linux/skmsg.h>
 #include <linux/perf_event.h>
+#include <linux/bsearch.h>
+#include <linux/btf_ids.h>
 #include <net/sock.h>
 
 /* BTF (BPF Type Format) is the meta data format which describes
@@ -4740,3 +4742,15 @@ u32 btf_id(const struct btf *btf)
 {
 	return btf->id;
 }
+
+static int btf_id_cmp_func(const void *a, const void *b)
+{
+	const int *pa = a, *pb = b;
+
+	return *pa - *pb;
+}
+
+bool btf_id_set_contains(struct btf_id_set *set, u32 id)
+{
+	return bsearch(&id, set->ids, set->cnt, sizeof(int), btf_id_cmp_func) != NULL;
+}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 39922fa07154..49f728c696a9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4706,6 +4706,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
 		return -EINVAL;
 	}
 
+	if (fn->allowed && !fn->allowed(env->prog)) {
+		verbose(env, "helper call is not allowed in probe\n");
+		return -EINVAL;
+	}
+
 	/* With LD_ABS/IND some JITs save/restore skb from r1. */
 	changes_data = bpf_helper_changes_pkt_data(fn->func);
 	if (changes_data && fn->arg1_type != ARG_PTR_TO_CTX) {
diff --git a/tools/include/linux/btf_ids.h b/tools/include/linux/btf_ids.h
index 4867d549e3c1..840c3e63cc13 100644
--- a/tools/include/linux/btf_ids.h
+++ b/tools/include/linux/btf_ids.h
@@ -3,6 +3,11 @@
 #ifndef _LINUX_BTF_IDS_H
 #define _LINUX_BTF_IDS_H
 
+struct btf_id_set {
+	u32 cnt;
+	u32 ids[];
+};
+
 #ifdef CONFIG_DEBUG_INFO_BTF
 
 #include <linux/compiler.h> /* for __PASTE */
@@ -62,7 +67,7 @@ asm(							\
 ".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
 "." #scope " " #name ";                        \n"	\
 #name ":;                                      \n"	\
-".popsection;                                  \n");	\
+".popsection;                                  \n");
 
 #define BTF_ID_LIST(name)				\
 __BTF_ID_LIST(name, local)				\
@@ -88,12 +93,48 @@ asm(							\
 ".zero 4                                       \n"	\
 ".popsection;                                  \n");
 
+/*
+ * The BTF_SET_START/END macros pair defines sorted list of
+ * BTF IDs plus its members count, with following layout:
+ *
+ * BTF_SET_START(list)
+ * BTF_ID(type1, name1)
+ * BTF_ID(type2, name2)
+ * BTF_SET_END(list)
+ *
+ * __BTF_ID__set__list:
+ * .zero 4
+ * list:
+ * __BTF_ID__type1__name1__3:
+ * .zero 4
+ * __BTF_ID__type2__name2__4:
+ * .zero 4
+ *
+ */
+#define BTF_SET_START(name)				\
+__BTF_ID_LIST(name, local)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";       \n"	\
+".local __BTF_ID__set__" #name ";              \n"	\
+"__BTF_ID__set__" #name ":;                    \n"	\
+".zero 4                                       \n"	\
+".popsection;                                  \n");
+
+#define BTF_SET_END(name)				\
+asm(							\
+".pushsection " BTF_IDS_SECTION ",\"a\";      \n"	\
+".size __BTF_ID__set__" #name ", .-" #name "  \n"	\
+".popsection;                                 \n");	\
+extern struct btf_id_set name;
+
 #else
 
 #define BTF_ID_LIST(name) static u32 name[5];
 #define BTF_ID(prefix, name)
 #define BTF_ID_UNUSED
 #define BTF_ID_LIST_GLOBAL(name) u32 name[1];
+#define BTF_SET_START(name) static struct btf_id_set name = { 0 };
+#define BTF_SET_END(name)
 
 #endif /* CONFIG_DEBUG_INFO_BTF */
 
-- 
2.25.4

Re: [PATCH v8 bpf-next 08/13] bpf: Add BTF_SET_START/END macros

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:13 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to define sorted set of BTF ID values.
>
> Following defines sorted set of BTF ID values:
>
>   BTF_SET_START(btf_whitelist_d_path)
>   BTF_ID(func, vfs_truncate)
>   BTF_ID(func, vfs_fallocate)
>   BTF_ID(func, dentry_open)
>   BTF_ID(func, vfs_getattr)
>   BTF_ID(func, filp_close)
>   BTF_SET_END(btf_whitelist_d_path)
>
> It defines following 'struct btf_id_set' variable to access
> values and count:
>
>   struct btf_id_set btf_whitelist_d_path;
>
> Adding 'allowed' callback to struct bpf_func_proto, to allow
> verifier the check on allowed callers.
>
> Adding btf_id_set_contains function, which will be used by
> allowed callbacks to verify the caller's BTF ID value is
> within allowed set.
>
> Also removing extra '\' in __BTF_ID_LIST macro.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h           |  4 ++++
>  include/linux/btf_ids.h       | 43 ++++++++++++++++++++++++++++++++++-
>  kernel/bpf/btf.c              | 14 ++++++++++++
>  kernel/bpf/verifier.c         |  5 ++++
>  tools/include/linux/btf_ids.h | 43 ++++++++++++++++++++++++++++++++++-
>  5 files changed, 107 insertions(+), 2 deletions(-)
>

[...]

> +#define BTF_SET_START(name)                            \
> +__BTF_ID_LIST(name, local)                             \
> +asm(                                                   \
> +".pushsection " BTF_IDS_SECTION ",\"a\";       \n"     \
> +".local __BTF_ID__set__" #name ";              \n"     \
> +"__BTF_ID__set__" #name ":;                    \n"     \
> +".zero 4                                       \n"     \
> +".popsection;                                  \n");
> +
> +#define BTF_SET_END(name)                              \
> +asm(                                                   \
> +".pushsection " BTF_IDS_SECTION ",\"a\";      \n"      \
> +".size __BTF_ID__set__" #name ", .-" #name "  \n"      \
> +".popsection;                                 \n");    \
> +extern struct btf_id_set name;
> +
>  #else

This local symbol assumption will probably at some point bite us.
Yonghong already did global vs static variants for BTF ID list, we'll
end up doing something like that for sets of BTF IDs as well. Let's do
this similarly from the get go.

>
>  #define BTF_ID_LIST(name) static u32 name[5];
>  #define BTF_ID(prefix, name)
>  #define BTF_ID_UNUSED
>  #define BTF_ID_LIST_GLOBAL(name) u32 name[1];
> +#define BTF_SET_START(name) static struct btf_id_set name = { 0 };

nit: this zero is unnecessary and misleading (it's initialized for
only the first member of a struct). Just {} is enough.

> +#define BTF_SET_END(name)
>
>  #endif /* CONFIG_DEBUG_INFO_BTF */
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 562d4453fad3..06714cdda0a9 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -21,6 +21,8 @@
>  #include <linux/btf_ids.h>
>  #include <linux/skmsg.h>
>  #include <linux/perf_event.h>
> +#include <linux/bsearch.h>
> +#include <linux/btf_ids.h>
>  #include <net/sock.h>
>
>  /* BTF (BPF Type Format) is the meta data format which describes
> @@ -4740,3 +4742,15 @@ u32 btf_id(const struct btf *btf)
>  {
>         return btf->id;
>  }
> +
> +static int btf_id_cmp_func(const void *a, const void *b)
> +{
> +       const int *pa = a, *pb = b;
> +
> +       return *pa - *pb;
> +}
> +
> +bool btf_id_set_contains(struct btf_id_set *set, u32 id)
> +{
> +       return bsearch(&id, set->ids, set->cnt, sizeof(int), btf_id_cmp_func) != NULL;

very nit ;) sizeof(__u32)

> +}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 39922fa07154..49f728c696a9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4706,6 +4706,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 return -EINVAL;
>         }
>

[...]

Re: [PATCH v8 bpf-next 08/13] bpf: Add BTF_SET_START/END macros

[Jiri Olsa]

On Tue, Jul 28, 2020 at 12:39:06PM -0700, Andrii Nakryiko wrote:

SNIP

> 
> [...]
> 
> > +#define BTF_SET_START(name)                            \
> > +__BTF_ID_LIST(name, local)                             \
> > +asm(                                                   \
> > +".pushsection " BTF_IDS_SECTION ",\"a\";       \n"     \
> > +".local __BTF_ID__set__" #name ";              \n"     \
> > +"__BTF_ID__set__" #name ":;                    \n"     \
> > +".zero 4                                       \n"     \
> > +".popsection;                                  \n");
> > +
> > +#define BTF_SET_END(name)                              \
> > +asm(                                                   \
> > +".pushsection " BTF_IDS_SECTION ",\"a\";      \n"      \
> > +".size __BTF_ID__set__" #name ", .-" #name "  \n"      \
> > +".popsection;                                 \n");    \
> > +extern struct btf_id_set name;
> > +
> >  #else
> 
> This local symbol assumption will probably at some point bite us.
> Yonghong already did global vs static variants for BTF ID list, we'll
> end up doing something like that for sets of BTF IDs as well. Let's do
> this similarly from the get go.

sure, will add that

> 
> >
> >  #define BTF_ID_LIST(name) static u32 name[5];
> >  #define BTF_ID(prefix, name)
> >  #define BTF_ID_UNUSED
> >  #define BTF_ID_LIST_GLOBAL(name) u32 name[1];
> > +#define BTF_SET_START(name) static struct btf_id_set name = { 0 };
> 
> nit: this zero is unnecessary and misleading (it's initialized for
> only the first member of a struct). Just {} is enough.

ok

> 
> > +#define BTF_SET_END(name)
> >
> >  #endif /* CONFIG_DEBUG_INFO_BTF */
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 562d4453fad3..06714cdda0a9 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/btf_ids.h>
> >  #include <linux/skmsg.h>
> >  #include <linux/perf_event.h>
> > +#include <linux/bsearch.h>
> > +#include <linux/btf_ids.h>
> >  #include <net/sock.h>
> >
> >  /* BTF (BPF Type Format) is the meta data format which describes
> > @@ -4740,3 +4742,15 @@ u32 btf_id(const struct btf *btf)
> >  {
> >         return btf->id;
> >  }
> > +
> > +static int btf_id_cmp_func(const void *a, const void *b)
> > +{
> > +       const int *pa = a, *pb = b;
> > +
> > +       return *pa - *pb;
> > +}
> > +
> > +bool btf_id_set_contains(struct btf_id_set *set, u32 id)
> > +{
> > +       return bsearch(&id, set->ids, set->cnt, sizeof(int), btf_id_cmp_func) != NULL;
> 
> very nit ;) sizeof(__u32)

sure ;-)

thanks,
jirka

[PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Jiri Olsa]

Adding d_path helper function that returns full path for
given 'struct path' object, which needs to be the kernel
BTF 'path' object. The path is returned in buffer provided
'buf' of size 'sz' and is zero terminated.

  bpf_d_path(&file->f_path, buf, size);

The helper calls directly d_path function, so there's only
limited set of function it can be called from. Adding just
very modest set for the start.

Updating also bpf.h tools uapi header and adding 'path' to
bpf_helpers_doc.py script.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/bpf.h       | 13 +++++++++
 kernel/trace/bpf_trace.c       | 48 ++++++++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 13 +++++++++
 4 files changed, 76 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 54d0c886e3ba..f8341219b5ea 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3377,6 +3377,18 @@ union bpf_attr {
  *		A non-negative value equal to or less than *size* on success,
  *		or a negative error in case of failure.
  *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz' and
+ *		is zero terminated.
+ *
+ *	Return
+ *		On success, the strictly positive length of the string,
+ *		including the trailing NUL character. On error, a negative
+ *		value.
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3521,6 +3533,7 @@ union bpf_attr {
 	FN(skc_to_tcp_request_sock),	\
 	FN(skc_to_udp6_sock),		\
 	FN(get_task_stack),		\
+	FN(d_path),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3cc0dcb60ca2..0488db62aaaf 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1098,6 +1098,52 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
+{
+	char *p = d_path(path, buf, sz - 1);
+	int len;
+
+	if (IS_ERR(p)) {
+		len = PTR_ERR(p);
+	} else {
+		len = strlen(p);
+		if (len && p != buf)
+			memmove(buf, p, len);
+		buf[len] = 0;
+		/* Include the trailing NUL. */
+		len++;
+	}
+
+	return len;
+}
+
+BTF_SET_START(btf_whitelist_d_path)
+BTF_ID(func, vfs_truncate)
+BTF_ID(func, vfs_fallocate)
+BTF_ID(func, dentry_open)
+BTF_ID(func, vfs_getattr)
+BTF_ID(func, filp_close)
+BTF_SET_END(btf_whitelist_d_path)
+
+static bool bpf_d_path_allowed(const struct bpf_prog *prog)
+{
+	return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
+}
+
+BTF_ID_LIST(bpf_d_path_btf_ids)
+BTF_ID(struct, path)
+
+static const struct bpf_func_proto bpf_d_path_proto = {
+	.func		= bpf_d_path,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_MEM,
+	.arg3_type	= ARG_CONST_SIZE,
+	.btf_id		= bpf_d_path_btf_ids,
+	.allowed	= bpf_d_path_allowed,
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1579,6 +1625,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return prog->expected_attach_type == BPF_TRACE_ITER ?
 		       &bpf_seq_write_proto :
 		       NULL;
+	case BPF_FUNC_d_path:
+		return &bpf_d_path_proto;
 	default:
 		return raw_tp_prog_func_proto(func_id, prog);
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 5bfa448b4704..08388173973f 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -432,6 +432,7 @@ class PrinterHelpers(Printer):
             'struct __sk_buff',
             'struct sk_msg_md',
             'struct xdp_md',
+            'struct path',
     ]
     known_types = {
             '...',
@@ -472,6 +473,7 @@ class PrinterHelpers(Printer):
             'struct tcp_request_sock',
             'struct udp6_sock',
             'struct task_struct',
+            'struct path',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 54d0c886e3ba..f8341219b5ea 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3377,6 +3377,18 @@ union bpf_attr {
  *		A non-negative value equal to or less than *size* on success,
  *		or a negative error in case of failure.
  *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz' and
+ *		is zero terminated.
+ *
+ *	Return
+ *		On success, the strictly positive length of the string,
+ *		including the trailing NUL character. On error, a negative
+ *		value.
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3521,6 +3533,7 @@ union bpf_attr {
 	FN(skc_to_tcp_request_sock),	\
 	FN(skc_to_udp6_sock),		\
 	FN(get_task_stack),		\
+	FN(d_path),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.25.4

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding d_path helper function that returns full path for
> given 'struct path' object, which needs to be the kernel
> BTF 'path' object. The path is returned in buffer provided
> 'buf' of size 'sz' and is zero terminated.
>
>   bpf_d_path(&file->f_path, buf, size);
>
> The helper calls directly d_path function, so there's only
> limited set of function it can be called from. Adding just
> very modest set for the start.
>
> Updating also bpf.h tools uapi header and adding 'path' to
> bpf_helpers_doc.py script.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/uapi/linux/bpf.h       | 13 +++++++++
>  kernel/trace/bpf_trace.c       | 48 ++++++++++++++++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  2 ++
>  tools/include/uapi/linux/bpf.h | 13 +++++++++
>  4 files changed, 76 insertions(+)
>

[...]

>
> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> +{
> +       char *p = d_path(path, buf, sz - 1);
> +       int len;
> +
> +       if (IS_ERR(p)) {
> +               len = PTR_ERR(p);
> +       } else {
> +               len = strlen(p);
> +               if (len && p != buf)
> +                       memmove(buf, p, len);

not sure if it's worth it, but if len == sz - 1 then memmove is not
necessary. Again, don't know if worth it, as it's probably not going
to be a common case.

> +               buf[len] = 0;
> +               /* Include the trailing NUL. */
> +               len++;
> +       }
> +
> +       return len;
> +}
> +
> +BTF_SET_START(btf_whitelist_d_path)
> +BTF_ID(func, vfs_truncate)
> +BTF_ID(func, vfs_fallocate)
> +BTF_ID(func, dentry_open)
> +BTF_ID(func, vfs_getattr)
> +BTF_ID(func, filp_close)
> +BTF_SET_END(btf_whitelist_d_path)


We should probably comply with an updated coding style ([0]) and use
an allowlist name for this?

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb

> +
> +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> +{
> +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
> +}
> +
> +BTF_ID_LIST(bpf_d_path_btf_ids)
> +BTF_ID(struct, path)
> +
> +static const struct bpf_func_proto bpf_d_path_proto = {
> +       .func           = bpf_d_path,
> +       .gpl_only       = false,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_BTF_ID,
> +       .arg2_type      = ARG_PTR_TO_MEM,
> +       .arg3_type      = ARG_CONST_SIZE,

I feel like we had a discussion about ARG_CONST_SIZE vs
ARG_CONST_SIZE_OR_ZERO before, maybe on some different thread.
Basically, this >0 restriction was a major nuisance for
bpf_perf_event_output() cases, so much that we changed it to _OR_ZERO.
In practice, while it might never be the case that we have sz == 0
passed into the function, having to prove this to the verifier is a
PITA. Unless there is a very strong reason not to, let's mark this as
ARG_CONST_SIZE_OR_ZERO and handle sz == 0 case as a noop?

> +       .btf_id         = bpf_d_path_btf_ids,
> +       .allowed        = bpf_d_path_allowed,
> +};
> +

[...]

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Jiri Olsa]

On Tue, Jul 28, 2020 at 12:47:03PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding d_path helper function that returns full path for
> > given 'struct path' object, which needs to be the kernel
> > BTF 'path' object. The path is returned in buffer provided
> > 'buf' of size 'sz' and is zero terminated.
> >
> >   bpf_d_path(&file->f_path, buf, size);
> >
> > The helper calls directly d_path function, so there's only
> > limited set of function it can be called from. Adding just
> > very modest set for the start.
> >
> > Updating also bpf.h tools uapi header and adding 'path' to
> > bpf_helpers_doc.py script.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/uapi/linux/bpf.h       | 13 +++++++++
> >  kernel/trace/bpf_trace.c       | 48 ++++++++++++++++++++++++++++++++++
> >  scripts/bpf_helpers_doc.py     |  2 ++
> >  tools/include/uapi/linux/bpf.h | 13 +++++++++
> >  4 files changed, 76 insertions(+)
> >
> 
> [...]
> 
> >
> > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > +{
> > +       char *p = d_path(path, buf, sz - 1);
> > +       int len;
> > +
> > +       if (IS_ERR(p)) {
> > +               len = PTR_ERR(p);
> > +       } else {
> > +               len = strlen(p);
> > +               if (len && p != buf)
> > +                       memmove(buf, p, len);
> 
> not sure if it's worth it, but if len == sz - 1 then memmove is not
> necessary. Again, don't know if worth it, as it's probably not going
> to be a common case.

I did not see condition like that for d_path/file_path usage,
I'll check if such return values are even possible

> 
> > +               buf[len] = 0;
> > +               /* Include the trailing NUL. */
> > +               len++;
> > +       }
> > +
> > +       return len;
> > +}
> > +
> > +BTF_SET_START(btf_whitelist_d_path)
> > +BTF_ID(func, vfs_truncate)
> > +BTF_ID(func, vfs_fallocate)
> > +BTF_ID(func, dentry_open)
> > +BTF_ID(func, vfs_getattr)
> > +BTF_ID(func, filp_close)
> > +BTF_SET_END(btf_whitelist_d_path)
> 
> 
> We should probably comply with an updated coding style ([0]) and use
> an allowlist name for this?
> 
>   [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb

right, will change

> 
> > +
> > +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
> > +{
> > +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
> > +}
> > +
> > +BTF_ID_LIST(bpf_d_path_btf_ids)
> > +BTF_ID(struct, path)
> > +
> > +static const struct bpf_func_proto bpf_d_path_proto = {
> > +       .func           = bpf_d_path,
> > +       .gpl_only       = false,
> > +       .ret_type       = RET_INTEGER,
> > +       .arg1_type      = ARG_PTR_TO_BTF_ID,
> > +       .arg2_type      = ARG_PTR_TO_MEM,
> > +       .arg3_type      = ARG_CONST_SIZE,
> 
> I feel like we had a discussion about ARG_CONST_SIZE vs
> ARG_CONST_SIZE_OR_ZERO before, maybe on some different thread.
> Basically, this >0 restriction was a major nuisance for
> bpf_perf_event_output() cases, so much that we changed it to _OR_ZERO.
> In practice, while it might never be the case that we have sz == 0
> passed into the function, having to prove this to the verifier is a
> PITA. Unless there is a very strong reason not to, let's mark this as
> ARG_CONST_SIZE_OR_ZERO and handle sz == 0 case as a noop?

sure, will change

thanks,
jirka

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Al Viro]

On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:

> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> +{
> +	char *p = d_path(path, buf, sz - 1);
> +	int len;
> +
> +	if (IS_ERR(p)) {
> +		len = PTR_ERR(p);
> +	} else {
> +		len = strlen(p);
> +		if (len && p != buf)
> +			memmove(buf, p, len);

*blink*
What the hell do you need that strlen() for?  d_path() copies into
the end of buffer (well, starts there and prepends to it); all you
really need is memmove(buf, p, buf + sz - p)


> +		buf[len] = 0;

Wait a minute...  Why are you NUL-terminating it separately?
You do rely upon having NUL in the damn thing (and d_path() does
guarantee it there).  Without that strlen() would've gone into
the nasal demon country; you can't call it on non-NUL-terminated
array.  So you are guaranteed that p[len] will be '\0'; why bother
copying the first len bytes and then separately deal with that
NUL?  Just memmove() the fucker and be done with that...

If you are worried about stray NUL in the middle of the returned
data... can't happen.  Note the rename_lock use in fs/d_path.c;
the names of everything involved are guaranteed to have been
stable throughout the copying them into the buffer - if anything
were to be renamed while we are doing that, we'd repeat the whole
thing (with rename_lock taken exclusive the second time around).

So make it simply
	if (IS_ERR(p))
		return PTR_ERR(p);
	len = buf + sz - p;
	memmove(buf, p, len);
	return len;
and be done with that.  BTW, the odds of p == buf are pretty much
nil - it would happen only if sz - 1 happened to be the exact length
of pathname.

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Jiri Olsa]

On Wed, Jul 29, 2020 at 09:11:17PM +0100, Al Viro wrote:
> On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:
> 
> > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > +{
> > +	char *p = d_path(path, buf, sz - 1);
> > +	int len;
> > +
> > +	if (IS_ERR(p)) {
> > +		len = PTR_ERR(p);
> > +	} else {
> > +		len = strlen(p);
> > +		if (len && p != buf)
> > +			memmove(buf, p, len);
> 
> *blink*
> What the hell do you need that strlen() for?  d_path() copies into
> the end of buffer (well, starts there and prepends to it); all you
> really need is memmove(buf, p, buf + sz - p)

I used the code from some of the other users like
  backing_dev_show
  fsg_show_file

nice, looks like we could omit strlen call in perf mmap event call as well

> 
> 
> > +		buf[len] = 0;
> 
> Wait a minute...  Why are you NUL-terminating it separately?
> You do rely upon having NUL in the damn thing (and d_path() does
> guarantee it there).  Without that strlen() would've gone into
> the nasal demon country; you can't call it on non-NUL-terminated
> array.  So you are guaranteed that p[len] will be '\0'; why bother
> copying the first len bytes and then separately deal with that
> NUL?  Just memmove() the fucker and be done with that...
> 
> If you are worried about stray NUL in the middle of the returned
> data... can't happen.  Note the rename_lock use in fs/d_path.c;
> the names of everything involved are guaranteed to have been
> stable throughout the copying them into the buffer - if anything
> were to be renamed while we are doing that, we'd repeat the whole
> thing (with rename_lock taken exclusive the second time around).
> 
> So make it simply
> 	if (IS_ERR(p))
> 		return PTR_ERR(p);
> 	len = buf + sz - p;
> 	memmove(buf, p, len);
> 	return len;

ok, will use this

> and be done with that.  BTW, the odds of p == buf are pretty much
> nil - it would happen only if sz - 1 happened to be the exact length
> of pathname.
> 

ok, great

thanks,
jirka

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Al Viro]

On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:

> +BTF_SET_START(btf_whitelist_d_path)
> +BTF_ID(func, vfs_truncate)
> +BTF_ID(func, vfs_fallocate)
> +BTF_ID(func, dentry_open)
> +BTF_ID(func, vfs_getattr)
> +BTF_ID(func, filp_close)
> +BTF_SET_END(btf_whitelist_d_path)

While we are at it, I hope you realize that the names of kernel function
are subject to change at zero notice.  If some script breaks since
we give e.g. filp_close a something less revolting name, it's Not My
Problem(tm)...

Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

[Jiri Olsa]

On Wed, Jul 29, 2020 at 09:17:40PM +0100, Al Viro wrote:
> On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:
> 
> > +BTF_SET_START(btf_whitelist_d_path)
> > +BTF_ID(func, vfs_truncate)
> > +BTF_ID(func, vfs_fallocate)
> > +BTF_ID(func, dentry_open)
> > +BTF_ID(func, vfs_getattr)
> > +BTF_ID(func, filp_close)
> > +BTF_SET_END(btf_whitelist_d_path)
> 
> While we are at it, I hope you realize that the names of kernel function
> are subject to change at zero notice.  If some script breaks since
> we give e.g. filp_close a something less revolting name, it's Not My
> Problem(tm)...

even now when we change function name some scripts will stop working,
so I don't think we are creating new problem in here

thanks,
jirka

[PATCH v8 bpf-next 10/13] bpf: Update .BTF_ids section in btf.rst with sets info

[Jiri Olsa]

Updating btf.rst doc with info about BTF_SET_START/END macros.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Documentation/bpf/btf.rst | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/bpf/btf.rst b/Documentation/bpf/btf.rst
index b5361b8621c9..44dc789de2b4 100644
--- a/Documentation/bpf/btf.rst
+++ b/Documentation/bpf/btf.rst
@@ -724,6 +724,31 @@ want to define unused entry in BTF_ID_LIST, like::
       BTF_ID_UNUSED
       BTF_ID(struct, task_struct)
 
+The ``BTF_SET_START/END`` macros pair defines sorted list of BTF ID values
+and their count, with following syntax::
+
+  BTF_SET_START(set)
+  BTF_ID(type1, name1)
+  BTF_ID(type2, name2)
+  BTF_SET_END(set)
+
+resulting in following layout in .BTF_ids section::
+
+  __BTF_ID__set__set:
+  .zero 4
+  __BTF_ID__type1__name1__3:
+  .zero 4
+  __BTF_ID__type2__name2__4:
+  .zero 4
+
+The ``struct btf_id_set set;`` variable is defined to access the list.
+
+The ``typeX`` name can be one of following::
+
+   struct, union, typedef, func
+
+and is used as a filter when resolving the BTF ID value.
+
 All the BTF ID lists and sets are compiled in the .BTF_ids section and
 resolved during the linking phase of kernel build by ``resolve_btfids`` tool.
 
-- 
2.25.4

[PATCH v8 bpf-next 11/13] selftests/bpf: Add verifier test for d_path helper

[Jiri Olsa]

Adding verifier test for attaching tracing program and
calling d_path helper from within and testing that it's
allowed for dentry_open function and denied for 'd_path'
function with appropriate error.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/test_verifier.c   | 19 +++++++++-
 tools/testing/selftests/bpf/verifier/d_path.c | 37 +++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 78a6bae56ea6..9be395d9dc64 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -114,6 +114,7 @@ struct bpf_test {
 		bpf_testdata_struct_t retvals[MAX_TEST_RUNS];
 	};
 	enum bpf_attach_type expected_attach_type;
+	const char *kfunc;
 };
 
 /* Note we want this to be 64 bit aligned so that the end of our array is
@@ -984,8 +985,24 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
 		attr.log_level = 4;
 	attr.prog_flags = pflags;
 
+	if (prog_type == BPF_PROG_TYPE_TRACING && test->kfunc) {
+		attr.attach_btf_id = libbpf_find_vmlinux_btf_id(test->kfunc,
+						attr.expected_attach_type);
+		if (attr.attach_btf_id < 0) {
+			printf("FAIL\nFailed to find BTF ID for '%s'!\n",
+				test->kfunc);
+			(*errors)++;
+			return;
+		}
+	}
+
 	fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
-	if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
+
+	/* BPF_PROG_TYPE_TRACING requires more setup and
+	 * bpf_probe_prog_type won't give correct answer
+	 */
+	if (fd_prog < 0 && prog_type != BPF_PROG_TYPE_TRACING &&
+	    !bpf_probe_prog_type(prog_type, 0)) {
 		printf("SKIP (unsupported program type %d)\n", prog_type);
 		skips++;
 		goto close_fds;
diff --git a/tools/testing/selftests/bpf/verifier/d_path.c b/tools/testing/selftests/bpf/verifier/d_path.c
new file mode 100644
index 000000000000..b988396379a7
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/d_path.c
@@ -0,0 +1,37 @@
+{
+	"d_path accept",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+	BPF_MOV64_IMM(BPF_REG_6, 0),
+	BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
+	BPF_LD_IMM64(BPF_REG_3, 8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "dentry_open",
+},
+{
+	"d_path reject",
+	.insns = {
+	BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0),
+	BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+	BPF_MOV64_IMM(BPF_REG_6, 0),
+	BPF_STX_MEM(BPF_DW, BPF_REG_2, BPF_REG_6, 0),
+	BPF_LD_IMM64(BPF_REG_3, 8),
+	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_d_path),
+	BPF_MOV64_IMM(BPF_REG_0, 0),
+	BPF_EXIT_INSN(),
+	},
+	.errstr = "helper call is not allowed in probe",
+	.result = REJECT,
+	.prog_type = BPF_PROG_TYPE_TRACING,
+	.expected_attach_type = BPF_TRACE_FENTRY,
+	.kfunc = "d_path",
+},
-- 
2.25.4

Re: [PATCH v8 bpf-next 11/13] selftests/bpf: Add verifier test for d_path helper

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding verifier test for attaching tracing program and
> calling d_path helper from within and testing that it's
> allowed for dentry_open function and denied for 'd_path'
> function with appropriate error.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/test_verifier.c   | 19 +++++++++-
>  tools/testing/selftests/bpf/verifier/d_path.c | 37 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/verifier/d_path.c
>

[...]

[PATCH v8 bpf-next 12/13] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

Adding test for d_path helper which is pretty much
copied from Wenbo Zhang's test for bpf_get_fd_path,
which never made it in.

The test is doing fstat/close on several fd types,
and verifies we got the d_path helper working on
kernel probes for vfs_getattr/filp_close functions.

Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/d_path.c | 162 ++++++++++++++++++
 .../testing/selftests/bpf/progs/test_d_path.c |  64 +++++++
 2 files changed, 226 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c

diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
new file mode 100644
index 000000000000..3b8f87fb17d7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_FILES		7
+#define MAX_EVENT_NUM		16
+
+#include "test_d_path.skel.h"
+
+static struct {
+	__u32 cnt;
+	char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} src;
+
+static int set_pathname(int fd, pid_t pid)
+{
+	char buf[MAX_PATH_LEN];
+
+	snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", pid, fd);
+	return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_fstat_events(pid_t pid)
+{
+	int sockfd = -1, procfd = -1, devfd = -1;
+	int localfd = -1, indicatorfd = -1;
+	int pipefd[2] = { -1, -1 };
+	struct stat fileStat;
+	int ret = -1;
+
+	/* unmountable pseudo-filesystems */
+	if (CHECK_FAIL(pipe(pipefd) < 0))
+		return ret;
+	/* unmountable pseudo-filesystems */
+	sockfd = socket(AF_INET, SOCK_STREAM, 0);
+	if (CHECK_FAIL(sockfd < 0))
+		goto out_close;
+	/* mountable pseudo-filesystems */
+	procfd = open("/proc/self/comm", O_RDONLY);
+	if (CHECK_FAIL(procfd < 0))
+		goto out_close;
+	devfd = open("/dev/urandom", O_RDONLY);
+	if (CHECK_FAIL(devfd < 0))
+		goto out_close;
+	localfd = open("/tmp/d_path_loadgen.txt", O_CREAT | O_RDONLY);
+	if (CHECK_FAIL(localfd < 0))
+		goto out_close;
+	/* bpf_d_path will return path with (deleted) */
+	remove("/tmp/d_path_loadgen.txt");
+	indicatorfd = open("/tmp/", O_PATH);
+	if (CHECK_FAIL(indicatorfd < 0))
+		goto out_close;
+
+	ret = set_pathname(pipefd[0], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(pipefd[1], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(sockfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(procfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(devfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(localfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(indicatorfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+
+	/* triggers vfs_getattr */
+	fstat(pipefd[0], &fileStat);
+	fstat(pipefd[1], &fileStat);
+	fstat(sockfd, &fileStat);
+	fstat(procfd, &fileStat);
+	fstat(devfd, &fileStat);
+	fstat(localfd, &fileStat);
+	fstat(indicatorfd, &fileStat);
+
+out_close:
+	/* triggers filp_close */
+	close(pipefd[0]);
+	close(pipefd[1]);
+	close(sockfd);
+	close(procfd);
+	close(devfd);
+	close(localfd);
+	close(indicatorfd);
+	return ret;
+}
+
+void test_d_path(void)
+{
+	struct test_d_path__bss *bss;
+	struct test_d_path *skel;
+	__u32 duration = 0;
+	int err;
+
+	skel = test_d_path__open_and_load();
+	if (CHECK(!skel, "test_d_path_load", "d_path skeleton failed\n"))
+		goto cleanup;
+
+	err = test_d_path__attach(skel);
+	if (CHECK(err, "modify_return", "attach failed: %d\n", err))
+		goto cleanup;
+
+	bss = skel->bss;
+	bss->my_pid = getpid();
+
+	err = trigger_fstat_events(bss->my_pid);
+	if (CHECK_FAIL(err < 0))
+		goto cleanup;
+
+	for (int i = 0; i < MAX_FILES; i++) {
+		if (i < 3) {
+			CHECK((bss->paths_stat[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_stat[i]);
+			CHECK((bss->paths_close[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_close[i]);
+			CHECK((bss->rets_stat[i] < 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_close[i]);
+			CHECK((bss->rets_close[i] < 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_close[i]);
+		} else {
+			CHECK(strncmp(src.paths[i], bss->paths_stat[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get stat path[%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_stat[i]);
+			CHECK(strncmp(src.paths[i], bss->paths_close[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get close path[%d]: %s vs %s\n",
+			      i, src.paths[i], bss->paths_close[i]);
+			/* The d_path helper returns size plus NUL char, hence + 1 */
+			CHECK(bss->rets_stat[i] != strlen(bss->paths_stat[i]) + 1,
+			      "d_path",
+			      "failed to match stat return [%d]: %d vs %zd [%s]\n",
+			      i, bss->rets_stat[i], strlen(bss->paths_stat[i]) + 1,
+			      bss->paths_stat[i]);
+			CHECK(bss->rets_close[i] != strlen(bss->paths_stat[i]) + 1,
+			      "d_path",
+			      "failed to match stat return [%d]: %d vs %zd [%s]\n",
+			      i, bss->rets_close[i], strlen(bss->paths_close[i]) + 1,
+			      bss->paths_stat[i]);
+		}
+	}
+
+cleanup:
+	test_d_path__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
new file mode 100644
index 000000000000..e02dce614256
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_EVENT_NUM		16
+
+pid_t my_pid;
+__u32 cnt_stat;
+__u32 cnt_close;
+char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
+char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
+int rets_stat[MAX_EVENT_NUM];
+int rets_close[MAX_EVENT_NUM];
+
+SEC("fentry/vfs_getattr")
+int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
+	     __u32 request_mask, unsigned int query_flags)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+	int ret;
+
+	if (pid != my_pid)
+		return 0;
+
+	if (cnt_stat >= MAX_EVENT_NUM)
+		return 0;
+	ret = bpf_d_path(path, paths_stat[cnt_stat], MAX_PATH_LEN);
+
+	/* We need to recheck cnt_stat for verifier. */
+	if (cnt_stat >= MAX_EVENT_NUM)
+		return 0;
+	rets_stat[cnt_stat] = ret;
+
+	cnt_stat++;
+	return 0;
+}
+
+SEC("fentry/filp_close")
+int BPF_PROG(prog_close, struct file *file, void *id)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+	int ret;
+
+	if (pid != my_pid)
+		return 0;
+
+	if (cnt_close >= MAX_EVENT_NUM)
+		return 0;
+	ret = bpf_d_path(&file->f_path,
+			 paths_close[cnt_close], MAX_PATH_LEN);
+
+	/* We need to recheck cnt_stat for verifier. */
+	if (cnt_close >= MAX_EVENT_NUM)
+		return 0;
+	rets_close[cnt_close] = ret;
+
+	cnt_close++;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.4

Re: [PATCH v8 bpf-next 12/13] selftests/bpf: Add test for d_path helper

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test for d_path helper which is pretty much
> copied from Wenbo Zhang's test for bpf_get_fd_path,
> which never made it in.
>
> The test is doing fstat/close on several fd types,
> and verifies we got the d_path helper working on
> kernel probes for vfs_getattr/filp_close functions.
>
> Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../testing/selftests/bpf/prog_tests/d_path.c | 162 ++++++++++++++++++
>  .../testing/selftests/bpf/progs/test_d_path.c |  64 +++++++
>  2 files changed, 226 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> new file mode 100644
> index 000000000000..3b8f87fb17d7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +#include <test_progs.h>
> +#include <sys/stat.h>
> +#include <linux/sched.h>
> +#include <sys/syscall.h>
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_FILES              7
> +#define MAX_EVENT_NUM          16
> +
> +#include "test_d_path.skel.h"
> +
> +static struct {
> +       __u32 cnt;
> +       char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
> +} src;
> +
> +static int set_pathname(int fd, pid_t pid)
> +{
> +       char buf[MAX_PATH_LEN];
> +
> +       snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", pid, fd);
> +       return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
> +}
> +
> +static int trigger_fstat_events(pid_t pid)
> +{
> +       int sockfd = -1, procfd = -1, devfd = -1;
> +       int localfd = -1, indicatorfd = -1;
> +       int pipefd[2] = { -1, -1 };
> +       struct stat fileStat;
> +       int ret = -1;
> +
> +       /* unmountable pseudo-filesystems */
> +       if (CHECK_FAIL(pipe(pipefd) < 0))
> +               return ret;
> +       /* unmountable pseudo-filesystems */
> +       sockfd = socket(AF_INET, SOCK_STREAM, 0);
> +       if (CHECK_FAIL(sockfd < 0))
> +               goto out_close;
> +       /* mountable pseudo-filesystems */
> +       procfd = open("/proc/self/comm", O_RDONLY);
> +       if (CHECK_FAIL(procfd < 0))
> +               goto out_close;
> +       devfd = open("/dev/urandom", O_RDONLY);
> +       if (CHECK_FAIL(devfd < 0))
> +               goto out_close;
> +       localfd = open("/tmp/d_path_loadgen.txt", O_CREAT | O_RDONLY);
> +       if (CHECK_FAIL(localfd < 0))
> +               goto out_close;
> +       /* bpf_d_path will return path with (deleted) */
> +       remove("/tmp/d_path_loadgen.txt");
> +       indicatorfd = open("/tmp/", O_PATH);
> +       if (CHECK_FAIL(indicatorfd < 0))
> +               goto out_close;
> +
> +       ret = set_pathname(pipefd[0], pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(pipefd[1], pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(sockfd, pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(procfd, pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(devfd, pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(localfd, pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;
> +       ret = set_pathname(indicatorfd, pid);
> +       if (CHECK_FAIL(ret < 0))
> +               goto out_close;

Please use CHECK instead of CHECK_FAIL. Thanks.

> +
> +       /* triggers vfs_getattr */
> +       fstat(pipefd[0], &fileStat);
> +       fstat(pipefd[1], &fileStat);
> +       fstat(sockfd, &fileStat);
> +       fstat(procfd, &fileStat);
> +       fstat(devfd, &fileStat);
> +       fstat(localfd, &fileStat);
> +       fstat(indicatorfd, &fileStat);
> +
> +out_close:
> +       /* triggers filp_close */
> +       close(pipefd[0]);
> +       close(pipefd[1]);
> +       close(sockfd);
> +       close(procfd);
> +       close(devfd);
> +       close(localfd);
> +       close(indicatorfd);
> +       return ret;
> +}
> +

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> new file mode 100644
> index 000000000000..e02dce614256
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +#define MAX_PATH_LEN           128
> +#define MAX_EVENT_NUM          16
> +
> +pid_t my_pid;
> +__u32 cnt_stat;
> +__u32 cnt_close;
> +char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> +char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> +int rets_stat[MAX_EVENT_NUM];
> +int rets_close[MAX_EVENT_NUM];
> +

please zero-initialize all of these, it causes issues on some Clang versions

[...]

Re: [PATCH v8 bpf-next 12/13] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

On Tue, Jul 28, 2020 at 12:53:00PM -0700, Andrii Nakryiko wrote:

SNIP

> > +       if (CHECK_FAIL(ret < 0))
> > +               goto out_close;
> > +       ret = set_pathname(procfd, pid);
> > +       if (CHECK_FAIL(ret < 0))
> > +               goto out_close;
> > +       ret = set_pathname(devfd, pid);
> > +       if (CHECK_FAIL(ret < 0))
> > +               goto out_close;
> > +       ret = set_pathname(localfd, pid);
> > +       if (CHECK_FAIL(ret < 0))
> > +               goto out_close;
> > +       ret = set_pathname(indicatorfd, pid);
> > +       if (CHECK_FAIL(ret < 0))
> > +               goto out_close;
> 
> Please use CHECK instead of CHECK_FAIL. Thanks.

ok

> > diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
> > new file mode 100644
> > index 000000000000..e02dce614256
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_d_path.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +#define MAX_PATH_LEN           128
> > +#define MAX_EVENT_NUM          16
> > +
> > +pid_t my_pid;
> > +__u32 cnt_stat;
> > +__u32 cnt_close;
> > +char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
> > +int rets_stat[MAX_EVENT_NUM];
> > +int rets_close[MAX_EVENT_NUM];
> > +
> 
> please zero-initialize all of these, it causes issues on some Clang versions

ook

jirka

[PATCH v8 bpf-next 13/13] selftests/bpf: Add set test to resolve_btfids

[Jiri Olsa]

Adding test to for sets resolve_btfids. We're checking that
testing set gets properly resolved and sorted.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../selftests/bpf/prog_tests/resolve_btfids.c | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
index 101785b49f7e..cc90aa244285 100644
--- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
+++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
@@ -48,6 +48,15 @@ BTF_ID(struct,  S)
 BTF_ID(union,   U)
 BTF_ID(func,    func)
 
+BTF_SET_START(test_set)
+BTF_ID(typedef, S)
+BTF_ID(typedef, T)
+BTF_ID(typedef, U)
+BTF_ID(struct,  S)
+BTF_ID(union,   U)
+BTF_ID(func,    func)
+BTF_SET_END(test_set)
+
 static int
 __resolve_symbol(struct btf *btf, int type_id)
 {
@@ -126,5 +135,29 @@ int test_resolve_btfids(void)
 		}
 	}
 
+	/* Check BTF_SET_START(test_set) IDs */
+	for (i = 0; i < test_set.cnt && !ret; i++) {
+		bool found = false;
+
+		for (j = 0; j < ARRAY_SIZE(test_symbols); j++) {
+			if (test_symbols[j].id != test_set.ids[i])
+				continue;
+			found = true;
+			break;
+		}
+
+		ret = CHECK(!found, "id_check",
+			    "ID %d for %s not found in test_symbols\n",
+			    test_symbols[j].id, test_symbols[j].name);
+		if (ret)
+			break;
+
+		if (i > 0) {
+			ret = CHECK(test_set.ids[i - 1] > test_set.ids[i],
+				    "sort_check",
+				    "test_set is not sorted\n");
+		}
+	}
+
 	return ret;
 }
-- 
2.25.4

Re: [PATCH v8 bpf-next 13/13] selftests/bpf: Add set test to resolve_btfids

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 2:15 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test to for sets resolve_btfids. We're checking that
> testing set gets properly resolved and sorted.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../selftests/bpf/prog_tests/resolve_btfids.c | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> index 101785b49f7e..cc90aa244285 100644
> --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> @@ -48,6 +48,15 @@ BTF_ID(struct,  S)
>  BTF_ID(union,   U)
>  BTF_ID(func,    func)
>
> +BTF_SET_START(test_set)
> +BTF_ID(typedef, S)
> +BTF_ID(typedef, T)
> +BTF_ID(typedef, U)
> +BTF_ID(struct,  S)
> +BTF_ID(union,   U)
> +BTF_ID(func,    func)
> +BTF_SET_END(test_set)
> +
>  static int
>  __resolve_symbol(struct btf *btf, int type_id)
>  {
> @@ -126,5 +135,29 @@ int test_resolve_btfids(void)
>                 }
>         }
>
> +       /* Check BTF_SET_START(test_set) IDs */
> +       for (i = 0; i < test_set.cnt && !ret; i++) {

nit: usual we just do `goto err_out;` instead of complicating exit
condition in a for loop

> +               bool found = false;
> +
> +               for (j = 0; j < ARRAY_SIZE(test_symbols); j++) {
> +                       if (test_symbols[j].id != test_set.ids[i])
> +                               continue;
> +                       found = true;
> +                       break;
> +               }
> +
> +               ret = CHECK(!found, "id_check",
> +                           "ID %d for %s not found in test_symbols\n",
> +                           test_symbols[j].id, test_symbols[j].name);

j == ARRAY_SIZE(test_symbols), you probably meant to get
test_set.ids[i] instead of test_symbol name/id?

> +               if (ret)
> +                       break;
> +
> +               if (i > 0) {
> +                       ret = CHECK(test_set.ids[i - 1] > test_set.ids[i],

nit: >= would be the invalid condition

> +                                   "sort_check",
> +                                   "test_set is not sorted\n");
> +               }
> +       }
> +
>         return ret;
>  }
> --
> 2.25.4
>

Re: [PATCH v8 bpf-next 13/13] selftests/bpf: Add set test to resolve_btfids

[Jiri Olsa]

On Tue, Jul 28, 2020 at 12:56:02PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 2:15 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test to for sets resolve_btfids. We're checking that
> > testing set gets properly resolved and sorted.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  .../selftests/bpf/prog_tests/resolve_btfids.c | 33 +++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > index 101785b49f7e..cc90aa244285 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/resolve_btfids.c
> > @@ -48,6 +48,15 @@ BTF_ID(struct,  S)
> >  BTF_ID(union,   U)
> >  BTF_ID(func,    func)
> >
> > +BTF_SET_START(test_set)
> > +BTF_ID(typedef, S)
> > +BTF_ID(typedef, T)
> > +BTF_ID(typedef, U)
> > +BTF_ID(struct,  S)
> > +BTF_ID(union,   U)
> > +BTF_ID(func,    func)
> > +BTF_SET_END(test_set)
> > +
> >  static int
> >  __resolve_symbol(struct btf *btf, int type_id)
> >  {
> > @@ -126,5 +135,29 @@ int test_resolve_btfids(void)
> >                 }
> >         }
> >
> > +       /* Check BTF_SET_START(test_set) IDs */
> > +       for (i = 0; i < test_set.cnt && !ret; i++) {
> 
> nit: usual we just do `goto err_out;` instead of complicating exit
> condition in a for loop

ok

> 
> > +               bool found = false;
> > +
> > +               for (j = 0; j < ARRAY_SIZE(test_symbols); j++) {
> > +                       if (test_symbols[j].id != test_set.ids[i])
> > +                               continue;
> > +                       found = true;
> > +                       break;
> > +               }
> > +
> > +               ret = CHECK(!found, "id_check",
> > +                           "ID %d for %s not found in test_symbols\n",
> > +                           test_symbols[j].id, test_symbols[j].name);
> 
> j == ARRAY_SIZE(test_symbols), you probably meant to get
> test_set.ids[i] instead of test_symbol name/id?

oh yea.. test_set.ids[i] is not found in here

> 
> > +               if (ret)
> > +                       break;
> > +
> > +               if (i > 0) {
> > +                       ret = CHECK(test_set.ids[i - 1] > test_set.ids[i],
> 
> nit: >= would be the invalid condition

yes, we actualy allow for same IDs to appear in the set

thanks,
jirka

> 
> > +                                   "sort_check",
> > +                                   "test_set is not sorted\n");
> > +               }
> > +       }
> > +
> >         return ret;
> >  }
> > --
> > 2.25.4
> >
>