Skip to contentSkip to conversation

bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info

PATCH In review Eugene Syromiatnikov 5 participants 7 messages May 27, 2018 – Jun 2, 2018

Subsystems (3)
Networking Network drivers BPF
lore
Share & tools
lore

Eugene Syromiatnikov

author
#0 lore raw

PATCH 1/2 bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
tools/include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
Signature · 1 line
2.1.4
#1 lore raw
Quoted 23 lines · Eugene Syromiatnikov

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
include/uapi/linux/bpf.h +0 -0
index c5ec897..903010a 100644

include/uapi/linux/bpf.h struct bpf_prog_info { · line 1022

Quoted 8 lines
include/uapi/linux/bpf.h +2 -2
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
        __aligned_u64 map_ids;
        char name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
-       __u64 netns_dev;
-       __u64 netns_ino;
+       __aligned_u64 netns_dev;
+       __aligned_u64 netns_ino;
 } __attribute__((aligned(8)));

Shall we add a __u32 padding variable before netns_dev? We can use it for in the future.

Thanks,
Song

Quoted 42 lines
include/uapi/linux/bpf.h +2 -2
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
        __u32 map_flags;
        char  name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
-       __u64 netns_dev;
-       __u64 netns_ino;
+       __aligned_u64 netns_dev;
+       __aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
tools/include/uapi/linux/bpf.h +4 -5
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
        __aligned_u64 map_ids;
        char name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
-       __u64 netns_dev;
-       __u64 netns_ino;
+       __aligned_u64 netns_dev;
+       __aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
        __u32 map_flags;
        char  name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
-       __u64 netns_dev;
-       __u64 netns_ino;
+       __aligned_u64 netns_dev;
+       __aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
--

2.1.4

maintainer
#2 lore raw
Quoted 19 lines · Song Liu
Quoted 18 lines · Eugene Syromiatnikov

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
Quoted 16 lines

include/uapi/linux/bpf.h struct bpf_prog_info { · line 1022

Quoted 13 lines
include/uapi/linux/bpf.h +2 -2
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
        __aligned_u64 map_ids;
        char name[BPF_OBJ_NAME_LEN];
        __u32 ifindex;
-       __u64 netns_dev;
-       __u64 netns_ino;
+       __aligned_u64 netns_dev;
+       __aligned_u64 netns_ino;
 } __attribute__((aligned(8)));

Shall we add a __u32 padding variable before netns_dev? We can use it for in the future.

Agree with Song, and definitely prefer that approach since we already use the hole as a bitfield in net-next; like this https://patchwork.ozlabs.org/patch/924415/.

Reviewed-by on v1 1/2
#3 lore raw
In reply to Eugene Syromiatnikov
Quoted 11 lines · Eugene Syromiatnikov

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Reviewed-by "Dmitry V. Levin" <[email protected]>
Cc <[email protected]> # v4.16+

Thanks,

Signature · 1 line
ldv
#4 lore raw

Hi,

Looks like the ABI bug in bpf_map_info and bpf_prog info introduced in 4.16 is going to slip into 4.17, causing extra pain to 32-bit userspace. I'm adding Linus to this thread in hope it might help to get a fix applied before 4.17 is released.

Quoted 77 lines · Dmitry V. Levin
Quoted 6 lines · Eugene Syromiatnikov

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Quoted 7 lines
Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
Reviewed-by "Dmitry V. Levin" <[email protected]>
Cc <[email protected]> # v4.16+

Thanks,

Quoted 57 lines
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
tools/include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
Signature · 1 line
ldv
#5 lore raw
Quoted 6 lines · Dmitry V. Levin

Hi,

Looks like the ABI bug in bpf_map_info and bpf_prog info introduced in 4.16 is going to slip into 4.17, causing extra pain to 32-bit userspace. I'm adding Linus to this thread in hope it might help to get a fix applied before 4.17 is released.

