LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
@ 2018-05-27 11:28 Eugene Syromiatnikov
  2018-05-29 17:17 ` Song Liu
  2018-05-30 18:18 ` Dmitry V. Levin
  0 siblings, 2 replies; 7+ messages in thread
From: Eugene Syromiatnikov @ 2018-05-27 11:28 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, Martin KaFai Lau, Daniel Borkmann,
	Alexei Starovoitov, David S. Miller, Jiri Olsa, Ingo Molnar,
	Lawrence Brakmo, Andrey Ignatov, Jakub Kicinski, John Fastabend,
	Dmitry V. Levin

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 <ldv@altlinux.org>
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 <esyr@redhat.com>
---
 include/uapi/linux/bpf.h       | 8 ++++----
 tools/include/uapi/linux/bpf.h | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c5ec897..903010a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -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
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c5ec897..903010a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-05-27 11:28 [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info Eugene Syromiatnikov
@ 2018-05-29 17:17 ` Song Liu
  2018-06-02  3:28   ` Daniel Borkmann
  2018-05-30 18:18 ` Dmitry V. Levin
  1 sibling, 1 reply; 7+ messages in thread
From: Song Liu @ 2018-05-29 17:17 UTC (permalink / raw)
  To: Eugene Syromiatnikov
  Cc: netdev, open list, Martin KaFai Lau, Daniel Borkmann,
	Alexei Starovoitov, David S. Miller, Jiri Olsa, Ingo Molnar,
	Lawrence Brakmo, Andrey Ignatov, Jakub Kicinski, John Fastabend,
	Dmitry V. Levin

On Sun, May 27, 2018 at 4:28 AM, Eugene Syromiatnikov <esyr@redhat.com> wrote:
> 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 <ldv@altlinux.org>
> 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 <esyr@redhat.com>
> ---
>  include/uapi/linux/bpf.h       | 8 ++++----
>  tools/include/uapi/linux/bpf.h | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c5ec897..903010a 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -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

>
>  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
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index c5ec897..903010a 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-05-27 11:28 [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info Eugene Syromiatnikov
  2018-05-29 17:17 ` Song Liu
@ 2018-05-30 18:18 ` Dmitry V. Levin
  2018-06-01  3:12   ` Dmitry V. Levin
  1 sibling, 1 reply; 7+ messages in thread
From: Dmitry V. Levin @ 2018-05-30 18:18 UTC (permalink / raw)
  To: Eugene Syromiatnikov
  Cc: netdev, linux-kernel, Martin KaFai Lau, Daniel Borkmann,
	Alexei Starovoitov, David S. Miller, Jiri Olsa, Ingo Molnar,
	Lawrence Brakmo, Andrey Ignatov, Jakub Kicinski, John Fastabend

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Sun, May 27, 2018 at 01:28:42PM +0200, Eugene Syromiatnikov wrote:
> 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 <ldv@altlinux.org>
> 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" <ldv@altlinux.org>
Cc: <stable@vger.kernel.org> # v4.16+

Thanks,


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-05-30 18:18 ` Dmitry V. Levin
@ 2018-06-01  3:12   ` Dmitry V. Levin
  2018-06-01  8:41     ` Alexei Starovoitov
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry V. Levin @ 2018-06-01  3:12 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Eugene Syromiatnikov, netdev, linux-kernel, Martin KaFai Lau,
	Daniel Borkmann, Alexei Starovoitov, David S. Miller, Jiri Olsa,
	Ingo Molnar, Lawrence Brakmo, Andrey Ignatov, Jakub Kicinski,
	John Fastabend

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

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.

On Wed, May 30, 2018 at 09:18:58PM +0300, Dmitry V. Levin wrote:
> On Sun, May 27, 2018 at 01:28:42PM +0200, Eugene Syromiatnikov wrote:
> > 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 <ldv@altlinux.org>
> > 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 <esyr@redhat.com>
> 
> Reviewed-by: "Dmitry V. Levin" <ldv@altlinux.org>
> Cc: <stable@vger.kernel.org> # v4.16+
> 
> Thanks,
> 
> > ---
> >  include/uapi/linux/bpf.h       | 8 ++++----
> >  tools/include/uapi/linux/bpf.h | 8 ++++----
> >  2 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c5ec897..903010a 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -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
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index c5ec897..903010a 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -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


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-06-01  3:12   ` Dmitry V. Levin
@ 2018-06-01  8:41     ` Alexei Starovoitov
  2018-06-01 11:12       ` Dmitry V. Levin
  0 siblings, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2018-06-01  8:41 UTC (permalink / raw)
  To: Linus Torvalds, Eugene Syromiatnikov, netdev, linux-kernel,
	Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
	David S. Miller, Jiri Olsa, Ingo Molnar, Lawrence Brakmo,
	Andrey Ignatov, Jakub Kicinski, John Fastabend

On Fri, Jun 01, 2018 at 06:12:10AM +0300, Dmitry V. Levin wrote:
> 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.

> On Wed, May 30, 2018 at 09:18:58PM +0300, Dmitry V. Levin wrote:
> > On Sun, May 27, 2018 at 01:28:42PM +0200, Eugene Syromiatnikov wrote:
> > > 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 <ldv@altlinux.org>
> > > 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 <esyr@redhat.com>
> > 
> > Reviewed-by: "Dmitry V. Levin" <ldv@altlinux.org>
> > Cc: <stable@vger.kernel.org> # v4.16+
> > 
> > Thanks,
> > 
> > > ---
> > >  include/uapi/linux/bpf.h       | 8 ++++----
> > >  tools/include/uapi/linux/bpf.h | 8 ++++----
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index c5ec897..903010a 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -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
> > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > index c5ec897..903010a 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -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
> 
> 
> -- 
> ldv

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-06-01  8:41     ` Alexei Starovoitov
@ 2018-06-01 11:12       ` Dmitry V. Levin
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry V. Levin @ 2018-06-01 11:12 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Linus Torvalds, Eugene Syromiatnikov, netdev, linux-kernel,
	Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
	David S. Miller, Jiri Olsa, Ingo Molnar, Lawrence Brakmo,
	Andrey Ignatov, Jakub Kicinski, John Fastabend

[-- Attachment #1: Type: text/plain, Size: 5859 bytes --]

On Fri, Jun 01, 2018 at 04:41:16AM -0400, Alexei Starovoitov wrote:
> On Fri, Jun 01, 2018 at 06:12:10AM +0300, Dmitry V. Levin wrote:
> > 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.

> But the fix in patch 1 is wrong.

Please elaborate.

> 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.

> 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?

> 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/20180527112835.GA9118@asgard.redhat.com
https://lkml.kernel.org/r/20180527112842.GA18204@asgard.redhat.com

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.

> > On Wed, May 30, 2018 at 09:18:58PM +0300, Dmitry V. Levin wrote:
> > > On Sun, May 27, 2018 at 01:28:42PM +0200, Eugene Syromiatnikov wrote:
> > > > 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 <ldv@altlinux.org>
> > > > 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 <esyr@redhat.com>
> > > 
> > > Reviewed-by: "Dmitry V. Levin" <ldv@altlinux.org>
> > > Cc: <stable@vger.kernel.org> # v4.16+
> > > 
> > > Thanks,
> > > 
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 8 ++++----
> > > >  tools/include/uapi/linux/bpf.h | 8 ++++----
> > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index c5ec897..903010a 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -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
> > > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > > index c5ec897..903010a 100644
> > > > --- a/tools/include/uapi/linux/bpf.h
> > > > +++ b/tools/include/uapi/linux/bpf.h
> > > > @@ -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


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info
  2018-05-29 17:17 ` Song Liu
@ 2018-06-02  3:28   ` Daniel Borkmann
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2018-06-02  3:28 UTC (permalink / raw)
  To: Song Liu, Eugene Syromiatnikov
  Cc: netdev, open list, Martin KaFai Lau, Alexei Starovoitov,
	David S. Miller, Jiri Olsa, Ingo Molnar, Lawrence Brakmo,
	Andrey Ignatov, Jakub Kicinski, John Fastabend, Dmitry V. Levin

On 05/29/2018 07:17 PM, Song Liu wrote:
> On Sun, May 27, 2018 at 4:28 AM, Eugene Syromiatnikov <esyr@redhat.com> wrote:
>> 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 <ldv@altlinux.org>
>> 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 <esyr@redhat.com>
>> ---
>>  include/uapi/linux/bpf.h       | 8 ++++----
>>  tools/include/uapi/linux/bpf.h | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index c5ec897..903010a 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -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/.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-06-02  3:29 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-27 11:28 [PATCH bpf 1/2] bpf: fix alignment of netns_dev/netns_ino fields in bpf_{map,prog}_info Eugene Syromiatnikov
2018-05-29 17:17 ` Song Liu
2018-06-02  3:28   ` Daniel Borkmann
2018-05-30 18:18 ` Dmitry V. Levin
2018-06-01  3:12   ` Dmitry V. Levin
2018-06-01  8:41     ` Alexei Starovoitov
2018-06-01 11:12       ` Dmitry V. Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).