[PATCH bpf-next v3 0/2] Allow data_meta size > 32

[PATCH bpf-next v3 0/2] Allow data_meta size > 32

[Larysa Zaremba]

Currently, there is no reason for data_meta to be limited to 32 bytes.
Loosen this limitation and make maximum meta size 252.

Also, modify the selftest, so test_xdp_context_error does not complain
about the unexpected success.

v2->v3:
* Fix main patch author
* Add selftests path

v1->v2:
* replace 'typeof(metalen)' with the actual type

Aleksander Lobakin (1):
  net, xdp: allow metadata > 32

Larysa Zaremba (1):
  selftests/bpf: increase invalid metadata size

 include/linux/skbuff.h                              | 13 ++++++++-----
 include/net/xdp.h                                   |  7 ++++++-
 .../selftests/bpf/prog_tests/xdp_context_test_run.c |  4 ++--
 3 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH bpf-next v3 1/2] selftests/bpf: increase invalid metadata size

[Larysa Zaremba]

Changed check expects passed data meta to be deemed invalid.
After loosening the requirement, the size of 36 bytes becomes valid.
Therefore, increase tested meta size to 256, so we do not get an unexpected
success.

Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
---
 tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
index ab4952b9fb1d..e6a783c7f5db 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
@@ -77,8 +77,8 @@ void test_xdp_context_test_run(void)
 	test_xdp_context_error(prog_fd, opts, 4, sizeof(__u32), sizeof(data),
 			       0, 0, 0);
 
