[PATCH v4 bpf-next 0/7] Add fd_array_cnt attribute for BPF_PROG_LOAD

[PATCH v4 bpf-next 0/7] Add fd_array_cnt attribute for BPF_PROG_LOAD

[Anton Protopopov]

Add a new attribute to the bpf(BPF_PROG_LOAD) system call. If this
new attribute is non-zero, then the fd_array is considered to be a
continuous array of the fd_array_cnt length and to contain only
proper map file descriptors or btf file descriptors.

This change allows maps, which aren't referenced directly by a BPF
program, to be bound to the program _and_ also to be present during
the program verification (so BPF_PROG_BIND_MAP is not enough for this
use case).

The primary reason for this change is that it is a prerequisite for
adding "instruction set" maps, which are both non-referenced by the
program and must be present during the program verification.

The first four commits add the new functionality, the fifth adds
corresponding self-tests, and the last two are small additional fixes.

v1 -> v2:
  * rewrite the add_fd_from_fd_array() function (Eduard)
  * a few cleanups in selftests (Eduard)

v2 -> v3:
  * various renamings (Alexei)
  * "0 is not special" (Alexei, Andrii)
  * do not alloc memory on fd_array init (Alexei)
  * fix leaking maps for error path (Hou Tao)
  * use libbpf helpers vs. raw syscalls (Andrii)
  * add comments on __btf_get_by_fd/__bpf_map_get (Alexei)
  * remove extra code (Alexei)

v3 -> v4:
  * simplify error path when parsing fd_array
  * libbpf: pass fd_array_cnt only in prog_load (Alexei)
  * selftests patch contained extra code (Alexei)
  * renames, fix comments (Alexei)

Anton Protopopov (7):
  bpf: add a __btf_get_by_fd helper
  bpf: move map/prog compatibility checks
  bpf: add fd_array_cnt attribute for prog_load
  libbpf: prog load: allow to use fd_array_cnt
  selftests/bpf: Add tests for fd_array_cnt
  bpf: fix potential error return
  selftest/bpf: replace magic constants by macros

 include/linux/bpf.h                           |  17 +
 include/linux/btf.h                           |   2 +
 include/uapi/linux/bpf.h                      |  10 +
 kernel/bpf/btf.c                              |  13 +-
 kernel/bpf/core.c                             |   6 +-
 kernel/bpf/syscall.c                          |   2 +-
 kernel/bpf/verifier.c                         | 193 ++++++----
 tools/include/uapi/linux/bpf.h                |  10 +
 tools/lib/bpf/bpf.c                           |   3 +-
 tools/lib/bpf/bpf.h                           |   5 +-
 .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/syscall.c   |   6 +-
 12 files changed, 524 insertions(+), 83 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c

-- 
2.34.1

[PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Anton Protopopov]

Add a new helper to get a pointer to a struct btf from a file
descriptor. This helper doesn't increase a refcnt. Add a comment
explaining this and pointing to a corresponding function which
does take a reference.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 include/linux/bpf.h | 17 +++++++++++++++++
 include/linux/btf.h |  2 ++
 kernel/bpf/btf.c    | 13 ++++---------
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eaee2a819f4c..ac44b857b2f9 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
 struct bpf_map *bpf_map_get(u32 ufd);
 struct bpf_map *bpf_map_get_with_uref(u32 ufd);
 
+/*
+ * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
+ * descriptor and return a corresponding map or btf object.
+ * Their names are double underscored to emphasize the fact that they
+ * do not increase refcnt. To also increase refcnt use corresponding
+ * bpf_map_get() and btf_get_by_fd() functions.
+ */
+
 static inline struct bpf_map *__bpf_map_get(struct fd f)
 {
 	if (fd_empty(f))
@@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
 	return fd_file(f)->private_data;
 }
 
+static inline struct btf *__btf_get_by_fd(struct fd f)
+{
+	if (fd_empty(f))
+		return ERR_PTR(-EBADF);
+	if (unlikely(fd_file(f)->f_op != &btf_fops))
+		return ERR_PTR(-EINVAL);
+	return fd_file(f)->private_data;
+}
+
 void bpf_map_inc(struct bpf_map *map);
 void bpf_map_inc_with_uref(struct bpf_map *map);
 struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 4214e76c9168..69159e649675 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -4,6 +4,7 @@
 #ifndef _LINUX_BTF_H
 #define _LINUX_BTF_H 1
 
+#include <linux/file.h>
 #include <linux/types.h>
 #include <linux/bpfptr.h>
 #include <linux/bsearch.h>
@@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
 void btf_put(struct btf *btf);
 const struct btf_header *btf_header(const struct btf *btf);
 int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