The issue identified in patch 1 is valid.
These two fields won't be properly accessible by 32-bit user space on 64-bit kernel.
But the fix in patch 1 is wrong.
The patch 2 is completely unnecessary.
Everyone can also see that the patches are still marked as 'new' in patchworks meaning that we didn't process them.
At the moment many networking folks are traveling to netconf
and we only had a chance to discuss this set last night.
We'll try to send a fix in coming days.
But regardless whether 4.17 is realesed this sunday or not
we're not going to rush wrong patch in without proper code review and discussion.
That future patch either will land in 4.17 (if it's dealyed into next sunday) or it will be sent to stable.

To speed up the situation next time please report the issue that you find to public netdev mailing list instead of using proprietary distro emails.

Thanks.

Quoted 82 lines · Dmitry V. Levin
Quoted 77 lines · Eugene Syromiatnikov
Quoted 6 lines

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Quoted 7 lines
Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
Reviewed-by "Dmitry V. Levin" <[email protected]>
Cc <[email protected]> # v4.16+

Thanks,

Quoted 57 lines
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
tools/include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
Signature · 1 line
ldv
#6 lore raw
Quoted 11 lines · Alexei Starovoitov
Quoted 6 lines · Dmitry V. Levin

Hi,

Looks like the ABI bug in bpf_map_info and bpf_prog info introduced in 4.16 is going to slip into 4.17, causing extra pain to 32-bit userspace. I'm adding Linus to this thread in hope it might help to get a fix applied before 4.17 is released.

The issue identified in patch 1 is valid.
These two fields won't be properly accessible by 32-bit user space on 64-bit kernel.

Yes, and currently there is no way to build a 32-bit userspace that would work properly both with 32-bit and 64-bit kernels.

Quoted 1 line

But the fix in patch 1 is wrong.

Please elaborate.

Quoted 1 line

The patch 2 is completely unnecessary.

The patch 2 doesn't have to be backported to 4.16, but if it was implemented in 4.16, there would be no bug to discuss. That is, the patch 2 is a good policy that would help to avoid this class of bugs in the future.

Alexei, looks like your Acked-by on the buggy commit 52775b33bb5072fbc07b02c0cf4fe8da1f7ee7cd affects your judgement. If this is the case, please refer patch 2 to other people who are less biased.

Quoted 10 lines

Everyone can also see that the patches are still marked as 'new' in patchworks meaning that we didn't process them.
At the moment many networking folks are traveling to netconf
and we only had a chance to discuss this set last night.
We'll try to send a fix in coming days.
But regardless whether 4.17 is realesed this sunday or not
we're not going to rush wrong patch in without proper code review and discussion.
That future patch either will land in 4.17 (if it's dealyed into next sunday) or it will be sent to stable.

Note that the fix was submitted to netdev on 2018-05-27. One week is surely enough to make this bug fixed, isn't it?

Quoted 2 lines

To speed up the situation next time please report the issue that you find to public netdev mailing list instead of using proprietary distro emails.

Please explain to me and to the public what do you mean by making this statement.

I do believe strace developers are free to discuss among themselves using whatever means of communication they find appropriate.

I do believe the issue was properly reported to netdev, see
https://lkml.kernel.org/r/[email protected]
https://lkml.kernel.org/r/[email protected]

I'd like to use this opportunity to thank Eugene for submitting the fix the same day the issue was identified as a kernel bug.

Alexei, please do not exclude my email address from this discussion. Thanks.

Quoted 78 lines
Quoted 78 lines · Dmitry V. Levin
Quoted 77 lines · Eugene Syromiatnikov
Quoted 6 lines

Recent introduction of netns_dev/netns_ino to bpf_map_info/bpf_prog info has broken compat, as offsets of these fields are different in 32-bit and 64-bit ABIs. One fix (other than implementing compat support in syscall in order to handle this discrepancy) is to use __aligned_u64 instead of __u64 for these fields.

Quoted 7 lines
Reported-by Dmitry V. Levin <[email protected]>
Fixes 52775b33bb507 ("bpf: offload: report device information about

offloaded maps")

Fixes 675fc275a3a2d ("bpf: offload: report device information for

offloaded programs")

Signed-off-by Eugene Syromiatnikov <[email protected]>
Reviewed-by "Dmitry V. Levin" <[email protected]>
Cc <[email protected]> # v4.16+

Thanks,

Quoted 57 lines
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)
include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
tools/include/uapi/linux/bpf.h +4 -4
index c5ec897..903010a 100644
@@ -1017,8 +1017,8 @@ struct bpf_prog_info {
 	__aligned_u64 map_ids;
 	char name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {

@@ -1030,8 +1030,8 @@ struct bpf_map_info {
 	__u32 map_flags;
 	char  name[BPF_OBJ_NAME_LEN];
 	__u32 ifindex;
-	__u64 netns_dev;
-	__u64 netns_ino;
+	__aligned_u64 netns_dev;
+	__aligned_u64 netns_ino;
 } __attribute__((aligned(8)));
 
 /* User bpf_sock_addr struct to access socket fields and sockaddr struct passed
Signature · 1 line
ldv