-	/* Meta data must be 32 bytes or smaller */
-	test_xdp_context_error(prog_fd, opts, 0, 36, sizeof(data), 0, 0, 0);
+	/* Meta data must be 255 bytes or smaller */
+	test_xdp_context_error(prog_fd, opts, 0, 256, sizeof(data), 0, 0, 0);
 
 	/* Total size of data must match data_end - data_meta */
 	test_xdp_context_error(prog_fd, opts, 0, sizeof(__u32),
-- 
2.41.0

[PATCH bpf-next v3 2/2] net, xdp: allow metadata > 32

[Larysa Zaremba]

From: Aleksander Lobakin <aleksander.lobakin@intel.com>

32 bytes may be not enough for some custom metadata. Relax the restriction,
allow metadata larger than 32 bytes and make __skb_metadata_differs() work
with bigger lengths.

Now size of metadata is only limited by the fact it is stored as u8
in skb_shared_info, so the upper limit is now is 255. Other important
conditions, such as having enough space for xdp_frame building, are already
checked in bpf_xdp_adjust_meta().

The requirement of having its length aligned to 4 bytes is still
valid.

Signed-off-by: Aleksander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
---
 include/linux/skbuff.h | 13 ++++++++-----
 include/net/xdp.h      |  7 ++++++-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 27998f73183e..6520ac186d96 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4235,10 +4235,13 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
 {
 	const void *a = skb_metadata_end(skb_a);
 	const void *b = skb_metadata_end(skb_b);
-	/* Using more efficient varaiant than plain call to memcmp(). */
-#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
 	u64 diffs = 0;
 
+	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ||
+	    BITS_PER_LONG != 64)
+		goto slow;
+
+	/* Using more efficient variant than plain call to memcmp(). */
 	switch (meta_len) {
 #define __it(x, op) (x -= sizeof(u##op))
 #define __it_diff(a, b, op) (*(u##op *)__it(a, op)) ^ (*(u##op *)__it(b, op))
@@ -4258,11 +4261,11 @@ static inline bool __skb_metadata_differs(const struct sk_buff *skb_a,
 		fallthrough;
 	case  4: diffs |= __it_diff(a, b, 32);
 		break;
+	default:
+slow:
+		return memcmp(a - meta_len, b - meta_len, meta_len);
 	}
 	return diffs;
-#else
-	return memcmp(a - meta_len, b - meta_len, meta_len);
-#endif
 }
 
 static inline bool skb_metadata_differs(const struct sk_buff *skb_a,
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 349c36fb5fd8..5d3673afc037 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -369,7 +369,12 @@ xdp_data_meta_unsupported(const struct xdp_buff *xdp)
 
 static inline bool xdp_metalen_invalid(unsigned long metalen)
 {
-	return (metalen & (sizeof(__u32) - 1)) || (metalen > 32);
+	unsigned long meta_max;
+
+	meta_max = type_max(typeof_member(struct skb_shared_info, meta_len));
+	BUILD_BUG_ON(!__builtin_constant_p(meta_max));
+
+	return !IS_ALIGNED(metalen, sizeof(u32)) || metalen > meta_max;
 }
 
 struct xdp_attachment_info {
-- 
2.41.0

Re: [PATCH bpf-next v3 0/2] Allow data_meta size > 32

[Jesper Dangaard Brouer]

On 11/27/23 19:32, Larysa Zaremba wrote:
> Currently, there is no reason for data_meta to be limited to 32 bytes.
> Loosen this limitation and make maximum meta size 252.

First I though you made a type here with 252 bytes, but then I 
remembered the 4 byte alignment.
I think commit message should elaborate on why 252 bytes.

> 
> Also, modify the selftest, so test_xdp_context_error does not complain
> about the unexpected success.
> 
> v2->v3:
> * Fix main patch author
> * Add selftests path
> 
> v1->v2:
> * replace 'typeof(metalen)' with the actual type
> 
> Aleksander Lobakin (1):
>    net, xdp: allow metadata > 32
> 
> Larysa Zaremba (1):
>    selftests/bpf: increase invalid metadata size
> 
>   include/linux/skbuff.h                              | 13 ++++++++-----
>   include/net/xdp.h                                   |  7 ++++++-
>   .../selftests/bpf/prog_tests/xdp_context_test_run.c |  4 ++--
>   3 files changed, 16 insertions(+), 8 deletions(-)
> 

Re: [PATCH bpf-next v3 0/2] Allow data_meta size > 32

[Larysa Zaremba]

On Tue, Nov 28, 2023 at 11:26:59AM +0100, Jesper Dangaard Brouer wrote:
> 
> 
> On 11/27/23 19:32, Larysa Zaremba wrote:
> > Currently, there is no reason for data_meta to be limited to 32 bytes.
> > Loosen this limitation and make maximum meta size 252.
> 
> First I though you made a type here with 252 bytes, but then I remembered
> the 4 byte alignment.
> I think commit message should elaborate on why 252 bytes.
>

You are right, will do.
 
> > 
> > Also, modify the selftest, so test_xdp_context_error does not complain
> > about the unexpected success.
> > 
> > v2->v3:
> > * Fix main patch author
> > * Add selftests path
> > 
> > v1->v2:
> > * replace 'typeof(metalen)' with the actual type
> > 
> > Aleksander Lobakin (1):
> >    net, xdp: allow metadata > 32
> > 
> > Larysa Zaremba (1):
> >    selftests/bpf: increase invalid metadata size
> > 
> >   include/linux/skbuff.h                              | 13 ++++++++-----
> >   include/net/xdp.h                                   |  7 ++++++-
> >   .../selftests/bpf/prog_tests/xdp_context_test_run.c |  4 ++--
> >   3 files changed, 16 insertions(+), 8 deletions(-)
> > 

Re: [PATCH bpf-next v3 0/2] Allow data_meta size > 32

[Jesper Dangaard Brouer]

On 11/28/23 11:33, Larysa Zaremba wrote:
> On Tue, Nov 28, 2023 at 11:26:59AM +0100, Jesper Dangaard Brouer wrote:
>>
>>
>> On 11/27/23 19:32, Larysa Zaremba wrote:
>>> Currently, there is no reason for data_meta to be limited to 32 bytes.
>>> Loosen this limitation and make maximum meta size 252.
>>
>> First I though you made a type here with 252 bytes, but then I remembered
>> the 4 byte alignment.
>> I think commit message should elaborate on why 252 bytes.
>>
> 
> You are right, will do.
>   

I'm looking at the code to see if metadata can be extended into area
used by xdp_frame, such that a BPF-prog get direct memory access to this
(which should not be allowed).  The bpf_xdp_adjust_meta() helper does
handle this (as you/Olek also mentioned in commit msg).  A driver could
set data_meta such that they overlap, but I guess we will categorize
this as a driver bug.

The XDP headroom have evolved to become dynamic (commonly 192 or 256
bytes). Thus, we cannot statically reduce metalen with sizeof(struct
xdp_frame).  The maximum meta size 252, could be achieved (and valid) if
a driver chooses to have more XDP headroom e.g. 288 bytes. So, I guess
it is correct to say maximum valid meta size is 252 bytes, but can be
limited and reduced further by drivers chosen XDP headroom and memory
reserved by struct xdp_frame (limited in bpf_xdp_adjust_meta).



>>>
>>> Also, modify the selftest, so test_xdp_context_error does not complain
>>> about the unexpected success.
>>>
>>> v2->v3:
>>> * Fix main patch author
>>> * Add selftests path
>>>
>>> v1->v2:
>>> * replace 'typeof(metalen)' with the actual type
>>>
>>> Aleksander Lobakin (1):
>>>     net, xdp: allow metadata > 32
>>>
>>> Larysa Zaremba (1):
>>>     selftests/bpf: increase invalid metadata size
>>>
>>>    include/linux/skbuff.h                              | 13 ++++++++-----
>>>    include/net/xdp.h                                   |  7 ++++++-
>>>    .../selftests/bpf/prog_tests/xdp_context_test_run.c |  4 ++--
>>>    3 files changed, 16 insertions(+), 8 deletions(-)
>>>

Re: [PATCH bpf-next v3 0/2] Allow data_meta size > 32

[Alexander Lobakin]

From: Jesper Dangaard Brouer <hawk@kernel.org>
Date: Tue, 28 Nov 2023 12:30:41 +0100

> 
> 
> On 11/28/23 11:33, Larysa Zaremba wrote:
>> On Tue, Nov 28, 2023 at 11:26:59AM +0100, Jesper Dangaard Brouer wrote:
>>>
>>>
>>> On 11/27/23 19:32, Larysa Zaremba wrote:
>>>> Currently, there is no reason for data_meta to be limited to 32 bytes.
>>>> Loosen this limitation and make maximum meta size 252.
>>>
>>> First I though you made a type here with 252 bytes, but then I
>>> remembered
>>> the 4 byte alignment.
>>> I think commit message should elaborate on why 252 bytes.
>>>
>>
>> You are right, will do.
>>   
> 
> I'm looking at the code to see if metadata can be extended into area
> used by xdp_frame, such that a BPF-prog get direct memory access to this
> (which should not be allowed).  The bpf_xdp_adjust_meta() helper does
> handle this (as you/Olek also mentioned in commit msg).  A driver could
> set data_meta such that they overlap, but I guess we will categorize
> this as a driver bug.
> 
> The XDP headroom have evolved to become dynamic (commonly 192 or 256
> bytes). Thus, we cannot statically reduce metalen with sizeof(struct
> xdp_frame).  The maximum meta size 252, could be achieved (and valid) if
> a driver chooses to have more XDP headroom e.g. 288 bytes. So, I guess
> it is correct to say maximum valid meta size is 252 bytes, but can be
> limited and reduced further by drivers chosen XDP headroom and memory
> reserved by struct xdp_frame (limited in bpf_xdp_adjust_meta).

Drivers which don't provide 256 bytes of headroom for XDP are
pathological :p

Either way, as Larysa and you mentioned, the actual available headroom
is checked with adjust_meta(), so here we just make sure its size fits
into u8 meta_len of &skb_shared_info.

> 
> 
> 
>>>>
>>>> Also, modify the selftest, so test_xdp_context_error does not complain
>>>> about the unexpected success.
>>>>
>>>> v2->v3:
>>>> * Fix main patch author
>>>> * Add selftests path
>>>>
>>>> v1->v2:
>>>> * replace 'typeof(metalen)' with the actual type
>>>>
>>>> Aleksander Lobakin (1):
>>>>     net, xdp: allow metadata > 32
>>>>
>>>> Larysa Zaremba (1):
>>>>     selftests/bpf: increase invalid metadata size
>>>>
>>>>    include/linux/skbuff.h                              | 13
>>>> ++++++++-----
>>>>    include/net/xdp.h                                   |  7 ++++++-
>>>>    .../selftests/bpf/prog_tests/xdp_context_test_run.c |  4 ++--
>>>>    3 files changed, 16 insertions(+), 8 deletions(-)
>>>>

Thanks,
Olek