+
 struct btf *btf_get_by_fd(int fd);
 int btf_get_info_by_fd(const struct btf *btf,
 		       const union bpf_attr *attr,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e7a59e6462a9..ad5310fa1d3b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 
 struct btf *btf_get_by_fd(int fd)
 {
-	struct btf *btf;
 	CLASS(fd, f)(fd);
+	struct btf *btf;
 
-	if (fd_empty(f))
-		return ERR_PTR(-EBADF);
-
-	if (fd_file(f)->f_op != &btf_fops)
-		return ERR_PTR(-EINVAL);
-
-	btf = fd_file(f)->private_data;
-	refcount_inc(&btf->refcnt);
+	btf = __btf_get_by_fd(f);
+	if (!IS_ERR(btf))
+		refcount_inc(&btf->refcnt);
 
 	return btf;
 }
-- 
2.34.1

Re: [PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> Add a new helper to get a pointer to a struct btf from a file
> descriptor. This helper doesn't increase a refcnt. Add a comment
> explaining this and pointing to a corresponding function which
> does take a reference.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  include/linux/bpf.h | 17 +++++++++++++++++
>  include/linux/btf.h |  2 ++
>  kernel/bpf/btf.c    | 13 ++++---------
>  3 files changed, 23 insertions(+), 9 deletions(-)
>

Minor (but unexplained and/or unnecessary) things I pointed out below,
but overall looks good

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index eaee2a819f4c..ac44b857b2f9 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
>  struct bpf_map *bpf_map_get(u32 ufd);
>  struct bpf_map *bpf_map_get_with_uref(u32 ufd);
>
> +/*
> + * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
> + * descriptor and return a corresponding map or btf object.
> + * Their names are double underscored to emphasize the fact that they
> + * do not increase refcnt. To also increase refcnt use corresponding
> + * bpf_map_get() and btf_get_by_fd() functions.
> + */
> +
>  static inline struct bpf_map *__bpf_map_get(struct fd f)
>  {
>         if (fd_empty(f))
> @@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
>         return fd_file(f)->private_data;
>  }
>
> +static inline struct btf *__btf_get_by_fd(struct fd f)
> +{
> +       if (fd_empty(f))
> +               return ERR_PTR(-EBADF);
> +       if (unlikely(fd_file(f)->f_op != &btf_fops))
> +               return ERR_PTR(-EINVAL);
> +       return fd_file(f)->private_data;
> +}
> +
>  void bpf_map_inc(struct bpf_map *map);
>  void bpf_map_inc_with_uref(struct bpf_map *map);
>  struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 4214e76c9168..69159e649675 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -4,6 +4,7 @@
>  #ifndef _LINUX_BTF_H
>  #define _LINUX_BTF_H 1
>
> +#include <linux/file.h>

do we need this in linux/btf.h header?

>  #include <linux/types.h>
>  #include <linux/bpfptr.h>
>  #include <linux/bsearch.h>
> @@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
>  void btf_put(struct btf *btf);
>  const struct btf_header *btf_header(const struct btf *btf);
>  int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
> +

?

>  struct btf *btf_get_by_fd(int fd);
>  int btf_get_info_by_fd(const struct btf *btf,
>                        const union bpf_attr *attr,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e7a59e6462a9..ad5310fa1d3b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
>
>  struct btf *btf_get_by_fd(int fd)
>  {
> -       struct btf *btf;
>         CLASS(fd, f)(fd);
> +       struct btf *btf;

nit: no need to just move this around



>
> -       if (fd_empty(f))
> -               return ERR_PTR(-EBADF);
> -
> -       if (fd_file(f)->f_op != &btf_fops)
> -               return ERR_PTR(-EINVAL);
> -
> -       btf = fd_file(f)->private_data;
> -       refcount_inc(&btf->refcnt);
> +       btf = __btf_get_by_fd(f);
> +       if (!IS_ERR(btf))
> +               refcount_inc(&btf->refcnt);
>
>         return btf;
>  }
> --
> 2.34.1
>
>

Re: [PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Anton Protopopov]

On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > Add a new helper to get a pointer to a struct btf from a file
> > descriptor. This helper doesn't increase a refcnt. Add a comment
> > explaining this and pointing to a corresponding function which
> > does take a reference.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  include/linux/bpf.h | 17 +++++++++++++++++
> >  include/linux/btf.h |  2 ++
> >  kernel/bpf/btf.c    | 13 ++++---------
> >  3 files changed, 23 insertions(+), 9 deletions(-)
> >
> 
> Minor (but unexplained and/or unnecessary) things I pointed out below,
> but overall looks good
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index eaee2a819f4c..ac44b857b2f9 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
> >  struct bpf_map *bpf_map_get(u32 ufd);
> >  struct bpf_map *bpf_map_get_with_uref(u32 ufd);
> >
> > +/*
> > + * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
> > + * descriptor and return a corresponding map or btf object.
> > + * Their names are double underscored to emphasize the fact that they
> > + * do not increase refcnt. To also increase refcnt use corresponding
> > + * bpf_map_get() and btf_get_by_fd() functions.
> > + */
> > +
> >  static inline struct bpf_map *__bpf_map_get(struct fd f)
> >  {
> >         if (fd_empty(f))
> > @@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
> >         return fd_file(f)->private_data;
> >  }
> >
> > +static inline struct btf *__btf_get_by_fd(struct fd f)
> > +{
> > +       if (fd_empty(f))
> > +               return ERR_PTR(-EBADF);
> > +       if (unlikely(fd_file(f)->f_op != &btf_fops))
> > +               return ERR_PTR(-EINVAL);
> > +       return fd_file(f)->private_data;
> > +}
> > +
> >  void bpf_map_inc(struct bpf_map *map);
> >  void bpf_map_inc_with_uref(struct bpf_map *map);
> >  struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 4214e76c9168..69159e649675 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -4,6 +4,7 @@
> >  #ifndef _LINUX_BTF_H
> >  #define _LINUX_BTF_H 1
> >
> > +#include <linux/file.h>
> 
> do we need this in linux/btf.h header?

Thanks, removed.

> >  #include <linux/types.h>
> >  #include <linux/bpfptr.h>
> >  #include <linux/bsearch.h>
> > @@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
> >  void btf_put(struct btf *btf);
> >  const struct btf_header *btf_header(const struct btf *btf);
> >  int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
> > +
> 
> ?

Thanks, removed.

> >  struct btf *btf_get_by_fd(int fd);
> >  int btf_get_info_by_fd(const struct btf *btf,
> >                        const union bpf_attr *attr,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index e7a59e6462a9..ad5310fa1d3b 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> >
> >  struct btf *btf_get_by_fd(int fd)
> >  {
> > -       struct btf *btf;
> >         CLASS(fd, f)(fd);
> > +       struct btf *btf;
> 
> nit: no need to just move this around

Ok, I can remove it. I moved it to form a reverse xmas tree,
as I was already editing this function.

> 
> 
> >
> > -       if (fd_empty(f))
> > -               return ERR_PTR(-EBADF);
> > -
> > -       if (fd_file(f)->f_op != &btf_fops)
> > -               return ERR_PTR(-EINVAL);
> > -
> > -       btf = fd_file(f)->private_data;
> > -       refcount_inc(&btf->refcnt);
> > +       btf = __btf_get_by_fd(f);
> > +       if (!IS_ERR(btf))
> > +               refcount_inc(&btf->refcnt);
> >
> >         return btf;
> >  }
> > --
> > 2.34.1
> >
> >

Re: [PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Andrii Nakryiko]

On Wed, Dec 4, 2024 at 2:39 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > Add a new helper to get a pointer to a struct btf from a file
> > > descriptor. This helper doesn't increase a refcnt. Add a comment
> > > explaining this and pointing to a corresponding function which
> > > does take a reference.
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > >  include/linux/bpf.h | 17 +++++++++++++++++
> > >  include/linux/btf.h |  2 ++
> > >  kernel/bpf/btf.c    | 13 ++++---------
> > >  3 files changed, 23 insertions(+), 9 deletions(-)
> > >
> >
> > Minor (but unexplained and/or unnecessary) things I pointed out below,
> > but overall looks good
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index eaee2a819f4c..ac44b857b2f9 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
> > >  struct bpf_map *bpf_map_get(u32 ufd);
> > >  struct bpf_map *bpf_map_get_with_uref(u32 ufd);
> > >
> > > +/*
> > > + * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
> > > + * descriptor and return a corresponding map or btf object.
> > > + * Their names are double underscored to emphasize the fact that they
> > > + * do not increase refcnt. To also increase refcnt use corresponding
> > > + * bpf_map_get() and btf_get_by_fd() functions.
> > > + */
> > > +
> > >  static inline struct bpf_map *__bpf_map_get(struct fd f)
> > >  {
> > >         if (fd_empty(f))
> > > @@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
> > >         return fd_file(f)->private_data;
> > >  }
> > >
> > > +static inline struct btf *__btf_get_by_fd(struct fd f)
> > > +{
> > > +       if (fd_empty(f))
> > > +               return ERR_PTR(-EBADF);
> > > +       if (unlikely(fd_file(f)->f_op != &btf_fops))
> > > +               return ERR_PTR(-EINVAL);
> > > +       return fd_file(f)->private_data;
> > > +}
> > > +
> > >  void bpf_map_inc(struct bpf_map *map);
> > >  void bpf_map_inc_with_uref(struct bpf_map *map);
> > >  struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > index 4214e76c9168..69159e649675 100644
> > > --- a/include/linux/btf.h
> > > +++ b/include/linux/btf.h
> > > @@ -4,6 +4,7 @@
> > >  #ifndef _LINUX_BTF_H
> > >  #define _LINUX_BTF_H 1
> > >
> > > +#include <linux/file.h>
> >
> > do we need this in linux/btf.h header?
>
> Thanks, removed.
>
> > >  #include <linux/types.h>
> > >  #include <linux/bpfptr.h>
> > >  #include <linux/bsearch.h>
> > > @@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
> > >  void btf_put(struct btf *btf);
> > >  const struct btf_header *btf_header(const struct btf *btf);
> > >  int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
> > > +
> >
> > ?
>
> Thanks, removed.
>
> > >  struct btf *btf_get_by_fd(int fd);
> > >  int btf_get_info_by_fd(const struct btf *btf,
> > >                        const union bpf_attr *attr,
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index e7a59e6462a9..ad5310fa1d3b 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> > >
> > >  struct btf *btf_get_by_fd(int fd)
> > >  {
> > > -       struct btf *btf;
> > >         CLASS(fd, f)(fd);
> > > +       struct btf *btf;
> >
> > nit: no need to just move this around
>
> Ok, I can remove it. I moved it to form a reverse xmas tree,
> as I was already editing this function.

we don't enforce the, or adjust to, reverse xmas tree styling, so please don't

>
> >
> >
> > >
> > > -       if (fd_empty(f))
> > > -               return ERR_PTR(-EBADF);
> > > -
> > > -       if (fd_file(f)->f_op != &btf_fops)
> > > -               return ERR_PTR(-EINVAL);
> > > -
> > > -       btf = fd_file(f)->private_data;
> > > -       refcount_inc(&btf->refcnt);
> > > +       btf = __btf_get_by_fd(f);
> > > +       if (!IS_ERR(btf))
> > > +               refcount_inc(&btf->refcnt);
> > >
> > >         return btf;
> > >  }
> > > --
> > > 2.34.1
> > >
> > >

Re: [PATCH v4 bpf-next 1/7] bpf: add a __btf_get_by_fd helper

[Anton Protopopov]

On 24/12/04 09:58AM, Andrii Nakryiko wrote:
> On Wed, Dec 4, 2024 at 2:39 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > >
> > > > Add a new helper to get a pointer to a struct btf from a file
> > > > descriptor. This helper doesn't increase a refcnt. Add a comment
> > > > explaining this and pointing to a corresponding function which
> > > > does take a reference.
> > > >
> > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > ---
> > > >  include/linux/bpf.h | 17 +++++++++++++++++
> > > >  include/linux/btf.h |  2 ++
> > > >  kernel/bpf/btf.c    | 13 ++++---------
> > > >  3 files changed, 23 insertions(+), 9 deletions(-)
> > > >
> > >
> > > Minor (but unexplained and/or unnecessary) things I pointed out below,
> > > but overall looks good
> > >
> > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index eaee2a819f4c..ac44b857b2f9 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -2301,6 +2301,14 @@ void __bpf_obj_drop_impl(void *p, const struct btf_record *rec, bool percpu);
> > > >  struct bpf_map *bpf_map_get(u32 ufd);
> > > >  struct bpf_map *bpf_map_get_with_uref(u32 ufd);
> > > >
> > > > +/*
> > > > + * The __bpf_map_get() and __btf_get_by_fd() functions parse a file
> > > > + * descriptor and return a corresponding map or btf object.
> > > > + * Their names are double underscored to emphasize the fact that they
> > > > + * do not increase refcnt. To also increase refcnt use corresponding
> > > > + * bpf_map_get() and btf_get_by_fd() functions.
> > > > + */
> > > > +
> > > >  static inline struct bpf_map *__bpf_map_get(struct fd f)
> > > >  {
> > > >         if (fd_empty(f))
> > > > @@ -2310,6 +2318,15 @@ static inline struct bpf_map *__bpf_map_get(struct fd f)
> > > >         return fd_file(f)->private_data;
> > > >  }
> > > >
> > > > +static inline struct btf *__btf_get_by_fd(struct fd f)
> > > > +{
> > > > +       if (fd_empty(f))
> > > > +               return ERR_PTR(-EBADF);
> > > > +       if (unlikely(fd_file(f)->f_op != &btf_fops))
> > > > +               return ERR_PTR(-EINVAL);
> > > > +       return fd_file(f)->private_data;
> > > > +}
> > > > +
> > > >  void bpf_map_inc(struct bpf_map *map);
> > > >  void bpf_map_inc_with_uref(struct bpf_map *map);
> > > >  struct bpf_map *__bpf_map_inc_not_zero(struct bpf_map *map, bool uref);
> > > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > > index 4214e76c9168..69159e649675 100644
> > > > --- a/include/linux/btf.h
> > > > +++ b/include/linux/btf.h
> > > > @@ -4,6 +4,7 @@
> > > >  #ifndef _LINUX_BTF_H
> > > >  #define _LINUX_BTF_H 1
> > > >
> > > > +#include <linux/file.h>
> > >
> > > do we need this in linux/btf.h header?
> >
> > Thanks, removed.
> >
> > > >  #include <linux/types.h>
> > > >  #include <linux/bpfptr.h>
> > > >  #include <linux/bsearch.h>
> > > > @@ -143,6 +144,7 @@ void btf_get(struct btf *btf);
> > > >  void btf_put(struct btf *btf);
> > > >  const struct btf_header *btf_header(const struct btf *btf);
> > > >  int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
> > > > +
> > >
> > > ?
> >
> > Thanks, removed.
> >
> > > >  struct btf *btf_get_by_fd(int fd);
> > > >  int btf_get_info_by_fd(const struct btf *btf,
> > > >                        const union bpf_attr *attr,
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index e7a59e6462a9..ad5310fa1d3b 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -7743,17 +7743,12 @@ int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> > > >
> > > >  struct btf *btf_get_by_fd(int fd)
> > > >  {
> > > > -       struct btf *btf;
> > > >         CLASS(fd, f)(fd);
> > > > +       struct btf *btf;
> > >
> > > nit: no need to just move this around
> >
> > Ok, I can remove it. I moved it to form a reverse xmas tree,
> > as I was already editing this function.
> 
> we don't enforce the, or adjust to, reverse xmas tree styling, so please don't

Ok, thanks, good to know. I've removed this diff.

> >
> > >
> > >
> > > >
> > > > -       if (fd_empty(f))
> > > > -               return ERR_PTR(-EBADF);
> > > > -
> > > > -       if (fd_file(f)->f_op != &btf_fops)
> > > > -               return ERR_PTR(-EINVAL);
> > > > -
> > > > -       btf = fd_file(f)->private_data;
> > > > -       refcount_inc(&btf->refcnt);
> > > > +       btf = __btf_get_by_fd(f);
> > > > +       if (!IS_ERR(btf))
> > > > +               refcount_inc(&btf->refcnt);
> > > >
> > > >         return btf;
> > > >  }
> > > > --
> > > > 2.34.1
> > > >
> > > >

[PATCH v4 bpf-next 2/7] bpf: move map/prog compatibility checks

[Anton Protopopov]

Move some inlined map/prog compatibility checks from the
resolve_pseudo_ldimm64() function to the dedicated
check_map_prog_compatibility() function. Call the latter function
from the add_used_map_from_fd() function directly.

This simplifies code and optimizes logic a bit, as before these
changes the check_map_prog_compatibility() function was executed on
every map usage, which doesn't make sense, as it doesn't include any
per-instruction checks, only map type vs. prog type.

(This patch also simplifies a consequent patch which will call the
add_used_map_from_fd() function from another code path.)

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 kernel/bpf/verifier.c | 101 +++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 55 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1c4ebb326785..8e034a22aa2a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19064,6 +19064,12 @@ static bool is_tracing_prog_type(enum bpf_prog_type type)
 	}
 }
 
+static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
+{
+	return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
+		map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
+}
+
 static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 					struct bpf_map *map,
 					struct bpf_prog *prog)
@@ -19142,25 +19148,48 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
-	return 0;
-}
+	if (bpf_map_is_cgroup_storage(map) &&
+	    bpf_cgroup_storage_assign(env->prog->aux, map)) {
+		verbose(env, "only one cgroup storage of each type is allowed\n");
+		return -EBUSY;
+	}
 
-static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
-{
-	return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
-		map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
+	if (map->map_type == BPF_MAP_TYPE_ARENA) {
+		if (env->prog->aux->arena) {
+			verbose(env, "Only one arena per program\n");
+			return -EBUSY;
+		}
+		if (!env->allow_ptr_leaks || !env->bpf_capable) {
+			verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
+			return -EPERM;
+		}
+		if (!env->prog->jit_requested) {
+			verbose(env, "JIT is required to use arena\n");
+			return -EOPNOTSUPP;
+		}
+		if (!bpf_jit_supports_arena()) {
+			verbose(env, "JIT doesn't support arena\n");
+			return -EOPNOTSUPP;
+		}
+		env->prog->aux->arena = (void *)map;
+		if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
+			verbose(env, "arena's user address must be set via map_extra or mmap()\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 /* Add map behind fd to used maps list, if it's not already there, and return
- * its index. Also set *reused to true if this map was already in the list of
- * used maps.
+ * its index.
  * Returns <0 on error, or >= 0 index, on success.
  */
-static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reused)
+static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
 {
 	CLASS(fd, f)(fd);
 	struct bpf_map *map;
-	int i;
+	int i, err;
 
 	map = __bpf_map_get(f);
 	if (IS_ERR(map)) {
@@ -19169,12 +19198,9 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 	}
 
 	/* check whether we recorded this map already */
-	for (i = 0; i < env->used_map_cnt; i++) {
-		if (env->used_maps[i] == map) {
-			*reused = true;
+	for (i = 0; i < env->used_map_cnt; i++)
+		if (env->used_maps[i] == map)
 			return i;
-		}
-	}
 
 	if (env->used_map_cnt >= MAX_USED_MAPS) {
 		verbose(env, "The total number of maps per program has reached the limit of %u\n",
@@ -19182,6 +19208,10 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 		return -E2BIG;
 	}
 
+	err = check_map_prog_compatibility(env, map, env->prog);
+	if (err)
+		return err;
+
 	if (env->prog->sleepable)
 		atomic64_inc(&map->sleepable_refcnt);
 
@@ -19192,7 +19222,6 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd, bool *reus
 	 */
 	bpf_map_inc(map);
 
-	*reused = false;
 	env->used_maps[env->used_map_cnt++] = map;
 
 	return env->used_map_cnt - 1;
@@ -19229,7 +19258,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			int map_idx;
 			u64 addr;
 			u32 fd;
-			bool reused;
 
 			if (i == insn_cnt - 1 || insn[1].code != 0 ||
 			    insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
@@ -19290,7 +19318,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 				break;
 			}
 
-			map_idx = add_used_map_from_fd(env, fd, &reused);
+			map_idx = add_used_map_from_fd(env, fd);
 			if (map_idx < 0)
 				return map_idx;
 			map = env->used_maps[map_idx];
@@ -19298,10 +19326,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			aux = &env->insn_aux_data[i];
 			aux->map_index = map_idx;
 
-			err = check_map_prog_compatibility(env, map, env->prog);
-			if (err)
-				return err;
-
 			if (insn[0].src_reg == BPF_PSEUDO_MAP_FD ||
 			    insn[0].src_reg == BPF_PSEUDO_MAP_IDX) {
 				addr = (unsigned long)map;
@@ -19332,39 +19356,6 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 			insn[0].imm = (u32)addr;
 			insn[1].imm = addr >> 32;
 
-			/* proceed with extra checks only if its newly added used map */
-			if (reused)
-				goto next_insn;
-
-			if (bpf_map_is_cgroup_storage(map) &&
-			    bpf_cgroup_storage_assign(env->prog->aux, map)) {
-				verbose(env, "only one cgroup storage of each type is allowed\n");
-				return -EBUSY;
-			}
-			if (map->map_type == BPF_MAP_TYPE_ARENA) {
-				if (env->prog->aux->arena) {
-					verbose(env, "Only one arena per program\n");
-					return -EBUSY;
-				}
-				if (!env->allow_ptr_leaks || !env->bpf_capable) {
-					verbose(env, "CAP_BPF and CAP_PERFMON are required to use arena\n");
-					return -EPERM;
-				}
-				if (!env->prog->jit_requested) {
-					verbose(env, "JIT is required to use arena\n");
-					return -EOPNOTSUPP;
-				}
-				if (!bpf_jit_supports_arena()) {
-					verbose(env, "JIT doesn't support arena\n");
-					return -EOPNOTSUPP;
-				}
-				env->prog->aux->arena = (void *)map;
-				if (!bpf_arena_get_user_vm_start(env->prog->aux->arena)) {
-					verbose(env, "arena's user address must be set via map_extra or mmap()\n");
-					return -EINVAL;
-				}
-			}
-
 next_insn:
 			insn++;
 			i++;
-- 
2.34.1

[PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
of file descriptors: maps or btfs. This field was introduced as a
sparse array. Introduce a new attribute, fd_array_cnt, which, if
present, indicates that the fd_array is a continuous array of the
corresponding length.

If fd_array_cnt is non-zero, then every map in the fd_array will be
bound to the program, as if it was used by the program. This
functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
maps can be used by the verifier during the program load.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 include/uapi/linux/bpf.h       | 10 ++++
 kernel/bpf/syscall.c           |  2 +-
 kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
 tools/include/uapi/linux/bpf.h | 10 ++++
 4 files changed, 104 insertions(+), 16 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 4162afc6b5d0..2acf9b336371 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1573,6 +1573,16 @@ union bpf_attr {
 		 * If provided, prog_flags should have BPF_F_TOKEN_FD flag set.
 		 */
 		__s32		prog_token_fd;
+		/* The fd_array_cnt can be used to pass the length of the
+		 * fd_array array. In this case all the [map] file descriptors
+		 * passed in this array will be bound to the program, even if
+		 * the maps are not referenced directly. The functionality is
+		 * similar to the BPF_PROG_BIND_MAP syscall, but maps can be
+		 * used by the verifier during the program load. If provided,
+		 * then the fd_array[0,...,fd_array_cnt-1] is expected to be
+		 * continuous.
+		 */
+		__u32		fd_array_cnt;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5684e8ce132d..4e88797fdbeb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2730,7 +2730,7 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 }
 
 /* last field in 'union bpf_attr' used by this command */
-#define BPF_PROG_LOAD_LAST_FIELD prog_token_fd
+#define BPF_PROG_LOAD_LAST_FIELD fd_array_cnt
 
 static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8e034a22aa2a..cda02153d90e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19181,22 +19181,10 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 	return 0;
 }
 
-/* Add map behind fd to used maps list, if it's not already there, and return
- * its index.
- * Returns <0 on error, or >= 0 index, on success.
- */
-static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
+static int __add_used_map(struct bpf_verifier_env *env, struct bpf_map *map)
 {
-	CLASS(fd, f)(fd);
-	struct bpf_map *map;
 	int i, err;
 
-	map = __bpf_map_get(f);
-	if (IS_ERR(map)) {
-		verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
-		return PTR_ERR(map);
-	}
-
 	/* check whether we recorded this map already */
 	for (i = 0; i < env->used_map_cnt; i++)
 		if (env->used_maps[i] == map)
@@ -19227,6 +19215,24 @@ static int add_used_map_from_fd(struct bpf_verifier_env *env, int fd)
 	return env->used_map_cnt - 1;
 }
 
+/* Add map behind fd to used maps list, if it's not already there, and return
+ * its index.
+ * Returns <0 on error, or >= 0 index, on success.
+ */
+static int add_used_map(struct bpf_verifier_env *env, int fd)
+{
+	struct bpf_map *map;
+	CLASS(fd, f)(fd);
+
+	map = __bpf_map_get(f);
+	if (IS_ERR(map)) {
+		verbose(env, "fd %d is not pointing to valid bpf_map\n", fd);
+		return PTR_ERR(map);
+	}
+
+	return __add_used_map(env, map);
+}
+
 /* find and rewrite pseudo imm in ld_imm64 instructions:
  *
  * 1. if it accesses map FD, replace it with actual map pointer.
@@ -19318,7 +19324,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
 				break;
 			}
 
-			map_idx = add_used_map_from_fd(env, fd);
+			map_idx = add_used_map(env, fd);
 			if (map_idx < 0)
 				return map_idx;
 			map = env->used_maps[map_idx];
@@ -22526,6 +22532,65 @@ struct btf *bpf_get_btf_vmlinux(void)
 	return btf_vmlinux;
 }
 
+/*
+ * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
+ * this case expect that every file descriptor in the array is either a map or
+ * a BTF. Everything else is considered to be trash.
+ */
+static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
+{
+	struct bpf_map *map;
+	CLASS(fd, f)(fd);
+	int ret;
+
+	map = __bpf_map_get(f);
+	if (!IS_ERR(map)) {
+		ret = __add_used_map(env, map);
+		if (ret < 0)
+			return ret;
+		return 0;
+	}
+
+	/*
+	 * Unlike "unused" maps which do not appear in the BPF program,
+	 * BTFs are visible, so no reason to refcnt them now
+	 */
+	if (!IS_ERR(__btf_get_by_fd(f)))
+		return 0;
+
+	verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
+	return PTR_ERR(map);
+}
+
+static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
+{
+	size_t size = sizeof(int);
+	int ret;
+	int fd;
+	u32 i;
+
+	env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
+
+	/*
+	 * The only difference between old (no fd_array_cnt is given) and new
+	 * APIs is that in the latter case the fd_array is expected to be
+	 * continuous and is scanned for map fds right away
+	 */
+	if (!attr->fd_array_cnt)
+		return 0;
+
+	for (i = 0; i < attr->fd_array_cnt; i++) {
+		if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
+			return -EFAULT;
+
+		ret = add_fd_from_fd_array(env, fd);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size)
 {
 	u64 start_time = ktime_get_ns();
@@ -22557,7 +22622,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
 		env->insn_aux_data[i].orig_idx = i;
 	env->prog = *prog;
 	env->ops = bpf_verifier_ops[env->prog->type];
-	env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
 
 	env->allow_ptr_leaks = bpf_allow_ptr_leaks(env->prog->aux->token);
 	env->allow_uninit_stack = bpf_allow_uninit_stack(env->prog->aux->token);
@@ -22580,6 +22644,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
 	if (ret)
 		goto err_unlock;
 
+	ret = process_fd_array(env, attr, uattr);
+	if (ret)
+		goto err_release_maps;
+
 	mark_verifier_state_clean(env);
 
 	if (IS_ERR(btf_vmlinux)) {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 4162afc6b5d0..2acf9b336371 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1573,6 +1573,16 @@ union bpf_attr {
 		 * If provided, prog_flags should have BPF_F_TOKEN_FD flag set.
 		 */
 		__s32		prog_token_fd;
+		/* The fd_array_cnt can be used to pass the length of the
+		 * fd_array array. In this case all the [map] file descriptors
+		 * passed in this array will be bound to the program, even if
+		 * the maps are not referenced directly. The functionality is
+		 * similar to the BPF_PROG_BIND_MAP syscall, but maps can be
+		 * used by the verifier during the program load. If provided,
+		 * then the fd_array[0,...,fd_array_cnt-1] is expected to be
+		 * continuous.
+		 */
+		__u32		fd_array_cnt;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
-- 
2.34.1

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> of file descriptors: maps or btfs. This field was introduced as a
> sparse array. Introduce a new attribute, fd_array_cnt, which, if
> present, indicates that the fd_array is a continuous array of the
> corresponding length.
>
> If fd_array_cnt is non-zero, then every map in the fd_array will be
> bound to the program, as if it was used by the program. This
> functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> maps can be used by the verifier during the program load.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  include/uapi/linux/bpf.h       | 10 ++++
>  kernel/bpf/syscall.c           |  2 +-
>  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
>  tools/include/uapi/linux/bpf.h | 10 ++++
>  4 files changed, 104 insertions(+), 16 deletions(-)
>

[...]

> +/*
> + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> + * this case expect that every file descriptor in the array is either a map or
> + * a BTF. Everything else is considered to be trash.
> + */
> +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> +{
> +       struct bpf_map *map;
> +       CLASS(fd, f)(fd);
> +       int ret;
> +
> +       map = __bpf_map_get(f);
> +       if (!IS_ERR(map)) {
> +               ret = __add_used_map(env, map);
> +               if (ret < 0)
> +                       return ret;
> +               return 0;
> +       }
> +
> +       /*
> +        * Unlike "unused" maps which do not appear in the BPF program,
> +        * BTFs are visible, so no reason to refcnt them now

What does "BTFs are visible" mean? I find this behavior surprising,
tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
Why?

> +        */
> +       if (!IS_ERR(__btf_get_by_fd(f)))
> +               return 0;
> +
> +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> +       return PTR_ERR(map);
> +}
> +
> +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> +{
> +       size_t size = sizeof(int);
> +       int ret;
> +       int fd;
> +       u32 i;
> +
> +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> +
> +       /*
> +        * The only difference between old (no fd_array_cnt is given) and new
> +        * APIs is that in the latter case the fd_array is expected to be
> +        * continuous and is scanned for map fds right away
> +        */
> +       if (!attr->fd_array_cnt)
> +               return 0;
> +
> +       for (i = 0; i < attr->fd_array_cnt; i++) {
> +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))

potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
less than INT_MAX/4?

> +                       return -EFAULT;
> +
> +               ret = add_fd_from_fd_array(env, fd);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +

[...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > of file descriptors: maps or btfs. This field was introduced as a
> > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > present, indicates that the fd_array is a continuous array of the
> > corresponding length.
> >
> > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > bound to the program, as if it was used by the program. This
> > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > maps can be used by the verifier during the program load.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  include/uapi/linux/bpf.h       | 10 ++++
> >  kernel/bpf/syscall.c           |  2 +-
> >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> >  tools/include/uapi/linux/bpf.h | 10 ++++
> >  4 files changed, 104 insertions(+), 16 deletions(-)
> >
> 
> [...]
> 
> > +/*
> > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > + * this case expect that every file descriptor in the array is either a map or
> > + * a BTF. Everything else is considered to be trash.
> > + */
> > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > +{
> > +       struct bpf_map *map;
> > +       CLASS(fd, f)(fd);
> > +       int ret;
> > +
> > +       map = __bpf_map_get(f);
> > +       if (!IS_ERR(map)) {
> > +               ret = __add_used_map(env, map);
> > +               if (ret < 0)
> > +                       return ret;
> > +               return 0;
> > +       }
> > +
> > +       /*
> > +        * Unlike "unused" maps which do not appear in the BPF program,
> > +        * BTFs are visible, so no reason to refcnt them now
> 
> What does "BTFs are visible" mean? I find this behavior surprising,
> tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> Why?

This functionality is added to catch maps, and work with them during
verification, which aren't otherwise referenced by program code. The
actual application is those "instructions set" maps for static keys.
All other objects are "visible" during verification.

> > +        */
> > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > +               return 0;
> > +
> > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > +       return PTR_ERR(map);
> > +}
> > +
> > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > +{
> > +       size_t size = sizeof(int);
> > +       int ret;
> > +       int fd;
> > +       u32 i;
> > +
> > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > +
> > +       /*
> > +        * The only difference between old (no fd_array_cnt is given) and new
> > +        * APIs is that in the latter case the fd_array is expected to be
> > +        * continuous and is scanned for map fds right away
> > +        */
> > +       if (!attr->fd_array_cnt)
> > +               return 0;
> > +
> > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> 
> potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> less than INT_MAX/4?

Right. So, probably cap to (UINT_MAX/size)?

> > +                       return -EFAULT;
> > +
> > +               ret = add_fd_from_fd_array(env, fd);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> [...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Andrii Nakryiko]

On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > > of file descriptors: maps or btfs. This field was introduced as a
> > > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > > present, indicates that the fd_array is a continuous array of the
> > > corresponding length.
> > >
> > > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > > bound to the program, as if it was used by the program. This
> > > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > > maps can be used by the verifier during the program load.
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > >  include/uapi/linux/bpf.h       | 10 ++++
> > >  kernel/bpf/syscall.c           |  2 +-
> > >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> > >  tools/include/uapi/linux/bpf.h | 10 ++++
> > >  4 files changed, 104 insertions(+), 16 deletions(-)
> > >
> >
> > [...]
> >
> > > +/*
> > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > > + * this case expect that every file descriptor in the array is either a map or
> > > + * a BTF. Everything else is considered to be trash.
> > > + */
> > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > +{
> > > +       struct bpf_map *map;
> > > +       CLASS(fd, f)(fd);
> > > +       int ret;
> > > +
> > > +       map = __bpf_map_get(f);
> > > +       if (!IS_ERR(map)) {
> > > +               ret = __add_used_map(env, map);
> > > +               if (ret < 0)
> > > +                       return ret;
> > > +               return 0;
> > > +       }
> > > +
> > > +       /*
> > > +        * Unlike "unused" maps which do not appear in the BPF program,
> > > +        * BTFs are visible, so no reason to refcnt them now
> >
> > What does "BTFs are visible" mean? I find this behavior surprising,
> > tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> > Why?
>
> This functionality is added to catch maps, and work with them during
> verification, which aren't otherwise referenced by program code. The
> actual application is those "instructions set" maps for static keys.
> All other objects are "visible" during verification.

That's your specific intended use case, but API is semantically more
generic and shouldn't tailor to your specific interpretation on how it
will/should be used. I think this is a landmine to add reference to
just BPF maps and not to BTF objects, we won't be able to retrofit the
proper and uniform treatment later without extra flags or backwards
compatibility breakage.

Even though we don't need extra "detached" BTF objects associated with
BPF program, right now, I can anticipate some interesting use case
where we might want to attach additional BTF objects to BPF programs
(for whatever reasons, BTFs are a convenient bag of strings and
graph-based types, so could be useful for extra
debugging/metadata/whatever information).

So I can see only two ways forward. Either we disable BTFs in fd_array
if fd_array_cnt>0, which will prevent its usage from light skeleton,
so not great. Or we bump refcount both BPF maps and BTFs in fd_array.


The latter seems saner and I don't think is a problem at all, we
already have used_btfs that function similarly to used_maps.

>
> > > +        */
> > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > +               return 0;
> > > +
> > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > +       return PTR_ERR(map);
> > > +}
> > > +
> > > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > > +{
> > > +       size_t size = sizeof(int);
> > > +       int ret;
> > > +       int fd;
> > > +       u32 i;
> > > +
> > > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > > +
> > > +       /*
> > > +        * The only difference between old (no fd_array_cnt is given) and new
> > > +        * APIs is that in the latter case the fd_array is expected to be
> > > +        * continuous and is scanned for map fds right away
> > > +        */
> > > +       if (!attr->fd_array_cnt)
> > > +               return 0;
> > > +
> > > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> >
> > potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> > less than INT_MAX/4?
>
> Right. So, probably cap to (UINT_MAX/size)?

either that or use check_mul_overflow()

>
> > > +                       return -EFAULT;
> > > +
> > > +               ret = add_fd_from_fd_array(env, fd);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> >
> > [...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/12/04 10:08AM, Andrii Nakryiko wrote:
> On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > >
> > > > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > > > of file descriptors: maps or btfs. This field was introduced as a
> > > > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > > > present, indicates that the fd_array is a continuous array of the
> > > > corresponding length.
> > > >
> > > > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > > > bound to the program, as if it was used by the program. This
> > > > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > > > maps can be used by the verifier during the program load.
> > > >
> > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 10 ++++
> > > >  kernel/bpf/syscall.c           |  2 +-
> > > >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> > > >  tools/include/uapi/linux/bpf.h | 10 ++++
> > > >  4 files changed, 104 insertions(+), 16 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > +/*
> > > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > > > + * this case expect that every file descriptor in the array is either a map or
> > > > + * a BTF. Everything else is considered to be trash.
> > > > + */
> > > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > > +{
> > > > +       struct bpf_map *map;
> > > > +       CLASS(fd, f)(fd);
> > > > +       int ret;
> > > > +
> > > > +       map = __bpf_map_get(f);
> > > > +       if (!IS_ERR(map)) {
> > > > +               ret = __add_used_map(env, map);
> > > > +               if (ret < 0)
> > > > +                       return ret;
> > > > +               return 0;
> > > > +       }
> > > > +
> > > > +       /*
> > > > +        * Unlike "unused" maps which do not appear in the BPF program,
> > > > +        * BTFs are visible, so no reason to refcnt them now
> > >
> > > What does "BTFs are visible" mean? I find this behavior surprising,
> > > tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> > > Why?
> >
> > This functionality is added to catch maps, and work with them during
> > verification, which aren't otherwise referenced by program code. The
> > actual application is those "instructions set" maps for static keys.
> > All other objects are "visible" during verification.
> 
> That's your specific intended use case, but API is semantically more
> generic and shouldn't tailor to your specific interpretation on how it
> will/should be used. I think this is a landmine to add reference to
> just BPF maps and not to BTF objects, we won't be able to retrofit the
> proper and uniform treatment later without extra flags or backwards
> compatibility breakage.
> 
> Even though we don't need extra "detached" BTF objects associated with
> BPF program, right now, I can anticipate some interesting use case
> where we might want to attach additional BTF objects to BPF programs
> (for whatever reasons, BTFs are a convenient bag of strings and
> graph-based types, so could be useful for extra
> debugging/metadata/whatever information).
> 
> So I can see only two ways forward. Either we disable BTFs in fd_array
> if fd_array_cnt>0, which will prevent its usage from light skeleton,
> so not great. Or we bump refcount both BPF maps and BTFs in fd_array.
> 
> 
> The latter seems saner and I don't think is a problem at all, we
> already have used_btfs that function similarly to used_maps.

This makes total sense to treat all BPF objects in fd_array the same
way. With BTFs the problem is that, currently, a btf fd can end up
either in used_btfs or kfunc_btf_tab. I will take a look at how easy
it is to merge those two.

> >
> > > > +        */
> > > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > > +               return 0;
> > > > +
> > > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > > +       return PTR_ERR(map);
> > > > +}
> > > > +
> > > > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > > > +{
> > > > +       size_t size = sizeof(int);
> > > > +       int ret;
> > > > +       int fd;
> > > > +       u32 i;
> > > > +
> > > > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > > > +
> > > > +       /*
> > > > +        * The only difference between old (no fd_array_cnt is given) and new
> > > > +        * APIs is that in the latter case the fd_array is expected to be
> > > > +        * continuous and is scanned for map fds right away
> > > > +        */
> > > > +       if (!attr->fd_array_cnt)
> > > > +               return 0;
> > > > +
> > > > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > > > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> > >
> > > potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> > > less than INT_MAX/4?
> >
> > Right. So, probably cap to (UINT_MAX/size)?
> 
> either that or use check_mul_overflow()

Ok, will fix it, thanks.

> >
> > > > +                       return -EFAULT;
> > > > +
> > > > +               ret = add_fd_from_fd_array(env, fd);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > >
> > > [...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/12/05 08:41AM, Anton Protopopov wrote:
> On 24/12/04 10:08AM, Andrii Nakryiko wrote:
> > On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > > > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > > >
> > > > > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > > > > of file descriptors: maps or btfs. This field was introduced as a
> > > > > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > > > > present, indicates that the fd_array is a continuous array of the
> > > > > corresponding length.
> > > > >
> > > > > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > > > > bound to the program, as if it was used by the program. This
> > > > > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > > > > maps can be used by the verifier during the program load.
> > > > >
> > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > > ---
> > > > >  include/uapi/linux/bpf.h       | 10 ++++
> > > > >  kernel/bpf/syscall.c           |  2 +-
> > > > >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> > > > >  tools/include/uapi/linux/bpf.h | 10 ++++
> > > > >  4 files changed, 104 insertions(+), 16 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > +/*
> > > > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > > > > + * this case expect that every file descriptor in the array is either a map or
> > > > > + * a BTF. Everything else is considered to be trash.
> > > > > + */
> > > > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > > > +{
> > > > > +       struct bpf_map *map;
> > > > > +       CLASS(fd, f)(fd);
> > > > > +       int ret;
> > > > > +
> > > > > +       map = __bpf_map_get(f);
> > > > > +       if (!IS_ERR(map)) {
> > > > > +               ret = __add_used_map(env, map);
> > > > > +               if (ret < 0)
> > > > > +                       return ret;
> > > > > +               return 0;
> > > > > +       }
> > > > > +
> > > > > +       /*
> > > > > +        * Unlike "unused" maps which do not appear in the BPF program,
> > > > > +        * BTFs are visible, so no reason to refcnt them now
> > > >
> > > > What does "BTFs are visible" mean? I find this behavior surprising,
> > > > tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> > > > Why?
> > >
> > > This functionality is added to catch maps, and work with them during
> > > verification, which aren't otherwise referenced by program code. The
> > > actual application is those "instructions set" maps for static keys.
> > > All other objects are "visible" during verification.
> > 
> > That's your specific intended use case, but API is semantically more
> > generic and shouldn't tailor to your specific interpretation on how it
> > will/should be used. I think this is a landmine to add reference to
> > just BPF maps and not to BTF objects, we won't be able to retrofit the
> > proper and uniform treatment later without extra flags or backwards
> > compatibility breakage.
> > 
> > Even though we don't need extra "detached" BTF objects associated with
> > BPF program, right now, I can anticipate some interesting use case
> > where we might want to attach additional BTF objects to BPF programs
> > (for whatever reasons, BTFs are a convenient bag of strings and
> > graph-based types, so could be useful for extra
> > debugging/metadata/whatever information).
> > 
> > So I can see only two ways forward. Either we disable BTFs in fd_array
> > if fd_array_cnt>0, which will prevent its usage from light skeleton,
> > so not great. Or we bump refcount both BPF maps and BTFs in fd_array.
> > 
> > 
> > The latter seems saner and I don't think is a problem at all, we
> > already have used_btfs that function similarly to used_maps.
> 
> This makes total sense to treat all BPF objects in fd_array the same
> way. With BTFs the problem is that, currently, a btf fd can end up
> either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> it is to merge those two.

So, currently during program load BTFs are parsed from file
descriptors and are stored in two places: env->used_btfs and
env->prog->aux->kfunc_btf_tab:

  1) env->used_btfs populated only when a DW load with the
     (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed

  2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
     and the source is attr->fd_array[offset]. The kfunc_btf_tab is
     sorted by offset to allow faster search

So, to merge them something like this might be done:

  1) If fd_array_cnt != 0, then on load create a [sorted by offset]
     table "used_btfs", formatted similar to kfunc_btf_tab in (2)
     above.

  2) On program load change (1) to add a btf to this new sorted
     used_btfs. As there is no corresponding offset, just use
     offset=-1 (not literally like this, as bsearch() wants unique
     keys, so by offset=-1 an array of btfs, aka, old used_maps,
     should be stored)

Looks like this, conceptually, doesn't change things too much: kfuncs
btfs will still be searchable in log(n) time, the "normal" btfs will
still be searched in used_btfs in linear time.

(The other way is to just allow kfunc btfs to be loaded from fd_array
if fd_array_cnt != 0, as it is done now, but as you've mentioned
before, you had other use cases in mind, so this won't work.)

> > >
> > > > > +        */
> > > > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > > > +               return 0;
> > > > > +
> > > > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > > > +       return PTR_ERR(map);
> > > > > +}
> > > > > +
> > > > > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > > > > +{
> > > > > +       size_t size = sizeof(int);
> > > > > +       int ret;
> > > > > +       int fd;
> > > > > +       u32 i;
> > > > > +
> > > > > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > > > > +
> > > > > +       /*
> > > > > +        * The only difference between old (no fd_array_cnt is given) and new
> > > > > +        * APIs is that in the latter case the fd_array is expected to be
> > > > > +        * continuous and is scanned for map fds right away
> > > > > +        */
> > > > > +       if (!attr->fd_array_cnt)
> > > > > +               return 0;
> > > > > +
> > > > > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > > > > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> > > >
> > > > potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> > > > less than INT_MAX/4?
> > >
> > > Right. So, probably cap to (UINT_MAX/size)?
> > 
> > either that or use check_mul_overflow()
> 
> Ok, will fix it, thanks.

On the second look, there's no overflow here, as (int) * (size_t) is
expanded by C to (size_t), and argument is also (size_t).

However, maybe this is still makes sense to restrict the maximum size
of fd_array to something like (1 << 16). (The number of unique fds in
the end will be ~(MAX_USED_MAPS + MAX_USED_BTFS + MAX_KFUNC_BTFS).)

> > >
> > > > > +                       return -EFAULT;
> > > > > +
> > > > > +               ret = add_fd_from_fd_array(env, fd);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > >
> > > > [...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Alexei Starovoitov]

On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> >
> > This makes total sense to treat all BPF objects in fd_array the same
> > way. With BTFs the problem is that, currently, a btf fd can end up
> > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > it is to merge those two.
>
> So, currently during program load BTFs are parsed from file
> descriptors and are stored in two places: env->used_btfs and
> env->prog->aux->kfunc_btf_tab:
>
>   1) env->used_btfs populated only when a DW load with the
>      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
>
>   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
>      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
>      sorted by offset to allow faster search
>
> So, to merge them something like this might be done:
>
>   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
>      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
>      above.
>
>   2) On program load change (1) to add a btf to this new sorted
>      used_btfs. As there is no corresponding offset, just use
>      offset=-1 (not literally like this, as bsearch() wants unique
>      keys, so by offset=-1 an array of btfs, aka, old used_maps,
>      should be stored)
>
> Looks like this, conceptually, doesn't change things too much: kfuncs
> btfs will still be searchable in log(n) time, the "normal" btfs will
> still be searched in used_btfs in linear time.
>
> (The other way is to just allow kfunc btfs to be loaded from fd_array
> if fd_array_cnt != 0, as it is done now, but as you've mentioned
> before, you had other use cases in mind, so this won't work.)

This is getting a bit too complex.
I think Andrii is asking to keep BTFs if they are in fd_array.
No need to combine kfunc_btf_tab and used_btfs.
I think adding BTFs from fd_array to prog->aux->used_btfs
should do it.

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Andrii Nakryiko]

On Tue, Dec 10, 2024 at 7:57 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > >
> > > This makes total sense to treat all BPF objects in fd_array the same
> > > way. With BTFs the problem is that, currently, a btf fd can end up
> > > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > > it is to merge those two.
> >
> > So, currently during program load BTFs are parsed from file
> > descriptors and are stored in two places: env->used_btfs and
> > env->prog->aux->kfunc_btf_tab:
> >
> >   1) env->used_btfs populated only when a DW load with the
> >      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
> >
> >   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
> >      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
> >      sorted by offset to allow faster search
> >
> > So, to merge them something like this might be done:
> >
> >   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
> >      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
> >      above.
> >
> >   2) On program load change (1) to add a btf to this new sorted
> >      used_btfs. As there is no corresponding offset, just use
> >      offset=-1 (not literally like this, as bsearch() wants unique
> >      keys, so by offset=-1 an array of btfs, aka, old used_maps,
> >      should be stored)
> >
> > Looks like this, conceptually, doesn't change things too much: kfuncs
> > btfs will still be searchable in log(n) time, the "normal" btfs will
> > still be searched in used_btfs in linear time.
> >
> > (The other way is to just allow kfunc btfs to be loaded from fd_array
> > if fd_array_cnt != 0, as it is done now, but as you've mentioned
> > before, you had other use cases in mind, so this won't work.)
>
> This is getting a bit too complex.
> I think Andrii is asking to keep BTFs if they are in fd_array.
> No need to combine kfunc_btf_tab and used_btfs.
> I think adding BTFs from fd_array to prog->aux->used_btfs
> should do it.

Exactly, no need to do major changes, let's just add those BTFs into
used_btfs, that's all.

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/12/10 10:18AM, Andrii Nakryiko wrote:
> On Tue, Dec 10, 2024 at 7:57 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > >
> > > > This makes total sense to treat all BPF objects in fd_array the same
> > > > way. With BTFs the problem is that, currently, a btf fd can end up
> > > > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > > > it is to merge those two.
> > >
> > > So, currently during program load BTFs are parsed from file
> > > descriptors and are stored in two places: env->used_btfs and
> > > env->prog->aux->kfunc_btf_tab:
> > >
> > >   1) env->used_btfs populated only when a DW load with the
> > >      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
> > >
> > >   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
> > >      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
> > >      sorted by offset to allow faster search
> > >
> > > So, to merge them something like this might be done:
> > >
> > >   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
> > >      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
> > >      above.
> > >
> > >   2) On program load change (1) to add a btf to this new sorted
> > >      used_btfs. As there is no corresponding offset, just use
> > >      offset=-1 (not literally like this, as bsearch() wants unique
> > >      keys, so by offset=-1 an array of btfs, aka, old used_maps,
> > >      should be stored)
> > >
> > > Looks like this, conceptually, doesn't change things too much: kfuncs
> > > btfs will still be searchable in log(n) time, the "normal" btfs will
> > > still be searched in used_btfs in linear time.
> > >
> > > (The other way is to just allow kfunc btfs to be loaded from fd_array
> > > if fd_array_cnt != 0, as it is done now, but as you've mentioned
> > > before, you had other use cases in mind, so this won't work.)
> >
> > This is getting a bit too complex.
> > I think Andrii is asking to keep BTFs if they are in fd_array.
> > No need to combine kfunc_btf_tab and used_btfs.
> > I think adding BTFs from fd_array to prog->aux->used_btfs
> > should do it.
> 
> Exactly, no need to do major changes, let's just add those BTFs into
> used_btfs, that's all.

Added. However, I have a question here: how to add proper selftests? The btfs
listed in env->used_btfs are later copied to prog->aux->used_btfs, and are
never actually exposed to user-space in any way. So, one test I can think of is

  * passing a btf fd in fd_array on prog load
  * closing this btf fd and checking that id exists before closing the program
    (requires to wait until rcu sync to be sure that the btf wasn't destroyed,
    but still is refcounted)

Is this enough?

(I assume exposing used_btfs to user-space is also out of scope of this patch
set, right?)

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Andrii Nakryiko]

On Thu, Dec 12, 2024 at 9:15 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 24/12/10 10:18AM, Andrii Nakryiko wrote:
> > On Tue, Dec 10, 2024 at 7:57 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > >
> > > > >
> > > > > This makes total sense to treat all BPF objects in fd_array the same
> > > > > way. With BTFs the problem is that, currently, a btf fd can end up
> > > > > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > > > > it is to merge those two.
> > > >
> > > > So, currently during program load BTFs are parsed from file
> > > > descriptors and are stored in two places: env->used_btfs and
> > > > env->prog->aux->kfunc_btf_tab:
> > > >
> > > >   1) env->used_btfs populated only when a DW load with the
> > > >      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
> > > >
> > > >   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
> > > >      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
> > > >      sorted by offset to allow faster search
> > > >
> > > > So, to merge them something like this might be done:
> > > >
> > > >   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
> > > >      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
> > > >      above.
> > > >
> > > >   2) On program load change (1) to add a btf to this new sorted
> > > >      used_btfs. As there is no corresponding offset, just use
> > > >      offset=-1 (not literally like this, as bsearch() wants unique
> > > >      keys, so by offset=-1 an array of btfs, aka, old used_maps,
> > > >      should be stored)
> > > >
> > > > Looks like this, conceptually, doesn't change things too much: kfuncs
> > > > btfs will still be searchable in log(n) time, the "normal" btfs will
> > > > still be searched in used_btfs in linear time.
> > > >
> > > > (The other way is to just allow kfunc btfs to be loaded from fd_array
> > > > if fd_array_cnt != 0, as it is done now, but as you've mentioned
> > > > before, you had other use cases in mind, so this won't work.)
> > >
> > > This is getting a bit too complex.
> > > I think Andrii is asking to keep BTFs if they are in fd_array.
> > > No need to combine kfunc_btf_tab and used_btfs.
> > > I think adding BTFs from fd_array to prog->aux->used_btfs
> > > should do it.
> >
> > Exactly, no need to do major changes, let's just add those BTFs into
> > used_btfs, that's all.
>
> Added. However, I have a question here: how to add proper selftests? The btfs
> listed in env->used_btfs are later copied to prog->aux->used_btfs, and are
> never actually exposed to user-space in any way. So, one test I can think of is
>
>   * passing a btf fd in fd_array on prog load
>   * closing this btf fd and checking that id exists before closing the program
>     (requires to wait until rcu sync to be sure that the btf wasn't destroyed,
>     but still is refcounted)
>
> Is this enough?

Yeah, I think so, something minimal and simple should do, thanks.

>
> (I assume exposing used_btfs to user-space is also out of scope of this patch
> set, right?)

right

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Andrii Nakryiko]

On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 24/12/05 08:41AM, Anton Protopopov wrote:
> > On 24/12/04 10:08AM, Andrii Nakryiko wrote:
> > > On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > >
> > > > On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > > > > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > > > >
> > > > > > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > > > > > of file descriptors: maps or btfs. This field was introduced as a
> > > > > > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > > > > > present, indicates that the fd_array is a continuous array of the
> > > > > > corresponding length.
> > > > > >
> > > > > > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > > > > > bound to the program, as if it was used by the program. This
> > > > > > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > > > > > maps can be used by the verifier during the program load.
> > > > > >
> > > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > > > ---
> > > > > >  include/uapi/linux/bpf.h       | 10 ++++
> > > > > >  kernel/bpf/syscall.c           |  2 +-
> > > > > >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> > > > > >  tools/include/uapi/linux/bpf.h | 10 ++++
> > > > > >  4 files changed, 104 insertions(+), 16 deletions(-)
> > > > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > > +/*
> > > > > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > > > > > + * this case expect that every file descriptor in the array is either a map or
> > > > > > + * a BTF. Everything else is considered to be trash.
> > > > > > + */
> > > > > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > > > > +{
> > > > > > +       struct bpf_map *map;
> > > > > > +       CLASS(fd, f)(fd);
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       map = __bpf_map_get(f);
> > > > > > +       if (!IS_ERR(map)) {
> > > > > > +               ret = __add_used_map(env, map);
> > > > > > +               if (ret < 0)
> > > > > > +                       return ret;
> > > > > > +               return 0;
> > > > > > +       }
> > > > > > +
> > > > > > +       /*
> > > > > > +        * Unlike "unused" maps which do not appear in the BPF program,
> > > > > > +        * BTFs are visible, so no reason to refcnt them now
> > > > >
> > > > > What does "BTFs are visible" mean? I find this behavior surprising,
> > > > > tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> > > > > Why?
> > > >
> > > > This functionality is added to catch maps, and work with them during
> > > > verification, which aren't otherwise referenced by program code. The
> > > > actual application is those "instructions set" maps for static keys.
> > > > All other objects are "visible" during verification.
> > >
> > > That's your specific intended use case, but API is semantically more
> > > generic and shouldn't tailor to your specific interpretation on how it
> > > will/should be used. I think this is a landmine to add reference to
> > > just BPF maps and not to BTF objects, we won't be able to retrofit the
> > > proper and uniform treatment later without extra flags or backwards
> > > compatibility breakage.
> > >
> > > Even though we don't need extra "detached" BTF objects associated with
> > > BPF program, right now, I can anticipate some interesting use case
> > > where we might want to attach additional BTF objects to BPF programs
> > > (for whatever reasons, BTFs are a convenient bag of strings and
> > > graph-based types, so could be useful for extra
> > > debugging/metadata/whatever information).
> > >
> > > So I can see only two ways forward. Either we disable BTFs in fd_array
> > > if fd_array_cnt>0, which will prevent its usage from light skeleton,
> > > so not great. Or we bump refcount both BPF maps and BTFs in fd_array.
> > >
> > >
> > > The latter seems saner and I don't think is a problem at all, we
> > > already have used_btfs that function similarly to used_maps.
> >
> > This makes total sense to treat all BPF objects in fd_array the same
> > way. With BTFs the problem is that, currently, a btf fd can end up
> > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > it is to merge those two.
>
> So, currently during program load BTFs are parsed from file
> descriptors and are stored in two places: env->used_btfs and
> env->prog->aux->kfunc_btf_tab:
>
>   1) env->used_btfs populated only when a DW load with the
>      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
>
>   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
>      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
>      sorted by offset to allow faster search
>
> So, to merge them something like this might be done:
>
>   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
>      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
>      above.
>
>   2) On program load change (1) to add a btf to this new sorted
>      used_btfs. As there is no corresponding offset, just use
>      offset=-1 (not literally like this, as bsearch() wants unique
>      keys, so by offset=-1 an array of btfs, aka, old used_maps,
>      should be stored)
>
> Looks like this, conceptually, doesn't change things too much: kfuncs
> btfs will still be searchable in log(n) time, the "normal" btfs will
> still be searched in used_btfs in linear time.
>
> (The other way is to just allow kfunc btfs to be loaded from fd_array
> if fd_array_cnt != 0, as it is done now, but as you've mentioned
> before, you had other use cases in mind, so this won't work.)
>
> > > >
> > > > > > +        */
> > > > > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > > > > +       return PTR_ERR(map);
> > > > > > +}
> > > > > > +
> > > > > > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > > > > > +{
> > > > > > +       size_t size = sizeof(int);
> > > > > > +       int ret;
> > > > > > +       int fd;
> > > > > > +       u32 i;
> > > > > > +
> > > > > > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > > > > > +
> > > > > > +       /*
> > > > > > +        * The only difference between old (no fd_array_cnt is given) and new
> > > > > > +        * APIs is that in the latter case the fd_array is expected to be
> > > > > > +        * continuous and is scanned for map fds right away
> > > > > > +        */
> > > > > > +       if (!attr->fd_array_cnt)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > > > > > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> > > > >
> > > > > potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> > > > > less than INT_MAX/4?
> > > >
> > > > Right. So, probably cap to (UINT_MAX/size)?
> > >
> > > either that or use check_mul_overflow()
> >
> > Ok, will fix it, thanks.
>
> On the second look, there's no overflow here, as (int) * (size_t) is
> expanded by C to (size_t), and argument is also (size_t).

What about 32-bit architectures? 64-bit ones are not a problem, of course.

>
> However, maybe this is still makes sense to restrict the maximum size
> of fd_array to something like (1 << 16). (The number of unique fds in
> the end will be ~(MAX_USED_MAPS + MAX_USED_BTFS + MAX_KFUNC_BTFS).)
>
> > > >
> > > > > > +                       return -EFAULT;
> > > > > > +
> > > > > > +               ret = add_fd_from_fd_array(env, fd);
> > > > > > +               if (ret)
> > > > > > +                       return ret;
> > > > > > +       }
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > [...]

Re: [PATCH v4 bpf-next 3/7] bpf: add fd_array_cnt attribute for prog_load

[Anton Protopopov]

On 24/12/10 10:19AM, Andrii Nakryiko wrote:
> On Tue, Dec 10, 2024 at 12:56 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > On 24/12/05 08:41AM, Anton Protopopov wrote:
> > > On 24/12/04 10:08AM, Andrii Nakryiko wrote:
> > > > On Wed, Dec 4, 2024 at 4:19 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > > >
> > > > > On 24/12/03 01:25PM, Andrii Nakryiko wrote:
> > > > > > On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > > > > > >
> > > > > > > The fd_array attribute of the BPF_PROG_LOAD syscall may contain a set
> > > > > > > of file descriptors: maps or btfs. This field was introduced as a
> > > > > > > sparse array. Introduce a new attribute, fd_array_cnt, which, if
> > > > > > > present, indicates that the fd_array is a continuous array of the
> > > > > > > corresponding length.
> > > > > > >
> > > > > > > If fd_array_cnt is non-zero, then every map in the fd_array will be
> > > > > > > bound to the program, as if it was used by the program. This
> > > > > > > functionality is similar to the BPF_PROG_BIND_MAP syscall, but such
> > > > > > > maps can be used by the verifier during the program load.
> > > > > > >
> > > > > > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > > > > > ---
> > > > > > >  include/uapi/linux/bpf.h       | 10 ++++
> > > > > > >  kernel/bpf/syscall.c           |  2 +-
> > > > > > >  kernel/bpf/verifier.c          | 98 ++++++++++++++++++++++++++++------
> > > > > > >  tools/include/uapi/linux/bpf.h | 10 ++++
> > > > > > >  4 files changed, 104 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > +/*
> > > > > > > + * The add_fd_from_fd_array() is executed only if fd_array_cnt is non-zero. In
> > > > > > > + * this case expect that every file descriptor in the array is either a map or
> > > > > > > + * a BTF. Everything else is considered to be trash.
> > > > > > > + */
> > > > > > > +static int add_fd_from_fd_array(struct bpf_verifier_env *env, int fd)
> > > > > > > +{
> > > > > > > +       struct bpf_map *map;
> > > > > > > +       CLASS(fd, f)(fd);
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       map = __bpf_map_get(f);
> > > > > > > +       if (!IS_ERR(map)) {
> > > > > > > +               ret = __add_used_map(env, map);
> > > > > > > +               if (ret < 0)
> > > > > > > +                       return ret;
> > > > > > > +               return 0;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * Unlike "unused" maps which do not appear in the BPF program,
> > > > > > > +        * BTFs are visible, so no reason to refcnt them now
> > > > > >
> > > > > > What does "BTFs are visible" mean? I find this behavior surprising,
> > > > > > tbh. Map is added to used_maps, but BTF is *not* added to used_btfs?
> > > > > > Why?
> > > > >
> > > > > This functionality is added to catch maps, and work with them during
> > > > > verification, which aren't otherwise referenced by program code. The
> > > > > actual application is those "instructions set" maps for static keys.
> > > > > All other objects are "visible" during verification.
> > > >
> > > > That's your specific intended use case, but API is semantically more
> > > > generic and shouldn't tailor to your specific interpretation on how it
> > > > will/should be used. I think this is a landmine to add reference to
> > > > just BPF maps and not to BTF objects, we won't be able to retrofit the
> > > > proper and uniform treatment later without extra flags or backwards
> > > > compatibility breakage.
> > > >
> > > > Even though we don't need extra "detached" BTF objects associated with
> > > > BPF program, right now, I can anticipate some interesting use case
> > > > where we might want to attach additional BTF objects to BPF programs
> > > > (for whatever reasons, BTFs are a convenient bag of strings and
> > > > graph-based types, so could be useful for extra
> > > > debugging/metadata/whatever information).
> > > >
> > > > So I can see only two ways forward. Either we disable BTFs in fd_array
> > > > if fd_array_cnt>0, which will prevent its usage from light skeleton,
> > > > so not great. Or we bump refcount both BPF maps and BTFs in fd_array.
> > > >
> > > >
> > > > The latter seems saner and I don't think is a problem at all, we
> > > > already have used_btfs that function similarly to used_maps.
> > >
> > > This makes total sense to treat all BPF objects in fd_array the same
> > > way. With BTFs the problem is that, currently, a btf fd can end up
> > > either in used_btfs or kfunc_btf_tab. I will take a look at how easy
> > > it is to merge those two.
> >
> > So, currently during program load BTFs are parsed from file
> > descriptors and are stored in two places: env->used_btfs and
> > env->prog->aux->kfunc_btf_tab:
> >
> >   1) env->used_btfs populated only when a DW load with the
> >      (src_reg == BPF_PSEUDO_BTF_ID) flag set is performed
> >
> >   2) kfunc_btf_tab is populated by __find_kfunc_desc_btf(),
> >      and the source is attr->fd_array[offset]. The kfunc_btf_tab is
> >      sorted by offset to allow faster search
> >
> > So, to merge them something like this might be done:
> >
> >   1) If fd_array_cnt != 0, then on load create a [sorted by offset]
> >      table "used_btfs", formatted similar to kfunc_btf_tab in (2)
> >      above.
> >
> >   2) On program load change (1) to add a btf to this new sorted
> >      used_btfs. As there is no corresponding offset, just use
> >      offset=-1 (not literally like this, as bsearch() wants unique
> >      keys, so by offset=-1 an array of btfs, aka, old used_maps,
> >      should be stored)
> >
> > Looks like this, conceptually, doesn't change things too much: kfuncs
> > btfs will still be searchable in log(n) time, the "normal" btfs will
> > still be searched in used_btfs in linear time.
> >
> > (The other way is to just allow kfunc btfs to be loaded from fd_array
> > if fd_array_cnt != 0, as it is done now, but as you've mentioned
> > before, you had other use cases in mind, so this won't work.)
> >
> > > > >
> > > > > > > +        */
> > > > > > > +       if (!IS_ERR(__btf_get_by_fd(f)))
> > > > > > > +               return 0;
> > > > > > > +
> > > > > > > +       verbose(env, "fd %d is not pointing to valid bpf_map or btf\n", fd);
> > > > > > > +       return PTR_ERR(map);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int process_fd_array(struct bpf_verifier_env *env, union bpf_attr *attr, bpfptr_t uattr)
> > > > > > > +{
> > > > > > > +       size_t size = sizeof(int);
> > > > > > > +       int ret;
> > > > > > > +       int fd;
> > > > > > > +       u32 i;
> > > > > > > +
> > > > > > > +       env->fd_array = make_bpfptr(attr->fd_array, uattr.is_kernel);
> > > > > > > +
> > > > > > > +       /*
> > > > > > > +        * The only difference between old (no fd_array_cnt is given) and new
> > > > > > > +        * APIs is that in the latter case the fd_array is expected to be
> > > > > > > +        * continuous and is scanned for map fds right away
> > > > > > > +        */
> > > > > > > +       if (!attr->fd_array_cnt)
> > > > > > > +               return 0;
> > > > > > > +
> > > > > > > +       for (i = 0; i < attr->fd_array_cnt; i++) {
> > > > > > > +               if (copy_from_bpfptr_offset(&fd, env->fd_array, i * size, size))
> > > > > >
> > > > > > potential overflow in `i * size`? Do we limit fd_array_cnt anywhere to
> > > > > > less than INT_MAX/4?
> > > > >
> > > > > Right. So, probably cap to (UINT_MAX/size)?
> > > >
> > > > either that or use check_mul_overflow()
> > >
> > > Ok, will fix it, thanks.
> >
> > On the second look, there's no overflow here, as (int) * (size_t) is
> > expanded by C to (size_t), and argument is also (size_t).
> 
> What about 32-bit architectures? 64-bit ones are not a problem, of course.

Yes, sure, thanks. I added the (U32_MAX/size) limit.

BTW, the resolve_pseudo_ldimm64() also does 

        if (copy_from_bpfptr_offset(&fd,
                                    env->fd_array,
                                    insn[0].imm * sizeof(fd),
                                    sizeof(fd)))

I don't see that insn[0].imm is checked at any place,
or am I wrong?

> > However, maybe this is still makes sense to restrict the maximum size
> > of fd_array to something like (1 << 16). (The number of unique fds in
> > the end will be ~(MAX_USED_MAPS + MAX_USED_BTFS + MAX_KFUNC_BTFS).)
> >
> > > > >
> > > > > > > +                       return -EFAULT;
> > > > > > > +
> > > > > > > +               ret = add_fd_from_fd_array(env, fd);
> > > > > > > +               if (ret)
> > > > > > > +                       return ret;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       return 0;
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > [...]

[PATCH v4 bpf-next 4/7] libbpf: prog load: allow to use fd_array_cnt

[Anton Protopopov]

Add new fd_array_cnt field to bpf_prog_load_opts
and pass it in bpf_attr, if set.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 tools/lib/bpf/bpf.c | 3 ++-
 tools/lib/bpf/bpf.h | 5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index becdfa701c75..359f73ead613 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -238,7 +238,7 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
 		  const struct bpf_insn *insns, size_t insn_cnt,
 		  struct bpf_prog_load_opts *opts)
 {
-	const size_t attr_sz = offsetofend(union bpf_attr, prog_token_fd);
+	const size_t attr_sz = offsetofend(union bpf_attr, fd_array_cnt);
 	void *finfo = NULL, *linfo = NULL;
 	const char *func_info, *line_info;
 	__u32 log_size, log_level, attach_prog_fd, attach_btf_obj_fd;
@@ -311,6 +311,7 @@ int bpf_prog_load(enum bpf_prog_type prog_type,
 	attr.line_info_cnt = OPTS_GET(opts, line_info_cnt, 0);
 
 	attr.fd_array = ptr_to_u64(OPTS_GET(opts, fd_array, NULL));
+	attr.fd_array_cnt = OPTS_GET(opts, fd_array_cnt, 0);
 
 	if (log_level) {
 		attr.log_buf = ptr_to_u64(log_buf);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index a4a7b1ad1b63..435da95d2058 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -107,9 +107,12 @@ struct bpf_prog_load_opts {
 	 */
 	__u32 log_true_size;
 	__u32 token_fd;
+
+	/* if set, provides the length of fd_array */
+	__u32 fd_array_cnt;
 	size_t :0;
 };
-#define bpf_prog_load_opts__last_field token_fd
+#define bpf_prog_load_opts__last_field fd_array_cnt
 
 LIBBPF_API int bpf_prog_load(enum bpf_prog_type prog_type,
 			     const char *prog_name, const char *license,
-- 
2.34.1

Re: [PATCH v4 bpf-next 4/7] libbpf: prog load: allow to use fd_array_cnt

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 5:48 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> Add new fd_array_cnt field to bpf_prog_load_opts
> and pass it in bpf_attr, if set.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  tools/lib/bpf/bpf.c | 3 ++-
>  tools/lib/bpf/bpf.h | 5 ++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>

LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>


[...]

[PATCH v4 bpf-next 5/7] selftests/bpf: Add tests for fd_array_cnt

[Anton Protopopov]

Add a new set of tests to test the new field in PROG_LOAD-related
part of bpf_attr: fd_array_cnt.

Add the following test cases:

  * fd_array_cnt/no-fd-array: program is loaded in a normal
    way, without any fd_array present

  * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
    check that they're bound to the program

  * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
    only two of which are unique

  * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
    fd_array which is also referenced from within the program

  * fd_array_cnt/fd-array-trash-input: pass array with some trash

  * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)

  * fd_array_cnt/fd-array-2big: pass too large array

All the tests above are using the bpf(2) syscall directly,
no libbpf involved.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
---
 .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
 1 file changed, 340 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c

diff --git a/tools/testing/selftests/bpf/prog_tests/fd_array.c b/tools/testing/selftests/bpf/prog_tests/fd_array.c
new file mode 100644
index 000000000000..1d4bff4a1269
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/fd_array.c
@@ -0,0 +1,340 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#include <linux/btf.h>
+#include <bpf/bpf.h>
+
+#include "../test_btf.h"
+
+static inline int new_map(void)
+{
+	LIBBPF_OPTS(bpf_map_create_opts, opts);
+	const char *name = NULL;
+	__u32 max_entries = 1;
+	__u32 value_size = 8;
+	__u32 key_size = 4;
+
+	return bpf_map_create(BPF_MAP_TYPE_ARRAY, name,
+			      key_size, value_size,
+			      max_entries, &opts);
+}
+
+static int new_btf(void)
+{
+	LIBBPF_OPTS(bpf_btf_load_opts, opts);
+	struct btf_blob {
+		struct btf_header btf_hdr;
+		__u32 types[8];
+		__u32 str;
+	} raw_btf = {
+		.btf_hdr = {
+			.magic = BTF_MAGIC,
+			.version = BTF_VERSION,
+			.hdr_len = sizeof(struct btf_header),
+			.type_len = sizeof(raw_btf.types),
+			.str_off = offsetof(struct btf_blob, str) - offsetof(struct btf_blob, types),
+			.str_len = sizeof(raw_btf.str),
+		},
+		.types = {
+			/* long */
+			BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
+			/* unsigned long */
+			BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
+		},
+	};
+
+	return bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);
+}
+
+static bool map_exists(__u32 id)
+{
+	int fd;
+
+	fd = bpf_map_get_fd_by_id(id);
+	if (fd >= 0) {
+		close(fd);
+		return true;
+	}
+	return false;
+}
+
+static inline int bpf_prog_get_map_ids(int prog_fd, __u32 *nr_map_ids, __u32 *map_ids)
+{
+	__u32 len = sizeof(struct bpf_prog_info);
+	struct bpf_prog_info info = {
+		.nr_map_ids = *nr_map_ids,
+		.map_ids = ptr_to_u64(map_ids),
+	};
+	int err;
+
+	err = bpf_prog_get_info_by_fd(prog_fd, &info, &len);
+	if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
+		return -1;
+
+	*nr_map_ids = info.nr_map_ids;
+
+	return 0;
+}
+
+static int __load_test_prog(int map_fd, const int *fd_array, int fd_array_cnt)
+{
+	/* A trivial program which uses one map */
+	struct bpf_insn insns[] = {
+		BPF_LD_MAP_FD(BPF_REG_1, map_fd),
+		BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	LIBBPF_OPTS(bpf_prog_load_opts, opts);
+
+	opts.fd_array = fd_array;
+	opts.fd_array_cnt = fd_array_cnt;
+
+	return bpf_prog_load(BPF_PROG_TYPE_XDP, NULL, "GPL", insns, ARRAY_SIZE(insns), &opts);
+}
+
+static int load_test_prog(const int *fd_array, int fd_array_cnt)
+{
+	int map_fd;
+	int ret;
+
+	map_fd = new_map();
+	if (!ASSERT_GE(map_fd, 0, "new_map"))
+		return map_fd;
+
+	ret = __load_test_prog(map_fd, fd_array, fd_array_cnt);
+	close(map_fd);
+
+	/* switch back to returning the actual value */
+	if (ret < 0)
+		return -errno;
+	return ret;
+}
+
+static bool check_expected_map_ids(int prog_fd, int expected, __u32 *map_ids, __u32 *nr_map_ids)
+{
+	int err;
+
+	err = bpf_prog_get_map_ids(prog_fd, nr_map_ids, map_ids);
+	if (!ASSERT_OK(err, "bpf_prog_get_map_ids"))
+		return false;
+	if (!ASSERT_EQ(*nr_map_ids, expected, "unexpected nr_map_ids"))
+		return false;
+
+	return true;
+}
+
+/*
+ * Load a program, which uses one map. No fd_array maps are present.
+ * On return only one map is expected to be bound to prog.
+ */
+static void check_fd_array_cnt__no_fd_array(void)
+{
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd = -1;
+
+	prog_fd = load_test_prog(NULL, 0);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		return;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids);
+	close(prog_fd);
+}
+
+/*
+ * Load a program, which uses one map, and pass two extra, non-equal, maps in
+ * fd_array with fd_array_cnt=2. On return three maps are expected to be bound
+ * to the program.
+ */
+static void check_fd_array_cnt__fd_array_ok(void)
+{
+	int extra_fds[2] = { -1, -1 };
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd = -1;
+
+	extra_fds[0] = new_map();
+	if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
+		goto cleanup;
+	extra_fds[1] = new_map();
+	if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
+		goto cleanup;
+	prog_fd = load_test_prog(extra_fds, 2);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		goto cleanup;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
+		goto cleanup;
+
+	/* maps should still exist when original file descriptors are closed */
+	close(extra_fds[0]);
+	close(extra_fds[1]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
+		goto cleanup;
+	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
+		goto cleanup;
+
+	/* some fds might be invalid, so ignore return codes */
+cleanup:
+	close(extra_fds[1]);
+	close(extra_fds[0]);
+	close(prog_fd);
+}
+
+/*
+ * Load a program with a few extra maps duplicated in the fd_array.
+ * After the load maps should only be referenced once.
+ */
+static void check_fd_array_cnt__duplicated_maps(void)
+{
+	int extra_fds[4] = { -1, -1, -1, -1 };
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd = -1;
+
+	extra_fds[0] = extra_fds[2] = new_map();
+	if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
+		goto cleanup;
+	extra_fds[1] = extra_fds[3] = new_map();
+	if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
+		goto cleanup;
+	prog_fd = load_test_prog(extra_fds, 4);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		goto cleanup;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
+		goto cleanup;
+
+	/* maps should still exist when original file descriptors are closed */
+	close(extra_fds[0]);
+	close(extra_fds[1]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
+		goto cleanup;
+	if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map should exist"))
+		goto cleanup;
+
+	/* some fds might be invalid, so ignore return codes */
+cleanup:
+	close(extra_fds[1]);
+	close(extra_fds[0]);
+	close(prog_fd);
+}
+
+/*
+ * Check that if maps which are referenced by a program are
+ * passed in fd_array, then they will be referenced only once
+ */
+static void check_fd_array_cnt__referenced_maps_in_fd_array(void)
+{
+	int extra_fds[1] = { -1 };
+	__u32 map_ids[16];
+	__u32 nr_map_ids;
+	int prog_fd = -1;
+
+	extra_fds[0] = new_map();
+	if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
+		goto cleanup;
+	prog_fd = __load_test_prog(extra_fds[0], extra_fds, 1);
+	if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
+		goto cleanup;
+	nr_map_ids = ARRAY_SIZE(map_ids);
+	if (!check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids))
+		goto cleanup;
+
+	/* map should still exist when original file descriptor is closed */
+	close(extra_fds[0]);
+	if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
+		goto cleanup;
+
+	/* some fds might be invalid, so ignore return codes */
+cleanup:
+	close(extra_fds[0]);
+	close(prog_fd);
+}
+
+/*
+ * Test that a program with trash in fd_array can't be loaded:
+ * only map and BTF file descriptors should be accepted.
+ */
+static void check_fd_array_cnt__fd_array_with_trash(void)
+{
+	int extra_fds[3] = { -1, -1, -1 };
+	int prog_fd = -1;
+
+	extra_fds[0] = new_map();
+	if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
+		goto cleanup;
+	extra_fds[1] = new_btf();
+	if (!ASSERT_GE(extra_fds[1], 0, "new_btf"))
+		goto cleanup;
+
+	/* trash 1: not a file descriptor */
+	extra_fds[2] = 0xbeef;
+	prog_fd = load_test_prog(extra_fds, 3);
+	if (!ASSERT_EQ(prog_fd, -EBADF, "prog should have been rejected with -EBADF"))
+		goto cleanup;
+
+	/* trash 2: not a map or btf */
+	extra_fds[2] = socket(AF_INET, SOCK_STREAM, 0);
+	if (!ASSERT_GE(extra_fds[2], 0, "socket"))
+		goto cleanup;
+
+	prog_fd = load_test_prog(extra_fds, 3);
+	if (!ASSERT_EQ(prog_fd, -EINVAL, "prog should have been rejected with -EINVAL"))
+		goto cleanup;
+
+	/* some fds might be invalid, so ignore return codes */
+cleanup:
+	close(extra_fds[2]);
+	close(extra_fds[1]);
+	close(extra_fds[0]);
+}
+
+/*
+ * Test that a program with too big fd_array can't be loaded.
+ */
+static void check_fd_array_cnt__fd_array_too_big(void)
+{
+	int extra_fds[65];
+	int prog_fd = -1;
+	int i;
+
+	for (i = 0; i < 65; i++) {
+		extra_fds[i] = new_map();
+		if (!ASSERT_GE(extra_fds[i], 0, "new_map"))
+			goto cleanup_fds;
+	}
+
+	prog_fd = load_test_prog(extra_fds, 65);
+	ASSERT_EQ(prog_fd, -E2BIG, "prog should have been rejected with -E2BIG");
+
+cleanup_fds:
+	while (i > 0)
+		close(extra_fds[--i]);
+}
+
+void test_fd_array_cnt(void)
+{
+	if (test__start_subtest("no-fd-array"))
+		check_fd_array_cnt__no_fd_array();
+
+	if (test__start_subtest("fd-array-ok"))
+		check_fd_array_cnt__fd_array_ok();
+
+	if (test__start_subtest("fd-array-dup-input"))
+		check_fd_array_cnt__duplicated_maps();
+
+	if (test__start_subtest("fd-array-ref-maps-in-array"))
+		check_fd_array_cnt__referenced_maps_in_fd_array();
+
+	if (test__start_subtest("fd-array-trash-input"))
+		check_fd_array_cnt__fd_array_with_trash();
+
+	if (test__start_subtest("fd-array-2big"))
+		check_fd_array_cnt__fd_array_too_big();
+}
-- 
2.34.1

Re: [PATCH v4 bpf-next 5/7] selftests/bpf: Add tests for fd_array_cnt

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 6:13 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> Add a new set of tests to test the new field in PROG_LOAD-related
> part of bpf_attr: fd_array_cnt.
>
> Add the following test cases:
>
>   * fd_array_cnt/no-fd-array: program is loaded in a normal
>     way, without any fd_array present
>
>   * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
>     check that they're bound to the program
>
>   * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
>     only two of which are unique
>
>   * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
>     fd_array which is also referenced from within the program
>
>   * fd_array_cnt/fd-array-trash-input: pass array with some trash
>
>   * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)

nit: should be removed, there is no such test anymore

>
>   * fd_array_cnt/fd-array-2big: pass too large array
>
> All the tests above are using the bpf(2) syscall directly,
> no libbpf involved.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> ---
>  .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
>  1 file changed, 340 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fd_array.c b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> new file mode 100644
> index 000000000000..1d4bff4a1269
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> @@ -0,0 +1,340 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +
> +#include <linux/btf.h>
> +#include <bpf/bpf.h>
> +
> +#include "../test_btf.h"
> +
> +static inline int new_map(void)
> +{
> +       LIBBPF_OPTS(bpf_map_create_opts, opts);
> +       const char *name = NULL;
> +       __u32 max_entries = 1;
> +       __u32 value_size = 8;
> +       __u32 key_size = 4;
> +
> +       return bpf_map_create(BPF_MAP_TYPE_ARRAY, name,
> +                             key_size, value_size,
> +                             max_entries, &opts);

nit: you don't really need to pass empty opts, passing NULL is always
ok if no options are specified

> +}
> +
> +static int new_btf(void)
> +{
> +       LIBBPF_OPTS(bpf_btf_load_opts, opts);
> +       struct btf_blob {
> +               struct btf_header btf_hdr;
> +               __u32 types[8];
> +               __u32 str;
> +       } raw_btf = {
> +               .btf_hdr = {
> +                       .magic = BTF_MAGIC,
> +                       .version = BTF_VERSION,
> +                       .hdr_len = sizeof(struct btf_header),
> +                       .type_len = sizeof(raw_btf.types),
> +                       .str_off = offsetof(struct btf_blob, str) - offsetof(struct btf_blob, types),
> +                       .str_len = sizeof(raw_btf.str),
> +               },
> +               .types = {
> +                       /* long */
> +                       BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
> +                       /* unsigned long */
> +                       BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
> +               },
> +       };
> +
> +       return bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);

same, you don't seem to actually use opts

> +}
> +
> +static bool map_exists(__u32 id)
> +{
> +       int fd;
> +
> +       fd = bpf_map_get_fd_by_id(id);
> +       if (fd >= 0) {
> +               close(fd);
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static inline int bpf_prog_get_map_ids(int prog_fd, __u32 *nr_map_ids, __u32 *map_ids)
> +{
> +       __u32 len = sizeof(struct bpf_prog_info);
> +       struct bpf_prog_info info = {
> +               .nr_map_ids = *nr_map_ids,
> +               .map_ids = ptr_to_u64(map_ids),
> +       };

nit: bpf_prog_info should be explicitly memset(0), and only then
fields should be filled out. It might be ok right now because we don't
have any padding (or compiler does zero that padding out, even though
it's not required to do that), but this might pop up later, so best to
avoid that.

> +       int err;
> +
> +       err = bpf_prog_get_info_by_fd(prog_fd, &info, &len);
> +       if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
> +               return -1;
> +
> +       *nr_map_ids = info.nr_map_ids;
> +
> +       return 0;
> +}
> +
> +static int __load_test_prog(int map_fd, const int *fd_array, int fd_array_cnt)
> +{
> +       /* A trivial program which uses one map */
> +       struct bpf_insn insns[] = {
> +               BPF_LD_MAP_FD(BPF_REG_1, map_fd),
> +               BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> +               BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +               BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> +               BPF_MOV64_IMM(BPF_REG_0, 0),
> +               BPF_EXIT_INSN(),
> +       };
> +       LIBBPF_OPTS(bpf_prog_load_opts, opts);
> +
> +       opts.fd_array = fd_array;
> +       opts.fd_array_cnt = fd_array_cnt;
> +
> +       return bpf_prog_load(BPF_PROG_TYPE_XDP, NULL, "GPL", insns, ARRAY_SIZE(insns), &opts);
> +}
> +
> +static int load_test_prog(const int *fd_array, int fd_array_cnt)
> +{
> +       int map_fd;
> +       int ret;
> +
> +       map_fd = new_map();
> +       if (!ASSERT_GE(map_fd, 0, "new_map"))
> +               return map_fd;
> +
> +       ret = __load_test_prog(map_fd, fd_array, fd_array_cnt);
> +       close(map_fd);
> +
> +       /* switch back to returning the actual value */
> +       if (ret < 0)
> +               return -errno;

this errno might have been modified by close(), but you actually don't
need errno, libbpf will return errno directly from bpf_prog_load(), so
you can just do:

ret = __load_test_prog(...);
close(map_fd);
return ret;

> +       return ret;
> +}
> +
> +static bool check_expected_map_ids(int prog_fd, int expected, __u32 *map_ids, __u32 *nr_map_ids)
> +{
> +       int err;
> +
> +       err = bpf_prog_get_map_ids(prog_fd, nr_map_ids, map_ids);
> +       if (!ASSERT_OK(err, "bpf_prog_get_map_ids"))
> +               return false;
> +       if (!ASSERT_EQ(*nr_map_ids, expected, "unexpected nr_map_ids"))
> +               return false;
> +
> +       return true;
> +}
> +
> +/*
> + * Load a program, which uses one map. No fd_array maps are present.
> + * On return only one map is expected to be bound to prog.
> + */
> +static void check_fd_array_cnt__no_fd_array(void)
> +{
> +       __u32 map_ids[16];
> +       __u32 nr_map_ids;
> +       int prog_fd = -1;
> +
> +       prog_fd = load_test_prog(NULL, 0);
> +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> +               return;
> +       nr_map_ids = ARRAY_SIZE(map_ids);
> +       check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids);
> +       close(prog_fd);
> +}
> +
> +/*
> + * Load a program, which uses one map, and pass two extra, non-equal, maps in
> + * fd_array with fd_array_cnt=2. On return three maps are expected to be bound
> + * to the program.
> + */
> +static void check_fd_array_cnt__fd_array_ok(void)
> +{
> +       int extra_fds[2] = { -1, -1 };
> +       __u32 map_ids[16];
> +       __u32 nr_map_ids;
> +       int prog_fd = -1;
> +
> +       extra_fds[0] = new_map();
> +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> +               goto cleanup;
> +       extra_fds[1] = new_map();
> +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> +               goto cleanup;
> +       prog_fd = load_test_prog(extra_fds, 2);
> +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> +               goto cleanup;
> +       nr_map_ids = ARRAY_SIZE(map_ids);
> +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> +               goto cleanup;
> +
> +       /* maps should still exist when original file descriptors are closed */
> +       close(extra_fds[0]);
> +       close(extra_fds[1]);
> +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
> +               goto cleanup;
> +
> +       /* some fds might be invalid, so ignore return codes */
> +cleanup:
> +       close(extra_fds[1]);
> +       close(extra_fds[0]);
> +       close(prog_fd);

nit: technically, you should check each fd to be >= 0 before closing it

> +}
> +
> +/*
> + * Load a program with a few extra maps duplicated in the fd_array.
> + * After the load maps should only be referenced once.
> + */
> +static void check_fd_array_cnt__duplicated_maps(void)
> +{
> +       int extra_fds[4] = { -1, -1, -1, -1 };
> +       __u32 map_ids[16];
> +       __u32 nr_map_ids;
> +       int prog_fd = -1;
> +
> +       extra_fds[0] = extra_fds[2] = new_map();
> +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> +               goto cleanup;
> +       extra_fds[1] = extra_fds[3] = new_map();
> +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> +               goto cleanup;
> +       prog_fd = load_test_prog(extra_fds, 4);
> +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> +               goto cleanup;
> +       nr_map_ids = ARRAY_SIZE(map_ids);
> +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> +               goto cleanup;
> +
> +       /* maps should still exist when original file descriptors are closed */
> +       close(extra_fds[0]);
> +       close(extra_fds[1]);
> +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> +               goto cleanup;
> +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map should exist"))
> +               goto cleanup;
> +
> +       /* some fds might be invalid, so ignore return codes */
> +cleanup:
> +       close(extra_fds[1]);
> +       close(extra_fds[0]);
> +       close(prog_fd);

same about if (fd >=0) close(fd); pattern

> +}
> +
> +/*
> + * Check that if maps which are referenced by a program are
> + * passed in fd_array, then they will be referenced only once
> + */
> +static void check_fd_array_cnt__referenced_maps_in_fd_array(void)
> +{
> +       int extra_fds[1] = { -1 };
> +       __u32 map_ids[16];
> +       __u32 nr_map_ids;
> +       int prog_fd = -1;
> +
> +       extra_fds[0] = new_map();
> +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> +               goto cleanup;
> +       prog_fd = __load_test_prog(extra_fds[0], extra_fds, 1);
> +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> +               goto cleanup;
> +       nr_map_ids = ARRAY_SIZE(map_ids);
> +       if (!check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids))
> +               goto cleanup;
> +
> +       /* map should still exist when original file descriptor is closed */
> +       close(extra_fds[0]);
> +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> +               goto cleanup;
> +
> +       /* some fds might be invalid, so ignore return codes */
> +cleanup:
> +       close(extra_fds[0]);
> +       close(prog_fd);

ditto

> +}
> +
> +/*
> + * Test that a program with trash in fd_array can't be loaded:
> + * only map and BTF file descriptors should be accepted.
> + */
> +static void check_fd_array_cnt__fd_array_with_trash(void)
> +{
> +       int extra_fds[3] = { -1, -1, -1 };
> +       int prog_fd = -1;
> +
> +       extra_fds[0] = new_map();
> +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> +               goto cleanup;
> +       extra_fds[1] = new_btf();
> +       if (!ASSERT_GE(extra_fds[1], 0, "new_btf"))
> +               goto cleanup;
> +
> +       /* trash 1: not a file descriptor */
> +       extra_fds[2] = 0xbeef;
> +       prog_fd = load_test_prog(extra_fds, 3);
> +       if (!ASSERT_EQ(prog_fd, -EBADF, "prog should have been rejected with -EBADF"))
> +               goto cleanup;
> +
> +       /* trash 2: not a map or btf */
> +       extra_fds[2] = socket(AF_INET, SOCK_STREAM, 0);
> +       if (!ASSERT_GE(extra_fds[2], 0, "socket"))
> +               goto cleanup;
> +
> +       prog_fd = load_test_prog(extra_fds, 3);
> +       if (!ASSERT_EQ(prog_fd, -EINVAL, "prog should have been rejected with -EINVAL"))
> +               goto cleanup;
> +
> +       /* some fds might be invalid, so ignore return codes */
> +cleanup:
> +       close(extra_fds[2]);
> +       close(extra_fds[1]);
> +       close(extra_fds[0]);

ditto

> +}
> +
> +/*
> + * Test that a program with too big fd_array can't be loaded.
> + */
> +static void check_fd_array_cnt__fd_array_too_big(void)
> +{
> +       int extra_fds[65];
> +       int prog_fd = -1;
> +       int i;
> +
> +       for (i = 0; i < 65; i++) {
> +               extra_fds[i] = new_map();
> +               if (!ASSERT_GE(extra_fds[i], 0, "new_map"))
> +                       goto cleanup_fds;
> +       }
> +
> +       prog_fd = load_test_prog(extra_fds, 65);

nit: hard-coding 65 as the limit seems iffy, when we change
MAX_USED_MAPS this will need adjustment immediately. How about picking
something significantly larger, like 4096, creating just one map with
new_map(), but using that map FD in each entry, then doing
load_test_prog() once and check for -E2BIG?


> +       ASSERT_EQ(prog_fd, -E2BIG, "prog should have been rejected with -E2BIG");
> +
> +cleanup_fds:
> +       while (i > 0)
> +               close(extra_fds[--i]);
> +}
> +
> +void test_fd_array_cnt(void)
> +{
> +       if (test__start_subtest("no-fd-array"))
> +               check_fd_array_cnt__no_fd_array();
> +
> +       if (test__start_subtest("fd-array-ok"))
> +               check_fd_array_cnt__fd_array_ok();
> +
> +       if (test__start_subtest("fd-array-dup-input"))
> +               check_fd_array_cnt__duplicated_maps();
> +
> +       if (test__start_subtest("fd-array-ref-maps-in-array"))
> +               check_fd_array_cnt__referenced_maps_in_fd_array();
> +
> +       if (test__start_subtest("fd-array-trash-input"))
> +               check_fd_array_cnt__fd_array_with_trash();
> +
> +       if (test__start_subtest("fd-array-2big"))
> +               check_fd_array_cnt__fd_array_too_big();
> +}
> --
> 2.34.1
>
>

Re: [PATCH v4 bpf-next 5/7] selftests/bpf: Add tests for fd_array_cnt

[Anton Protopopov]

On 24/12/03 01:27PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 6:13 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > Add a new set of tests to test the new field in PROG_LOAD-related
> > part of bpf_attr: fd_array_cnt.
> >
> > Add the following test cases:
> >
> >   * fd_array_cnt/no-fd-array: program is loaded in a normal
> >     way, without any fd_array present
> >
> >   * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
> >     check that they're bound to the program
> >
> >   * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
> >     only two of which are unique
> >
> >   * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
> >     fd_array which is also referenced from within the program
> >
> >   * fd_array_cnt/fd-array-trash-input: pass array with some trash
> >
> >   * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)
> 
> nit: should be removed, there is no such test anymore
> 
> >
> >   * fd_array_cnt/fd-array-2big: pass too large array
> >
> > All the tests above are using the bpf(2) syscall directly,
> > no libbpf involved.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > ---
> >  .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
> >  1 file changed, 340 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fd_array.c b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> > new file mode 100644
> > index 000000000000..1d4bff4a1269
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> > @@ -0,0 +1,340 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +
> > +#include <linux/btf.h>
> > +#include <bpf/bpf.h>
> > +
> > +#include "../test_btf.h"
> > +
> > +static inline int new_map(void)
> > +{
> > +       LIBBPF_OPTS(bpf_map_create_opts, opts);
> > +       const char *name = NULL;
> > +       __u32 max_entries = 1;
> > +       __u32 value_size = 8;
> > +       __u32 key_size = 4;
> > +
> > +       return bpf_map_create(BPF_MAP_TYPE_ARRAY, name,
> > +                             key_size, value_size,
> > +                             max_entries, &opts);
> 
> nit: you don't really need to pass empty opts, passing NULL is always
> ok if no options are specified
> 
> > +}
> > +
> > +static int new_btf(void)
> > +{
> > +       LIBBPF_OPTS(bpf_btf_load_opts, opts);
> > +       struct btf_blob {
> > +               struct btf_header btf_hdr;
> > +               __u32 types[8];
> > +               __u32 str;
> > +       } raw_btf = {
> > +               .btf_hdr = {
> > +                       .magic = BTF_MAGIC,
> > +                       .version = BTF_VERSION,
> > +                       .hdr_len = sizeof(struct btf_header),
> > +                       .type_len = sizeof(raw_btf.types),
> > +                       .str_off = offsetof(struct btf_blob, str) - offsetof(struct btf_blob, types),
> > +                       .str_len = sizeof(raw_btf.str),
> > +               },
> > +               .types = {
> > +                       /* long */
> > +                       BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
> > +                       /* unsigned long */
> > +                       BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
> > +               },
> > +       };
> > +
> > +       return bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);
> 
> same, you don't seem to actually use opts
> 
> > +}
> > +
> > +static bool map_exists(__u32 id)
> > +{
> > +       int fd;
> > +
> > +       fd = bpf_map_get_fd_by_id(id);
> > +       if (fd >= 0) {
> > +               close(fd);
> > +               return true;
> > +       }
> > +       return false;
> > +}
> > +
> > +static inline int bpf_prog_get_map_ids(int prog_fd, __u32 *nr_map_ids, __u32 *map_ids)
> > +{
> > +       __u32 len = sizeof(struct bpf_prog_info);
> > +       struct bpf_prog_info info = {
> > +               .nr_map_ids = *nr_map_ids,
> > +               .map_ids = ptr_to_u64(map_ids),
> > +       };
> 
> nit: bpf_prog_info should be explicitly memset(0), and only then
> fields should be filled out. It might be ok right now because we don't
> have any padding (or compiler does zero that padding out, even though
> it's not required to do that), but this might pop up later, so best to
> avoid that.
> 
> > +       int err;
> > +
> > +       err = bpf_prog_get_info_by_fd(prog_fd, &info, &len);
> > +       if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
> > +               return -1;
> > +
> > +       *nr_map_ids = info.nr_map_ids;
> > +
> > +       return 0;
> > +}
> > +
> > +static int __load_test_prog(int map_fd, const int *fd_array, int fd_array_cnt)
> > +{
> > +       /* A trivial program which uses one map */
> > +       struct bpf_insn insns[] = {
> > +               BPF_LD_MAP_FD(BPF_REG_1, map_fd),
> > +               BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > +               BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > +               BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> > +               BPF_MOV64_IMM(BPF_REG_0, 0),
> > +               BPF_EXIT_INSN(),
> > +       };
> > +       LIBBPF_OPTS(bpf_prog_load_opts, opts);
> > +
> > +       opts.fd_array = fd_array;
> > +       opts.fd_array_cnt = fd_array_cnt;
> > +
> > +       return bpf_prog_load(BPF_PROG_TYPE_XDP, NULL, "GPL", insns, ARRAY_SIZE(insns), &opts);
> > +}
> > +
> > +static int load_test_prog(const int *fd_array, int fd_array_cnt)
> > +{
> > +       int map_fd;
> > +       int ret;
> > +
> > +       map_fd = new_map();
> > +       if (!ASSERT_GE(map_fd, 0, "new_map"))
> > +               return map_fd;
> > +
> > +       ret = __load_test_prog(map_fd, fd_array, fd_array_cnt);
> > +       close(map_fd);
> > +
> > +       /* switch back to returning the actual value */
> > +       if (ret < 0)
> > +               return -errno;
> 
> this errno might have been modified by close(), but you actually don't
> need errno, libbpf will return errno directly from bpf_prog_load(), so
> you can just do:
> 
> ret = __load_test_prog(...);
> close(map_fd);
> return ret;
> 
> > +       return ret;
> > +}
> > +
> > +static bool check_expected_map_ids(int prog_fd, int expected, __u32 *map_ids, __u32 *nr_map_ids)
> > +{
> > +       int err;
> > +
> > +       err = bpf_prog_get_map_ids(prog_fd, nr_map_ids, map_ids);
> > +       if (!ASSERT_OK(err, "bpf_prog_get_map_ids"))
> > +               return false;
> > +       if (!ASSERT_EQ(*nr_map_ids, expected, "unexpected nr_map_ids"))
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> > +/*
> > + * Load a program, which uses one map. No fd_array maps are present.
> > + * On return only one map is expected to be bound to prog.
> > + */
> > +static void check_fd_array_cnt__no_fd_array(void)
> > +{
> > +       __u32 map_ids[16];
> > +       __u32 nr_map_ids;
> > +       int prog_fd = -1;
> > +
> > +       prog_fd = load_test_prog(NULL, 0);
> > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > +               return;
> > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > +       check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids);
> > +       close(prog_fd);
> > +}
> > +
> > +/*
> > + * Load a program, which uses one map, and pass two extra, non-equal, maps in
> > + * fd_array with fd_array_cnt=2. On return three maps are expected to be bound
> > + * to the program.
> > + */
> > +static void check_fd_array_cnt__fd_array_ok(void)
> > +{
> > +       int extra_fds[2] = { -1, -1 };
> > +       __u32 map_ids[16];
> > +       __u32 nr_map_ids;
> > +       int prog_fd = -1;
> > +
> > +       extra_fds[0] = new_map();
> > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > +               goto cleanup;
> > +       extra_fds[1] = new_map();
> > +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> > +               goto cleanup;
> > +       prog_fd = load_test_prog(extra_fds, 2);
> > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > +               goto cleanup;
> > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> > +               goto cleanup;
> > +
> > +       /* maps should still exist when original file descriptors are closed */
> > +       close(extra_fds[0]);
> > +       close(extra_fds[1]);
> > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
> > +               goto cleanup;
> > +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
> > +               goto cleanup;
> > +
> > +       /* some fds might be invalid, so ignore return codes */
> > +cleanup:
> > +       close(extra_fds[1]);
> > +       close(extra_fds[0]);
> > +       close(prog_fd);
> 
> nit: technically, you should check each fd to be >= 0 before closing it
> 
> > +}
> > +
> > +/*
> > + * Load a program with a few extra maps duplicated in the fd_array.
> > + * After the load maps should only be referenced once.
> > + */
> > +static void check_fd_array_cnt__duplicated_maps(void)
> > +{
> > +       int extra_fds[4] = { -1, -1, -1, -1 };
> > +       __u32 map_ids[16];
> > +       __u32 nr_map_ids;
> > +       int prog_fd = -1;
> > +
> > +       extra_fds[0] = extra_fds[2] = new_map();
> > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > +               goto cleanup;
> > +       extra_fds[1] = extra_fds[3] = new_map();
> > +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> > +               goto cleanup;
> > +       prog_fd = load_test_prog(extra_fds, 4);
> > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > +               goto cleanup;
> > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> > +               goto cleanup;
> > +
> > +       /* maps should still exist when original file descriptors are closed */
> > +       close(extra_fds[0]);
> > +       close(extra_fds[1]);
> > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> > +               goto cleanup;
> > +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map should exist"))
> > +               goto cleanup;
> > +
> > +       /* some fds might be invalid, so ignore return codes */
> > +cleanup:
> > +       close(extra_fds[1]);
> > +       close(extra_fds[0]);
> > +       close(prog_fd);
> 
> same about if (fd >=0) close(fd); pattern
> 
> > +}
> > +
> > +/*
> > + * Check that if maps which are referenced by a program are
> > + * passed in fd_array, then they will be referenced only once
> > + */
> > +static void check_fd_array_cnt__referenced_maps_in_fd_array(void)
> > +{
> > +       int extra_fds[1] = { -1 };
> > +       __u32 map_ids[16];
> > +       __u32 nr_map_ids;
> > +       int prog_fd = -1;
> > +
> > +       extra_fds[0] = new_map();
> > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > +               goto cleanup;
> > +       prog_fd = __load_test_prog(extra_fds[0], extra_fds, 1);
> > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > +               goto cleanup;
> > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > +       if (!check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids))
> > +               goto cleanup;
> > +
> > +       /* map should still exist when original file descriptor is closed */
> > +       close(extra_fds[0]);
> > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> > +               goto cleanup;
> > +
> > +       /* some fds might be invalid, so ignore return codes */
> > +cleanup:
> > +       close(extra_fds[0]);
> > +       close(prog_fd);
> 
> ditto
> 
> > +}
> > +
> > +/*
> > + * Test that a program with trash in fd_array can't be loaded:
> > + * only map and BTF file descriptors should be accepted.
> > + */
> > +static void check_fd_array_cnt__fd_array_with_trash(void)
> > +{
> > +       int extra_fds[3] = { -1, -1, -1 };
> > +       int prog_fd = -1;
> > +
> > +       extra_fds[0] = new_map();
> > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > +               goto cleanup;
> > +       extra_fds[1] = new_btf();
> > +       if (!ASSERT_GE(extra_fds[1], 0, "new_btf"))
> > +               goto cleanup;
> > +
> > +       /* trash 1: not a file descriptor */
> > +       extra_fds[2] = 0xbeef;
> > +       prog_fd = load_test_prog(extra_fds, 3);
> > +       if (!ASSERT_EQ(prog_fd, -EBADF, "prog should have been rejected with -EBADF"))
> > +               goto cleanup;
> > +
> > +       /* trash 2: not a map or btf */
> > +       extra_fds[2] = socket(AF_INET, SOCK_STREAM, 0);
> > +       if (!ASSERT_GE(extra_fds[2], 0, "socket"))
> > +               goto cleanup;
> > +
> > +       prog_fd = load_test_prog(extra_fds, 3);
> > +       if (!ASSERT_EQ(prog_fd, -EINVAL, "prog should have been rejected with -EINVAL"))
> > +               goto cleanup;
> > +
> > +       /* some fds might be invalid, so ignore return codes */
> > +cleanup:
> > +       close(extra_fds[2]);
> > +       close(extra_fds[1]);
> > +       close(extra_fds[0]);
> 
> ditto
> 
> > +}
> > +
> > +/*
> > + * Test that a program with too big fd_array can't be loaded.
> > + */
> > +static void check_fd_array_cnt__fd_array_too_big(void)
> > +{
> > +       int extra_fds[65];
> > +       int prog_fd = -1;
> > +       int i;
> > +
> > +       for (i = 0; i < 65; i++) {
> > +               extra_fds[i] = new_map();
> > +               if (!ASSERT_GE(extra_fds[i], 0, "new_map"))
> > +                       goto cleanup_fds;
> > +       }
> > +
> > +       prog_fd = load_test_prog(extra_fds, 65);
> 
> nit: hard-coding 65 as the limit seems iffy, when we change
> MAX_USED_MAPS this will need adjustment immediately. How about picking
> something significantly larger, like 4096, creating just one map with
> new_map(), but using that map FD in each entry, then doing
> load_test_prog() once and check for -E2BIG?

This will not work with -E2BIG, as when maps are the same,
they will not be added to used_maps multiple times. I still
can try to bump the number here, but not sure if this is
possible to track MAX_USED_MAPS from userspace?

(All your comments above make sense, will fix.)

> 
> > +       ASSERT_EQ(prog_fd, -E2BIG, "prog should have been rejected with -E2BIG");
> > +
> > +cleanup_fds:
> > +       while (i > 0)
> > +               close(extra_fds[--i]);
> > +}
> > +
> > +void test_fd_array_cnt(void)
> > +{
> > +       if (test__start_subtest("no-fd-array"))
> > +               check_fd_array_cnt__no_fd_array();
> > +
> > +       if (test__start_subtest("fd-array-ok"))
> > +               check_fd_array_cnt__fd_array_ok();
> > +
> > +       if (test__start_subtest("fd-array-dup-input"))
> > +               check_fd_array_cnt__duplicated_maps();
> > +
> > +       if (test__start_subtest("fd-array-ref-maps-in-array"))
> > +               check_fd_array_cnt__referenced_maps_in_fd_array();
> > +
> > +       if (test__start_subtest("fd-array-trash-input"))
> > +               check_fd_array_cnt__fd_array_with_trash();
> > +
> > +       if (test__start_subtest("fd-array-2big"))
> > +               check_fd_array_cnt__fd_array_too_big();
> > +}
> > --
> > 2.34.1
> >
> >

Re: [PATCH v4 bpf-next 5/7] selftests/bpf: Add tests for fd_array_cnt

[Andrii Nakryiko]

On Wed, Dec 4, 2024 at 4:25 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> On 24/12/03 01:27PM, Andrii Nakryiko wrote:
> > On Tue, Dec 3, 2024 at 6:13 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> > >
> > > Add a new set of tests to test the new field in PROG_LOAD-related
> > > part of bpf_attr: fd_array_cnt.
> > >
> > > Add the following test cases:
> > >
> > >   * fd_array_cnt/no-fd-array: program is loaded in a normal
> > >     way, without any fd_array present
> > >
> > >   * fd_array_cnt/fd-array-ok: pass two extra non-used maps,
> > >     check that they're bound to the program
> > >
> > >   * fd_array_cnt/fd-array-dup-input: pass a few extra maps,
> > >     only two of which are unique
> > >
> > >   * fd_array_cnt/fd-array-ref-maps-in-array: pass a map in
> > >     fd_array which is also referenced from within the program
> > >
> > >   * fd_array_cnt/fd-array-trash-input: pass array with some trash
> > >
> > >   * fd_array_cnt/fd-array-with-holes: pass an array with holes (fd=0)
> >
> > nit: should be removed, there is no such test anymore
> >
> > >
> > >   * fd_array_cnt/fd-array-2big: pass too large array
> > >
> > > All the tests above are using the bpf(2) syscall directly,
> > > no libbpf involved.
> > >
> > > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > > ---
> > >  .../selftests/bpf/prog_tests/fd_array.c       | 340 ++++++++++++++++++
> > >  1 file changed, 340 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/fd_array.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/fd_array.c b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> > > new file mode 100644
> > > index 000000000000..1d4bff4a1269
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/fd_array.c
> > > @@ -0,0 +1,340 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <test_progs.h>
> > > +
> > > +#include <linux/btf.h>
> > > +#include <bpf/bpf.h>
> > > +
> > > +#include "../test_btf.h"
> > > +
> > > +static inline int new_map(void)
> > > +{
> > > +       LIBBPF_OPTS(bpf_map_create_opts, opts);
> > > +       const char *name = NULL;
> > > +       __u32 max_entries = 1;
> > > +       __u32 value_size = 8;
> > > +       __u32 key_size = 4;
> > > +
> > > +       return bpf_map_create(BPF_MAP_TYPE_ARRAY, name,
> > > +                             key_size, value_size,
> > > +                             max_entries, &opts);
> >
> > nit: you don't really need to pass empty opts, passing NULL is always
> > ok if no options are specified
> >
> > > +}
> > > +
> > > +static int new_btf(void)
> > > +{
> > > +       LIBBPF_OPTS(bpf_btf_load_opts, opts);
> > > +       struct btf_blob {
> > > +               struct btf_header btf_hdr;
> > > +               __u32 types[8];
> > > +               __u32 str;
> > > +       } raw_btf = {
> > > +               .btf_hdr = {
> > > +                       .magic = BTF_MAGIC,
> > > +                       .version = BTF_VERSION,
> > > +                       .hdr_len = sizeof(struct btf_header),
> > > +                       .type_len = sizeof(raw_btf.types),
> > > +                       .str_off = offsetof(struct btf_blob, str) - offsetof(struct btf_blob, types),
> > > +                       .str_len = sizeof(raw_btf.str),
> > > +               },
> > > +               .types = {
> > > +                       /* long */
> > > +                       BTF_TYPE_INT_ENC(0, BTF_INT_SIGNED, 0, 64, 8),  /* [1] */
> > > +                       /* unsigned long */
> > > +                       BTF_TYPE_INT_ENC(0, 0, 0, 64, 8),  /* [2] */
> > > +               },
> > > +       };
> > > +
> > > +       return bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);
> >
> > same, you don't seem to actually use opts
> >
> > > +}
> > > +
> > > +static bool map_exists(__u32 id)
> > > +{
> > > +       int fd;
> > > +
> > > +       fd = bpf_map_get_fd_by_id(id);
> > > +       if (fd >= 0) {
> > > +               close(fd);
> > > +               return true;
> > > +       }
> > > +       return false;
> > > +}
> > > +
> > > +static inline int bpf_prog_get_map_ids(int prog_fd, __u32 *nr_map_ids, __u32 *map_ids)
> > > +{
> > > +       __u32 len = sizeof(struct bpf_prog_info);
> > > +       struct bpf_prog_info info = {
> > > +               .nr_map_ids = *nr_map_ids,
> > > +               .map_ids = ptr_to_u64(map_ids),
> > > +       };
> >
> > nit: bpf_prog_info should be explicitly memset(0), and only then
> > fields should be filled out. It might be ok right now because we don't
> > have any padding (or compiler does zero that padding out, even though
> > it's not required to do that), but this might pop up later, so best to
> > avoid that.
> >
> > > +       int err;
> > > +
> > > +       err = bpf_prog_get_info_by_fd(prog_fd, &info, &len);
> > > +       if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd"))
> > > +               return -1;
> > > +
> > > +       *nr_map_ids = info.nr_map_ids;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __load_test_prog(int map_fd, const int *fd_array, int fd_array_cnt)
> > > +{
> > > +       /* A trivial program which uses one map */
> > > +       struct bpf_insn insns[] = {
> > > +               BPF_LD_MAP_FD(BPF_REG_1, map_fd),
> > > +               BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> > > +               BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> > > +               BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> > > +               BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> > > +               BPF_MOV64_IMM(BPF_REG_0, 0),
> > > +               BPF_EXIT_INSN(),
> > > +       };
> > > +       LIBBPF_OPTS(bpf_prog_load_opts, opts);
> > > +
> > > +       opts.fd_array = fd_array;
> > > +       opts.fd_array_cnt = fd_array_cnt;
> > > +
> > > +       return bpf_prog_load(BPF_PROG_TYPE_XDP, NULL, "GPL", insns, ARRAY_SIZE(insns), &opts);
> > > +}
> > > +
> > > +static int load_test_prog(const int *fd_array, int fd_array_cnt)
> > > +{
> > > +       int map_fd;
> > > +       int ret;
> > > +
> > > +       map_fd = new_map();
> > > +       if (!ASSERT_GE(map_fd, 0, "new_map"))
> > > +               return map_fd;
> > > +
> > > +       ret = __load_test_prog(map_fd, fd_array, fd_array_cnt);
> > > +       close(map_fd);
> > > +
> > > +       /* switch back to returning the actual value */
> > > +       if (ret < 0)
> > > +               return -errno;
> >
> > this errno might have been modified by close(), but you actually don't
> > need errno, libbpf will return errno directly from bpf_prog_load(), so
> > you can just do:
> >
> > ret = __load_test_prog(...);
> > close(map_fd);
> > return ret;
> >
> > > +       return ret;
> > > +}
> > > +
> > > +static bool check_expected_map_ids(int prog_fd, int expected, __u32 *map_ids, __u32 *nr_map_ids)
> > > +{
> > > +       int err;
> > > +
> > > +       err = bpf_prog_get_map_ids(prog_fd, nr_map_ids, map_ids);
> > > +       if (!ASSERT_OK(err, "bpf_prog_get_map_ids"))
> > > +               return false;
> > > +       if (!ASSERT_EQ(*nr_map_ids, expected, "unexpected nr_map_ids"))
> > > +               return false;
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +/*
> > > + * Load a program, which uses one map. No fd_array maps are present.
> > > + * On return only one map is expected to be bound to prog.
> > > + */
> > > +static void check_fd_array_cnt__no_fd_array(void)
> > > +{
> > > +       __u32 map_ids[16];
> > > +       __u32 nr_map_ids;
> > > +       int prog_fd = -1;
> > > +
> > > +       prog_fd = load_test_prog(NULL, 0);
> > > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > > +               return;
> > > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > > +       check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids);
> > > +       close(prog_fd);
> > > +}
> > > +
> > > +/*
> > > + * Load a program, which uses one map, and pass two extra, non-equal, maps in
> > > + * fd_array with fd_array_cnt=2. On return three maps are expected to be bound
> > > + * to the program.
> > > + */
> > > +static void check_fd_array_cnt__fd_array_ok(void)
> > > +{
> > > +       int extra_fds[2] = { -1, -1 };
> > > +       __u32 map_ids[16];
> > > +       __u32 nr_map_ids;
> > > +       int prog_fd = -1;
> > > +
> > > +       extra_fds[0] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > > +               goto cleanup;
> > > +       extra_fds[1] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> > > +               goto cleanup;
> > > +       prog_fd = load_test_prog(extra_fds, 2);
> > > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > > +               goto cleanup;
> > > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > > +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> > > +               goto cleanup;
> > > +
> > > +       /* maps should still exist when original file descriptors are closed */
> > > +       close(extra_fds[0]);
> > > +       close(extra_fds[1]);
> > > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map_ids[0] should exist"))
> > > +               goto cleanup;
> > > +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map_ids[1] should exist"))
> > > +               goto cleanup;
> > > +
> > > +       /* some fds might be invalid, so ignore return codes */
> > > +cleanup:
> > > +       close(extra_fds[1]);
> > > +       close(extra_fds[0]);
> > > +       close(prog_fd);
> >
> > nit: technically, you should check each fd to be >= 0 before closing it
> >
> > > +}
> > > +
> > > +/*
> > > + * Load a program with a few extra maps duplicated in the fd_array.
> > > + * After the load maps should only be referenced once.
> > > + */
> > > +static void check_fd_array_cnt__duplicated_maps(void)
> > > +{
> > > +       int extra_fds[4] = { -1, -1, -1, -1 };
> > > +       __u32 map_ids[16];
> > > +       __u32 nr_map_ids;
> > > +       int prog_fd = -1;
> > > +
> > > +       extra_fds[0] = extra_fds[2] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > > +               goto cleanup;
> > > +       extra_fds[1] = extra_fds[3] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[1], 0, "new_map"))
> > > +               goto cleanup;
> > > +       prog_fd = load_test_prog(extra_fds, 4);
> > > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > > +               goto cleanup;
> > > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > > +       if (!check_expected_map_ids(prog_fd, 3, map_ids, &nr_map_ids))
> > > +               goto cleanup;
> > > +
> > > +       /* maps should still exist when original file descriptors are closed */
> > > +       close(extra_fds[0]);
> > > +       close(extra_fds[1]);
> > > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> > > +               goto cleanup;
> > > +       if (!ASSERT_EQ(map_exists(map_ids[1]), true, "map should exist"))
> > > +               goto cleanup;
> > > +
> > > +       /* some fds might be invalid, so ignore return codes */
> > > +cleanup:
> > > +       close(extra_fds[1]);
> > > +       close(extra_fds[0]);
> > > +       close(prog_fd);
> >
> > same about if (fd >=0) close(fd); pattern
> >
> > > +}
> > > +
> > > +/*
> > > + * Check that if maps which are referenced by a program are
> > > + * passed in fd_array, then they will be referenced only once
> > > + */
> > > +static void check_fd_array_cnt__referenced_maps_in_fd_array(void)
> > > +{
> > > +       int extra_fds[1] = { -1 };
> > > +       __u32 map_ids[16];
> > > +       __u32 nr_map_ids;
> > > +       int prog_fd = -1;
> > > +
> > > +       extra_fds[0] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > > +               goto cleanup;
> > > +       prog_fd = __load_test_prog(extra_fds[0], extra_fds, 1);
> > > +       if (!ASSERT_GE(prog_fd, 0, "BPF_PROG_LOAD"))
> > > +               goto cleanup;
> > > +       nr_map_ids = ARRAY_SIZE(map_ids);
> > > +       if (!check_expected_map_ids(prog_fd, 1, map_ids, &nr_map_ids))
> > > +               goto cleanup;
> > > +
> > > +       /* map should still exist when original file descriptor is closed */
> > > +       close(extra_fds[0]);
> > > +       if (!ASSERT_EQ(map_exists(map_ids[0]), true, "map should exist"))
> > > +               goto cleanup;
> > > +
> > > +       /* some fds might be invalid, so ignore return codes */
> > > +cleanup:
> > > +       close(extra_fds[0]);
> > > +       close(prog_fd);
> >
> > ditto
> >
> > > +}
> > > +
> > > +/*
> > > + * Test that a program with trash in fd_array can't be loaded:
> > > + * only map and BTF file descriptors should be accepted.
> > > + */
> > > +static void check_fd_array_cnt__fd_array_with_trash(void)
> > > +{
> > > +       int extra_fds[3] = { -1, -1, -1 };
> > > +       int prog_fd = -1;
> > > +
> > > +       extra_fds[0] = new_map();
> > > +       if (!ASSERT_GE(extra_fds[0], 0, "new_map"))
> > > +               goto cleanup;
> > > +       extra_fds[1] = new_btf();
> > > +       if (!ASSERT_GE(extra_fds[1], 0, "new_btf"))
> > > +               goto cleanup;
> > > +
> > > +       /* trash 1: not a file descriptor */
> > > +       extra_fds[2] = 0xbeef;
> > > +       prog_fd = load_test_prog(extra_fds, 3);
> > > +       if (!ASSERT_EQ(prog_fd, -EBADF, "prog should have been rejected with -EBADF"))
> > > +               goto cleanup;
> > > +
> > > +       /* trash 2: not a map or btf */
> > > +       extra_fds[2] = socket(AF_INET, SOCK_STREAM, 0);
> > > +       if (!ASSERT_GE(extra_fds[2], 0, "socket"))
> > > +               goto cleanup;
> > > +
> > > +       prog_fd = load_test_prog(extra_fds, 3);
> > > +       if (!ASSERT_EQ(prog_fd, -EINVAL, "prog should have been rejected with -EINVAL"))
> > > +               goto cleanup;
> > > +
> > > +       /* some fds might be invalid, so ignore return codes */
> > > +cleanup:
> > > +       close(extra_fds[2]);
> > > +       close(extra_fds[1]);
> > > +       close(extra_fds[0]);
> >
> > ditto
> >
> > > +}
> > > +
> > > +/*
> > > + * Test that a program with too big fd_array can't be loaded.
> > > + */
> > > +static void check_fd_array_cnt__fd_array_too_big(void)
> > > +{
> > > +       int extra_fds[65];
> > > +       int prog_fd = -1;
> > > +       int i;
> > > +
> > > +       for (i = 0; i < 65; i++) {
> > > +               extra_fds[i] = new_map();
> > > +               if (!ASSERT_GE(extra_fds[i], 0, "new_map"))
> > > +                       goto cleanup_fds;
> > > +       }
> > > +
> > > +       prog_fd = load_test_prog(extra_fds, 65);
> >
> > nit: hard-coding 65 as the limit seems iffy, when we change
> > MAX_USED_MAPS this will need adjustment immediately. How about picking
> > something significantly larger, like 4096, creating just one map with
> > new_map(), but using that map FD in each entry, then doing
> > load_test_prog() once and check for -E2BIG?
>
> This will not work with -E2BIG, as when maps are the same,
> they will not be added to used_maps multiple times. I still
> can try to bump the number here, but not sure if this is
> possible to track MAX_USED_MAPS from userspace?

ah, makes sense, it has to be all different maps... Ok, never mind
then, we will detect breakage easily if we ever change internal
constant, so not really a big deal

>
> (All your comments above make sense, will fix.)
>
> >
> > > +       ASSERT_EQ(prog_fd, -E2BIG, "prog should have been rejected with -E2BIG");
> > > +
> > > +cleanup_fds:
> > > +       while (i > 0)
> > > +               close(extra_fds[--i]);
> > > +}
> > > +
> > > +void test_fd_array_cnt(void)
> > > +{
> > > +       if (test__start_subtest("no-fd-array"))
> > > +               check_fd_array_cnt__no_fd_array();
> > > +
> > > +       if (test__start_subtest("fd-array-ok"))
> > > +               check_fd_array_cnt__fd_array_ok();
> > > +
> > > +       if (test__start_subtest("fd-array-dup-input"))
> > > +               check_fd_array_cnt__duplicated_maps();
> > > +
> > > +       if (test__start_subtest("fd-array-ref-maps-in-array"))
> > > +               check_fd_array_cnt__referenced_maps_in_fd_array();
> > > +
> > > +       if (test__start_subtest("fd-array-trash-input"))
> > > +               check_fd_array_cnt__fd_array_with_trash();
> > > +
> > > +       if (test__start_subtest("fd-array-2big"))
> > > +               check_fd_array_cnt__fd_array_too_big();
> > > +}
> > > --
> > > 2.34.1
> > >
> > >

[PATCH v4 bpf-next 6/7] bpf: fix potential error return

[Anton Protopopov]

The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
error is a result of bpf_adj_branches(), and thus should be always 0
However, if for any reason it is not 0, then it will be converted to
boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
error value. Fix this by returning the original err after the WARN check.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index a2327c4fdc8b..8b9711e6da6c 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 
 int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 {
+	int err;
+
 	/* Branch offsets can't overflow when program is shrinking, no need
 	 * to call bpf_adj_branches(..., true) here
 	 */
@@ -546,7 +548,9 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 		sizeof(struct bpf_insn) * (prog->len - off - cnt));
 	prog->len -= cnt;
 
-	return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
+	err = bpf_adj_branches(prog, off, off + cnt, off, false);
+	WARN_ON_ONCE(err);
+	return err;
 }
 
 static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
-- 
2.34.1

Re: [PATCH v4 bpf-next 6/7] bpf: fix potential error return

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 5:49 AM Anton Protopopov <aspsk@isovalent.com> wrote:
>
> The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
> error is a result of bpf_adj_branches(), and thus should be always 0
> However, if for any reason it is not 0, then it will be converted to
> boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
> error value. Fix this by returning the original err after the WARN check.
>
> Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/bpf/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>

Looks irrelevant to the patch set and probably should go through the
bpf tree? I'll leave it up to Alexei to decide, though.

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index a2327c4fdc8b..8b9711e6da6c 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>
>  int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
>  {
> +       int err;
> +
>         /* Branch offsets can't overflow when program is shrinking, no need
>          * to call bpf_adj_branches(..., true) here
>          */
> @@ -546,7 +548,9 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
>                 sizeof(struct bpf_insn) * (prog->len - off - cnt));
>         prog->len -= cnt;
>
> -       return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
> +       err = bpf_adj_branches(prog, off, off + cnt, off, false);
> +       WARN_ON_ONCE(err);
> +       return err;
>  }
>
>  static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
> --
> 2.34.1
>
>

Re: [PATCH v4 bpf-next 6/7] bpf: fix potential error return

[Anton Protopopov]

On 24/12/03 01:26PM, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 5:49 AM Anton Protopopov <aspsk@isovalent.com> wrote:
> >
> > The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
> > error is a result of bpf_adj_branches(), and thus should be always 0
> > However, if for any reason it is not 0, then it will be converted to
> > boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
> > error value. Fix this by returning the original err after the WARN check.
> >
> > Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/core.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> 
> Looks irrelevant to the patch set and probably should go through the
> bpf tree? I'll leave it up to Alexei to decide, though.

Sure, I can send it separately, if needed.

> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index a2327c4fdc8b..8b9711e6da6c 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> >
> >  int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
> >  {
> > +       int err;
> > +
> >         /* Branch offsets can't overflow when program is shrinking, no need
> >          * to call bpf_adj_branches(..., true) here
> >          */
> > @@ -546,7 +548,9 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
> >                 sizeof(struct bpf_insn) * (prog->len - off - cnt));
> >         prog->len -= cnt;
> >
> > -       return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
> > +       err = bpf_adj_branches(prog, off, off + cnt, off, false);
> > +       WARN_ON_ONCE(err);
> > +       return err;
> >  }
> >
> >  static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)
> > --
> > 2.34.1
> >
> >

[PATCH v4 bpf-next 7/7] selftest/bpf: replace magic constants by macros

[Anton Protopopov]

Replace magic constants in a BTF structure initialization code by
proper macros, as is done in other similar selftests.

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/progs/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/syscall.c b/tools/testing/selftests/bpf/progs/syscall.c
index 0f4dfb770c32..b698cc62a371 100644
--- a/tools/testing/selftests/bpf/progs/syscall.c
+++ b/tools/testing/selftests/bpf/progs/syscall.c
@@ -76,9 +76,9 @@ static int btf_load(void)
 			.magic = BTF_MAGIC,
 			.version = BTF_VERSION,
 			.hdr_len = sizeof(struct btf_header),
-			.type_len = sizeof(__u32) * 8,
-			.str_off = sizeof(__u32) * 8,
-			.str_len = sizeof(__u32),
+			.type_len = sizeof(raw_btf.types),
+			.str_off = offsetof(struct btf_blob, str) - offsetof(struct btf_blob, types),
+			.str_len = sizeof(raw_btf.str),
 		},
 		.types = {
 			/* long */
-- 
2.34.1