[LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Jan Stancek]

gcc 15 stopped zero-initializing padding bits:
  https://gcc.gnu.org/gcc-15/changes.html

However kernel bpf syscall checks that all unused fields for a command
are set to zero in CHECK_ATTR() macro, which causes tests to fail with
EINVAL.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/bpf/bpf_common.c | 32 ++++++++++++----------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index 95b5bc12eaa4..d765c4e32936 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -49,13 +49,14 @@ int bpf_map_create(union bpf_attr *const attr)
 
 int bpf_map_array_create(const uint32_t max_entries)
 {
-	union bpf_attr map_attr = {
-		.map_type = BPF_MAP_TYPE_ARRAY,
-		.key_size = 4,
-		.value_size = 8,
-		.max_entries = max_entries,
-		.map_flags = 0
-	};
+	/* zero-initialize entire struct including padding bits */
+	union bpf_attr map_attr = {};
+
+	map_attr.map_type = BPF_MAP_TYPE_ARRAY;
+	map_attr.key_size = 4;
+	map_attr.value_size = 8;
+	map_attr.max_entries = max_entries;
+	map_attr.map_flags = 0;
 
 	return bpf_map_create(&map_attr);
 }
@@ -64,13 +65,16 @@ void bpf_map_array_get(const int map_fd,
 		       const uint32_t *const array_indx,
 		       uint64_t *const array_val)
 {
-	union bpf_attr elem_attr = {
-		.map_fd = map_fd,
-		.key = ptr_to_u64(array_indx),
-		.value = ptr_to_u64(array_val),
-		.flags = 0
-	};
-	const int ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
+	/* zero-initialize entire struct including padding bits */
+	union bpf_attr elem_attr = {};
+	int ret;
+
+	elem_attr.map_fd = map_fd;
+	elem_attr.key = ptr_to_u64(array_indx);
+	elem_attr.value = ptr_to_u64(array_val);
+	elem_attr.flags = 0;
+
+	ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
 
 	if (ret) {
 		tst_brk(TBROK | TTERRNO,
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Cyril Hrubis]

Hi!
> gcc 15 stopped zero-initializing padding bits:
>   https://gcc.gnu.org/gcc-15/changes.html

I would even call this a bug even if it's allowed by the C standard.

> However kernel bpf syscall checks that all unused fields for a command
> are set to zero in CHECK_ATTR() macro, which causes tests to fail with
> EINVAL.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/bpf/bpf_common.c | 32 ++++++++++++----------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> index 95b5bc12eaa4..d765c4e32936 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_common.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -49,13 +49,14 @@ int bpf_map_create(union bpf_attr *const attr)
>  
>  int bpf_map_array_create(const uint32_t max_entries)
>  {
> -	union bpf_attr map_attr = {
> -		.map_type = BPF_MAP_TYPE_ARRAY,
> -		.key_size = 4,
> -		.value_size = 8,
> -		.max_entries = max_entries,
> -		.map_flags = 0
> -	};
> +	/* zero-initialize entire struct including padding bits */
> +	union bpf_attr map_attr = {};
> +
> +	map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> +	map_attr.key_size = 4;
> +	map_attr.value_size = 8;
> +	map_attr.max_entries = max_entries;
> +	map_attr.map_flags = 0;

So struct foo bar = {.bar = foo}; does not work,
but struct foo bar = {}: bar.bar = foo; does? That is insane...

>  	return bpf_map_create(&map_attr);
>  }
> @@ -64,13 +65,16 @@ void bpf_map_array_get(const int map_fd,
>  		       const uint32_t *const array_indx,
>  		       uint64_t *const array_val)
>  {
> -	union bpf_attr elem_attr = {
> -		.map_fd = map_fd,
> -		.key = ptr_to_u64(array_indx),
> -		.value = ptr_to_u64(array_val),
> -		.flags = 0
> -	};
> -	const int ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
> +	/* zero-initialize entire struct including padding bits */
> +	union bpf_attr elem_attr = {};
> +	int ret;
> +
> +	elem_attr.map_fd = map_fd;
> +	elem_attr.key = ptr_to_u64(array_indx);
> +	elem_attr.value = ptr_to_u64(array_val);
> +	elem_attr.flags = 0;
> +
> +	ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
>  
>  	if (ret) {
>  		tst_brk(TBROK | TTERRNO,
> -- 
> 2.43.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Cyril Hrubis]

Hi!
> However kernel bpf syscall checks that all unused fields for a command
> are set to zero in CHECK_ATTR() macro, which causes tests to fail with
> EINVAL.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/bpf/bpf_common.c | 32 ++++++++++++----------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> index 95b5bc12eaa4..d765c4e32936 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_common.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -49,13 +49,14 @@ int bpf_map_create(union bpf_attr *const attr)
>  
>  int bpf_map_array_create(const uint32_t max_entries)
>  {
> -	union bpf_attr map_attr = {
> -		.map_type = BPF_MAP_TYPE_ARRAY,
> -		.key_size = 4,
> -		.value_size = 8,
> -		.max_entries = max_entries,
> -		.map_flags = 0
> -	};
> +	/* zero-initialize entire struct including padding bits */
> +	union bpf_attr map_attr = {};
> +
> +	map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> +	map_attr.key_size = 4;
> +	map_attr.value_size = 8;
> +	map_attr.max_entries = max_entries;
> +	map_attr.map_flags = 0;

I had a closer look here, the map_attr is an union with anonymous
structures and I suppose that the problem here is that the padding after
the union is no longer cleared and that there have been some new fields
added, at least compared to the lapi fallback structures we have and we
possibly pass random mess in flags.

Maybe slightly better version would be:

diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index 95b5bc12e..a8289e106 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -49,7 +49,9 @@ int bpf_map_create(union bpf_attr *const attr)

 int bpf_map_array_create(const uint32_t max_entries)
 {
-       union bpf_attr map_attr = {
+       union bpf_attr map_attr = {};
+
+       map_attr = (union bpf_attr) {
                .map_type = BPF_MAP_TYPE_ARRAY,
                .key_size = 4,
                .value_size = 8,


-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Jan Stancek]

On Thu, Feb 6, 2025 at 3:08 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > However kernel bpf syscall checks that all unused fields for a command
> > are set to zero in CHECK_ATTR() macro, which causes tests to fail with
> > EINVAL.
> >
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  testcases/kernel/syscalls/bpf/bpf_common.c | 32 ++++++++++++----------
> >  1 file changed, 18 insertions(+), 14 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> > index 95b5bc12eaa4..d765c4e32936 100644
> > --- a/testcases/kernel/syscalls/bpf/bpf_common.c
> > +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> > @@ -49,13 +49,14 @@ int bpf_map_create(union bpf_attr *const attr)
> >
> >  int bpf_map_array_create(const uint32_t max_entries)
> >  {
> > -     union bpf_attr map_attr = {
> > -             .map_type = BPF_MAP_TYPE_ARRAY,
> > -             .key_size = 4,
> > -             .value_size = 8,
> > -             .max_entries = max_entries,
> > -             .map_flags = 0
> > -     };
> > +     /* zero-initialize entire struct including padding bits */
> > +     union bpf_attr map_attr = {};
> > +
> > +     map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> > +     map_attr.key_size = 4;
> > +     map_attr.value_size = 8;
> > +     map_attr.max_entries = max_entries;
> > +     map_attr.map_flags = 0;
>
> I had a closer look here, the map_attr is an union with anonymous
> structures and I suppose that the problem here is that the padding after
> the union is no longer cleared and that there have been some new fields
> added, at least compared to the lapi fallback structures we have and we
> possibly pass random mess in flags.

It's not just padding, some fields (from other structs) are also not
initialized:

void bpf_map_array_get(const int map_fd,
                       const uint32_t *const array_indx,
                       uint64_t *const array_val)
{
        union bpf_attr elem_attr = {
                .map_fd = map_fd,
                .key = ptr_to_u64(array_indx),
                .value = ptr_to_u64(array_val),
                .flags = 0
        };

        printf("should be zero? %u\n", elem_attr.func_info_cnt);

and I get:
should be zero? 4202093

func_info_cnt is at offset 88, which is far away from ones we
initialized explicitly:

        struct {
                uint32_t           map_fd;             /*     0     4 */

                /* XXX 4 bytes hole, try to pack */

                uint64_t           key
__attribute__((__aligned__(8))); /*     8     8 */
                union {
                        uint64_t   value
__attribute__((__aligned__(8))); /*    16     8 */
                        uint64_t   next_key
__attribute__((__aligned__(8))); /*    16     8 */
                } __attribute__((__aligned__(8)));     /*    16     8 */
                uint64_t           flags;              /*    24     8 */
        } __attribute__((__aligned__(8)))
__attribute__((__aligned__(8)));             /*     0    32 */

>
> Maybe slightly better version would be:
>
> diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
> index 95b5bc12e..a8289e106 100644
> --- a/testcases/kernel/syscalls/bpf/bpf_common.c
> +++ b/testcases/kernel/syscalls/bpf/bpf_common.c
> @@ -49,7 +49,9 @@ int bpf_map_create(union bpf_attr *const attr)
>
>  int bpf_map_array_create(const uint32_t max_entries)
>  {
> -       union bpf_attr map_attr = {
> +       union bpf_attr map_attr = {};
> +
> +       map_attr = (union bpf_attr) {
>                 .map_type = BPF_MAP_TYPE_ARRAY,
>                 .key_size = 4,
>                 .value_size = 8,
>
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Cyril Hrubis]

Hi!
> > I had a closer look here, the map_attr is an union with anonymous
> > structures and I suppose that the problem here is that the padding after
> > the union is no longer cleared and that there have been some new fields
> > added, at least compared to the lapi fallback structures we have and we
> > possibly pass random mess in flags.
> 
> It's not just padding, some fields (from other structs) are also not
> initialized:
> 
> void bpf_map_array_get(const int map_fd,
>                        const uint32_t *const array_indx,
>                        uint64_t *const array_val)
> {
>         union bpf_attr elem_attr = {
>                 .map_fd = map_fd,
>                 .key = ptr_to_u64(array_indx),
>                 .value = ptr_to_u64(array_val),
>                 .flags = 0
>         };
> 
>         printf("should be zero? %u\n", elem_attr.func_info_cnt);
> 
> and I get:
> should be zero? 4202093

That's what I meant, but phrased it wrongly. Padding after the shorter
union fields, which are anonymous structures. That means that quite a
lot of the fields may end up non-zeroed in case that we initialize
a structure that is on the shorter side. I would say that in this case
the optimalization is very evil one, because it looks like these members
should have been zeroed at first.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Jan Stancek]

gcc 15 stopped zero-initializing padding bits:
  https://gcc.gnu.org/gcc-15/changes.html

However kernel bpf syscall checks that all unused fields for a command
are set to zero in CHECK_ATTR() macro, which causes tests to fail with
EINVAL.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
v2 changes:
- Use slightly better version suggested by Cyril

 testcases/kernel/syscalls/bpf/bpf_common.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/bpf/bpf_common.c b/testcases/kernel/syscalls/bpf/bpf_common.c
index 95b5bc12eaa4..9148b0437279 100644
--- a/testcases/kernel/syscalls/bpf/bpf_common.c
+++ b/testcases/kernel/syscalls/bpf/bpf_common.c
@@ -49,7 +49,10 @@ int bpf_map_create(union bpf_attr *const attr)
 
 int bpf_map_array_create(const uint32_t max_entries)
 {
-	union bpf_attr map_attr = {
+	/* zero-initialize entire struct including padding bits */
+	union bpf_attr map_attr = {};
+
+	map_attr = (union bpf_attr) {
 		.map_type = BPF_MAP_TYPE_ARRAY,
 		.key_size = 4,
 		.value_size = 8,
@@ -64,13 +67,18 @@ void bpf_map_array_get(const int map_fd,
 		       const uint32_t *const array_indx,
 		       uint64_t *const array_val)
 {
-	union bpf_attr elem_attr = {
+	/* zero-initialize entire struct including padding bits */
+	union bpf_attr elem_attr = {};
+	int ret;
+
+	elem_attr = (union bpf_attr) {
 		.map_fd = map_fd,
 		.key = ptr_to_u64(array_indx),
 		.value = ptr_to_u64(array_val),
 		.flags = 0
 	};
-	const int ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
+
+	ret = bpf(BPF_MAP_LOOKUP_ELEM, &elem_attr, sizeof(elem_attr));
 
 	if (ret) {
 		tst_brk(TBROK | TTERRNO,
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] syscalls/bpf: zero-initialize bpf_attr including padding bits

[Jan Stancek]

On Fri, Feb 7, 2025 at 9:35 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Pushed.

Thanks,
Jan


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp