BPF uapi structures and 32-bit

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* BPF uapi structures and 32-bit
@ 2018-11-27 22:25 David Miller
  2018-11-28 10:34 ` Daniel Borkmann
  0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2018-11-27 22:25 UTC (permalink / raw)
  To: netdev; +Cc: daniel, ast, john.fastabend

In the linux/bpf.h UAPI header, we must absolutely avoid any
non-fixed-sized types.

Otherwise we have serious problems on 32-bit.

Unfortunately I discovered today that we have take on two such cases,
sk_msg_md and sk_reuseport_md, both of which start with two void
pointers.

I hit this because test_verifier.c on my sparc64 box was built as
a 32-bit binary and this causes a hundred or so tests to fail, and
many if not all are because of the changing struct layout.

I could built 64-bit but this absolutely should work properly.

But for fully native 32-bit it is even worse, this will really
need to be resolved because llvm/clang is always going to build
the BPF programs with 64-bit void pointers.

I would strongly suggest we try and fix this now if we can.

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

* Re: BPF uapi structures and 32-bit
  2018-11-27 22:25 BPF uapi structures and 32-bit David Miller
@ 2018-11-28 10:34 ` Daniel Borkmann
  2018-11-28 19:02   ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2018-11-28 10:34 UTC (permalink / raw)
  To: David Miller, netdev; +Cc: ast, john.fastabend

On 11/27/2018 11:25 PM, David Miller wrote:
> 
> In the linux/bpf.h UAPI header, we must absolutely avoid any
> non-fixed-sized types.
> 
> Otherwise we have serious problems on 32-bit.
> 
> Unfortunately I discovered today that we have take on two such cases,
> sk_msg_md and sk_reuseport_md, both of which start with two void
> pointers.
> 
> I hit this because test_verifier.c on my sparc64 box was built as
> a 32-bit binary and this causes a hundred or so tests to fail, and
> many if not all are because of the changing struct layout.
> 
> I could built 64-bit but this absolutely should work properly.
> 
> But for fully native 32-bit it is even worse, this will really
> need to be resolved because llvm/clang is always going to build
> the BPF programs with 64-bit void pointers.
> 
> I would strongly suggest we try and fix this now if we can.

Yeah fully agree. Thinking diff below should address it, do you
have a chance to give this a spin for sparc / 32 bit to check if
test_verifier still explodes?

Thanks a lot,
Daniel

 include/linux/filter.h         |  7 +++++++
 include/uapi/linux/bpf.h       | 17 ++++++++++++-----
 net/core/filter.c              | 16 ++++++++--------
 tools/include/uapi/linux/bpf.h | 17 ++++++++++++-----
 4 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 448dcc4..795ff0b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -449,6 +449,13 @@ struct sock_reuseport;
 	offsetof(TYPE, MEMBER) ... offsetofend(TYPE, MEMBER) - 1
 #define bpf_ctx_range_till(TYPE, MEMBER1, MEMBER2)				\
 	offsetof(TYPE, MEMBER1) ... offsetofend(TYPE, MEMBER2) - 1
+#if BITS_PER_LONG == 64
+# define bpf_ctx_range_ptr(TYPE, MEMBER)					\
+	offsetof(TYPE, MEMBER) ... offsetofend(TYPE, MEMBER) - 1
+#else
+# define bpf_ctx_range_ptr(TYPE, MEMBER)					\
+	offsetof(TYPE, MEMBER) ... offsetof(TYPE, MEMBER) + 8 - 1
+#endif /* BITS_PER_LONG == 64 */

 #define bpf_target_off(TYPE, MEMBER, SIZE, PTR_SIZE)				\
 	({									\
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 852dc17..426b5c8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2422,6 +2422,12 @@ enum bpf_lwt_encap_mode {
 	BPF_LWT_ENCAP_SEG6_INLINE
 };

+#define __bpf_md_ptr(type, name)	\
+union {					\
+	type name;			\
+	__u64 :64;			\
+} __attribute__((aligned(8)))
+
 /* user accessible mirror of in-kernel sk_buff.
  * new fields can only be added to the end of this structure
  */
@@ -2456,7 +2462,7 @@ struct __sk_buff {
 	/* ... here. */

 	__u32 data_meta;
-	struct bpf_flow_keys *flow_keys;
+	__bpf_md_ptr(struct bpf_flow_keys *, flow_keys);
 };

 struct bpf_tunnel_key {
@@ -2572,8 +2578,8 @@ enum sk_action {
  * be added to the end of this structure
  */
 struct sk_msg_md {
-	void *data;
-	void *data_end;
+	__bpf_md_ptr(void *, data);
+	__bpf_md_ptr(void *, data_end);

 	__u32 family;
 	__u32 remote_ip4;	/* Stored in network byte order */
@@ -2589,8 +2595,9 @@ struct sk_reuseport_md {
 	 * Start of directly accessible data. It begins from
 	 * the tcp/udp header.
 	 */
-	void *data;
-	void *data_end;		/* End of directly accessible data */
+	__bpf_md_ptr(void *, data);
+	/* End of directly accessible data */
+	__bpf_md_ptr(void *, data_end);
 	/*
 	 * Total length of packet (starting from the tcp/udp header).
 	 * Note that the directly accessible bytes (data_end - data)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9a1327e..6ee605d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5435,8 +5435,8 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type
 		if (size != size_default)
 			return false;
 		break;
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
-		if (size != sizeof(struct bpf_flow_keys *))
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
+		if (size != sizeof(__u64))
 			return false;
 		break;
 	default:
@@ -5464,7 +5464,7 @@ static bool sk_filter_is_valid_access(int off, int size,
 	case bpf_ctx_range(struct __sk_buff, data):
 	case bpf_ctx_range(struct __sk_buff, data_meta):
 	case bpf_ctx_range(struct __sk_buff, data_end):
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 	case bpf_ctx_range_till(struct __sk_buff, family, local_port):
 		return false;
 	}
@@ -5489,7 +5489,7 @@ static bool cg_skb_is_valid_access(int off, int size,
 	switch (off) {
 	case bpf_ctx_range(struct __sk_buff, tc_classid):
 	case bpf_ctx_range(struct __sk_buff, data_meta):
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 		return false;
 	case bpf_ctx_range(struct __sk_buff, data):
 	case bpf_ctx_range(struct __sk_buff, data_end):
@@ -5530,7 +5530,7 @@ static bool lwt_is_valid_access(int off, int size,
 	case bpf_ctx_range(struct __sk_buff, tc_classid):
 	case bpf_ctx_range_till(struct __sk_buff, family, local_port):
 	case bpf_ctx_range(struct __sk_buff, data_meta):
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 		return false;
 	}

@@ -5756,7 +5756,7 @@ static bool tc_cls_act_is_valid_access(int off, int size,
 	case bpf_ctx_range(struct __sk_buff, data_end):
 		info->reg_type = PTR_TO_PACKET_END;
 		break;
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 	case bpf_ctx_range_till(struct __sk_buff, family, local_port):
 		return false;
 	}
@@ -5958,7 +5958,7 @@ static bool sk_skb_is_valid_access(int off, int size,
 	switch (off) {
 	case bpf_ctx_range(struct __sk_buff, tc_classid):
 	case bpf_ctx_range(struct __sk_buff, data_meta):
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 		return false;
 	}

@@ -6039,7 +6039,7 @@ static bool flow_dissector_is_valid_access(int off, int size,
 	case bpf_ctx_range(struct __sk_buff, data_end):
 		info->reg_type = PTR_TO_PACKET_END;
 		break;
-	case bpf_ctx_range(struct __sk_buff, flow_keys):
+	case bpf_ctx_range_ptr(struct __sk_buff, flow_keys):
 		info->reg_type = PTR_TO_FLOW_KEYS;
 		break;
 	case bpf_ctx_range(struct __sk_buff, tc_classid):
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 852dc17..426b5c8 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2422,6 +2422,12 @@ enum bpf_lwt_encap_mode {
 	BPF_LWT_ENCAP_SEG6_INLINE
 };

+#define __bpf_md_ptr(type, name)	\
+union {					\
+	type name;			\
+	__u64 :64;			\
+} __attribute__((aligned(8)))
+
 /* user accessible mirror of in-kernel sk_buff.
  * new fields can only be added to the end of this structure
  */
@@ -2456,7 +2462,7 @@ struct __sk_buff {
 	/* ... here. */

 	__u32 data_meta;
-	struct bpf_flow_keys *flow_keys;
+	__bpf_md_ptr(struct bpf_flow_keys *, flow_keys);
 };

 struct bpf_tunnel_key {
@@ -2572,8 +2578,8 @@ enum sk_action {
  * be added to the end of this structure
  */
 struct sk_msg_md {
-	void *data;
-	void *data_end;
+	__bpf_md_ptr(void *, data);
+	__bpf_md_ptr(void *, data_end);

 	__u32 family;
 	__u32 remote_ip4;	/* Stored in network byte order */
@@ -2589,8 +2595,9 @@ struct sk_reuseport_md {
 	 * Start of directly accessible data. It begins from
 	 * the tcp/udp header.
 	 */
-	void *data;
-	void *data_end;		/* End of directly accessible data */
+	__bpf_md_ptr(void *, data);
+	/* End of directly accessible data */
+	__bpf_md_ptr(void *, data_end);
 	/*
 	 * Total length of packet (starting from the tcp/udp header).
 	 * Note that the directly accessible bytes (data_end - data)
-- 
2.9.5

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

* Re: BPF uapi structures and 32-bit
  2018-11-28 10:34 ` Daniel Borkmann
@ 2018-11-28 19:02   ` David Miller
  2018-11-30 23:33     ` Alexei Starovoitov
  0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2018-11-28 19:02 UTC (permalink / raw)
  To: daniel; +Cc: netdev, ast, john.fastabend

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 28 Nov 2018 11:34:55 +0100

> Yeah fully agree. Thinking diff below should address it, do you
> have a chance to give this a spin for sparc / 32 bit to check if
> test_verifier still explodes?

Great, let me play with this.

I did something simpler yesterday, just changing the data pointers to
"u64" and that made at least one test pass that didn't before :-)

I'll get back to you with results.

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

* Re: BPF uapi structures and 32-bit
  2018-11-28 19:02   ` David Miller
@ 2018-11-30 23:33     ` Alexei Starovoitov
  2018-11-30 23:53       ` Daniel Borkmann
  2018-12-01  0:58       ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2018-11-30 23:33 UTC (permalink / raw)
  To: David Miller; +Cc: daniel, netdev, ast, john.fastabend

On Wed, Nov 28, 2018 at 11:02:00AM -0800, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Wed, 28 Nov 2018 11:34:55 +0100
> 
> > Yeah fully agree. Thinking diff below should address it, do you
> > have a chance to give this a spin for sparc / 32 bit to check if
> > test_verifier still explodes?
> 
> Great, let me play with this.
> 
> I did something simpler yesterday, just changing the data pointers to
> "u64" and that made at least one test pass that didn't before :-)
> 
> I'll get back to you with results.

Did you have a chance to test it ?

I'd like to add a tested-by before I apply Daniel's patch
which looks good to me. btw.

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

* Re: BPF uapi structures and 32-bit
  2018-11-30 23:33     ` Alexei Starovoitov
@ 2018-11-30 23:53       ` Daniel Borkmann
  2018-12-01  0:58       ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2018-11-30 23:53 UTC (permalink / raw)
  To: Alexei Starovoitov, David Miller; +Cc: netdev, ast, john.fastabend

On 12/01/2018 12:33 AM, Alexei Starovoitov wrote:
> On Wed, Nov 28, 2018 at 11:02:00AM -0800, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed, 28 Nov 2018 11:34:55 +0100
>>
>>> Yeah fully agree. Thinking diff below should address it, do you
>>> have a chance to give this a spin for sparc / 32 bit to check if
>>> test_verifier still explodes?
>>
>> Great, let me play with this.
>>
>> I did something simpler yesterday, just changing the data pointers to
>> "u64" and that made at least one test pass that didn't before :-)
>>
>> I'll get back to you with results.
> 
> Did you have a chance to test it ?

David got back to me and mentioned it worked fine on sparc.

> I'd like to add a tested-by before I apply Daniel's patch
> which looks good to me. btw.

Yeah, wanted to cook an official patch today, let me do that now.

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

* Re: BPF uapi structures and 32-bit
  2018-11-30 23:33     ` Alexei Starovoitov
  2018-11-30 23:53       ` Daniel Borkmann
@ 2018-12-01  0:58       ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-01  0:58 UTC (permalink / raw)
  To: alexei.starovoitov; +Cc: daniel, netdev, ast, john.fastabend

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 30 Nov 2018 15:33:08 -0800

> On Wed, Nov 28, 2018 at 11:02:00AM -0800, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed, 28 Nov 2018 11:34:55 +0100
>> 
>> > Yeah fully agree. Thinking diff below should address it, do you
>> > have a chance to give this a spin for sparc / 32 bit to check if
>> > test_verifier still explodes?
>> 
>> Great, let me play with this.
>> 
>> I did something simpler yesterday, just changing the data pointers to
>> "u64" and that made at least one test pass that didn't before :-)
>> 
>> I'll get back to you with results.
> 
> Did you have a chance to test it ?
> 
> I'd like to add a tested-by before I apply Daniel's patch
> which looks good to me. btw.

Tested-by: David S. Miller <davem@davemloft.net>

Yes, it does the trick.

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

end of thread, other threads:[~2018-12-01 12:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-27 22:25 BPF uapi structures and 32-bit David Miller
2018-11-28 10:34 ` Daniel Borkmann
2018-11-28 19:02   ` David Miller
2018-11-30 23:33     ` Alexei Starovoitov
2018-11-30 23:53       ` Daniel Borkmann
2018-12-01  0:58       ` David Miller

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