[PATCH] net: Fix missing meta data in skb with vlan packet

[PATCH] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

skb_reorder_vlan_header() should move XDP meta data with ethernet
header if XDP meta data exists.

Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
---
 net/core/skbuff.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ef2cd5712098..6bc663249c4c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);

 static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 {
-       int mac_len;
+       int mac_len, meta_len;
+       void *meta;

        if (skb_cow(skb, skb_headroom(skb)) < 0) {
                kfree_skb(skb);
@@ -5095,6 +5096,13 @@ static struct sk_buff
*skb_reorder_vlan_header(struct sk_buff *skb)
                memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
                        mac_len - VLAN_HLEN - ETH_TLEN);
        }
+
+       meta_len = skb_metadata_len(skb);
+       if (meta_len) {
+               meta = skb_metadata_end(skb) - meta_len;
+               memmove(meta + VLAN_HLEN, meta, meta_len);
+       };
+
        skb->mac_header += VLAN_HLEN;
        return skb;
 }
--
2.20.1

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

Hi,
(CC: XDP maintainers)

On 2019/04/13 21:16, Yuya Kusakabe wrote:
> skb_reorder_vlan_header() should move XDP meta data with ethernet
> header if XDP meta data exists.
> 
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
> Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>

Missing fixes tag

Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")

> ---
>  net/core/skbuff.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ef2cd5712098..6bc663249c4c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
> 
>  static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>  {
> -       int mac_len;
> +       int mac_len, meta_len;
> +       void *meta;
> 
>         if (skb_cow(skb, skb_headroom(skb)) < 0) {
>                 kfree_skb(skb);
> @@ -5095,6 +5096,13 @@ static struct sk_buff
> *skb_reorder_vlan_header(struct sk_buff *skb)
>                 memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
>                         mac_len - VLAN_HLEN - ETH_TLEN);
>         }
> +
> +       meta_len = skb_metadata_len(skb);
> +       if (meta_len) {

Since this is not used by non-XDP skb and skb path is slow-path for XDP
anyway, should add unlikely here in favor of non-XDP case?

> +               meta = skb_metadata_end(skb) - meta_len;
> +               memmove(meta + VLAN_HLEN, meta, meta_len);
> +       };
> +
>         skb->mac_header += VLAN_HLEN;
>         return skb;
>  }
> --
> 2.20.1
> 
> 

-- 
Toshiaki Makita

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

Hi,

Thank you for the review.

On Mon, Apr 15, 2019 at 3:06 PM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> Hi,
> (CC: XDP maintainers)
>
> On 2019/04/13 21:16, Yuya Kusakabe wrote:
> > skb_reorder_vlan_header() should move XDP meta data with ethernet
> > header if XDP meta data exists.
> >
> > Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> > Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
> > Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
>
> Missing fixes tag
>
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")

I will add this line in v2 patch.

>
>
> > ---
> >  net/core/skbuff.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index ef2cd5712098..6bc663249c4c 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
> >
> >  static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
> >  {
> > -       int mac_len;
> > +       int mac_len, meta_len;
> > +       void *meta;
> >
> >         if (skb_cow(skb, skb_headroom(skb)) < 0) {
> >                 kfree_skb(skb);
> > @@ -5095,6 +5096,13 @@ static struct sk_buff
> > *skb_reorder_vlan_header(struct sk_buff *skb)
> >                 memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
> >                         mac_len - VLAN_HLEN - ETH_TLEN);
> >         }
> > +
> > +       meta_len = skb_metadata_len(skb);
> > +       if (meta_len) {
>
> Since this is not used by non-XDP skb and skb path is slow-path for XDP
> anyway, should add unlikely here in favor of non-XDP case?

I referred to meta data support of the ixgbe driver. It doesn't use "unlikely".
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176

>
>
> > +               meta = skb_metadata_end(skb) - meta_len;
> > +               memmove(meta + VLAN_HLEN, meta, meta_len);
> > +       };
> > +
> >         skb->mac_header += VLAN_HLEN;
> >         return skb;
> >  }
> > --
> > 2.20.1
> >
> >
>
> --
> Toshiaki Makita
>

Thank you.

-- 
Yuya Kusakabe

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

On 2019/04/15 17:06, Yuya Kusakabe wrote:
...
>>> @@ -5095,6 +5096,13 @@ static struct sk_buff
>>> *skb_reorder_vlan_header(struct sk_buff *skb)
...
>>> +               meta = skb_metadata_end(skb) - meta_len;
>>> +               memmove(meta + VLAN_HLEN, meta, meta_len);
>>> +       };

I just noticed that there is an unnecessary semicolon here.
Not sure if checkpatch.pl can detect this, but if you did not use it,
please try it.

Also it seems your patch is corrupted and tabs are converted into
whitespaces. Please check if you can apply the patch by sending your
patch to yourself, then send to the list.

Thanks,
Toshiaki Makita

[PATCH v2] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

skb_reorder_vlan_header() should move XDP meta data with ethernet header
if XDP meta data exists.

Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
---
 net/core/skbuff.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ef2cd5712098..6bc663249c4c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
 
 static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 {
-	int mac_len;
+	int mac_len, meta_len;
+	void *meta;
 
 	if (skb_cow(skb, skb_headroom(skb)) < 0) {
 		kfree_skb(skb);
@@ -5095,6 +5096,13 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 		memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
 			mac_len - VLAN_HLEN - ETH_TLEN);
 	}
+
+	meta_len = skb_metadata_len(skb);
+	if (meta_len) {
+		meta = skb_metadata_end(skb) - meta_len;
+		memmove(meta + VLAN_HLEN, meta, meta_len);
+	};
+
 	skb->mac_header += VLAN_HLEN;
 	return skb;
 }
-- 
2.20.1

Re: [PATCH v2] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

On 2019/04/15 23:21, Yuya Kusakabe wrote:
> skb_reorder_vlan_header() should move XDP meta data with ethernet header
> if XDP meta data exists.
> 
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
> Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
> ---
...
> @@ -5095,6 +5096,13 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>  		memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
>  			mac_len - VLAN_HLEN - ETH_TLEN);
>  	}
> +
> +	meta_len = skb_metadata_len(skb);
> +	if (meta_len) {
> +		meta = skb_metadata_end(skb) - meta_len;
> +		memmove(meta + VLAN_HLEN, meta, meta_len);
> +	};

You still have extra trailing semicolon.

> +
>  	skb->mac_header += VLAN_HLEN;
>  	return skb;
>  }
> 

-- 
Toshiaki Makita

Re: [PATCH v2] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

On 4/16/19 9:45 AM, Toshiaki Makita wrote:
> On 2019/04/15 23:21, Yuya Kusakabe wrote:
>> skb_reorder_vlan_header() should move XDP meta data with ethernet header
>> if XDP meta data exists.
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
>> Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
>> ---
> ...
>> @@ -5095,6 +5096,13 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>>  		memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
>>  			mac_len - VLAN_HLEN - ETH_TLEN);
>>  	}
>> +
>> +	meta_len = skb_metadata_len(skb);
>> +	if (meta_len) {
>> +		meta = skb_metadata_end(skb) - meta_len;
>> +		memmove(meta + VLAN_HLEN, meta, meta_len);
>> +	};
> 
> You still have extra trailing semicolon.

Sorry. I'll make the v3 patch soon.

> 
>> +
>>  	skb->mac_header += VLAN_HLEN;
>>  	return skb;
>>  }
>>
> 

[PATCH v3] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

skb_reorder_vlan_header() should move XDP meta data with ethernet header
if XDP meta data exists.

Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>
---
 net/core/skbuff.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ef2cd5712098..40796b8bf820 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
 
 static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 {
-	int mac_len;
+	int mac_len, meta_len;
+	void *meta;
 
 	if (skb_cow(skb, skb_headroom(skb)) < 0) {
 		kfree_skb(skb);
@@ -5095,6 +5096,13 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
 		memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
 			mac_len - VLAN_HLEN - ETH_TLEN);
 	}
+
+	meta_len = skb_metadata_len(skb);
+	if (meta_len) {
+		meta = skb_metadata_end(skb) - meta_len;
+		memmove(meta + VLAN_HLEN, meta, meta_len);
+	}
+
 	skb->mac_header += VLAN_HLEN;
 	return skb;
 }
-- 
2.20.1

Re: [PATCH v3] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

On 2019/04/16 10:22, Yuya Kusakabe wrote:
> skb_reorder_vlan_header() should move XDP meta data with ethernet header
> if XDP meta data exists.
> 
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
> Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>

Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

-- 
Toshiaki Makita

Re: [PATCH v3] net: Fix missing meta data in skb with vlan packet

[David Miller]

From: Yuya Kusakabe <yuya.kusakabe@gmail.com>
Date: Tue, 16 Apr 2019 10:22:28 +0900

> skb_reorder_vlan_header() should move XDP meta data with ethernet header
> if XDP meta data exists.
> 
> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
> Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com>
> Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com>

Applied and queued up for -stable, thank you.

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

On 2019/04/15 16:43, Yuya Kusakabe wrote:
...
>     > @@ -5095,6 +5096,13 @@ static struct sk_buff
>     > *skb_reorder_vlan_header(struct sk_buff *skb)
>     >                 memmove(skb_mac_header(skb) + VLAN_HLEN,
>     skb_mac_header(skb),
>     >                         mac_len - VLAN_HLEN - ETH_TLEN);
>     >         }
>     > +
>     > +       meta_len = skb_metadata_len(skb);
>     > +       if (meta_len) {
> 
>     Since this is not used by non-XDP skb and skb path is slow-path for XDP
>     anyway, should add unlikely here in favor of non-XDP case?
> 
> 
> I referred to meta data support of the ixgbe driver. It doesn't use
> "unlikely".
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176

No need to conform to existing logic when we can improve it...
Anyway this is a minor point and I'm OK with either.

Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

-- 
Toshiaki Makita

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Yuya Kusakabe]

On Mon, Apr 15, 2019 at 5:08 PM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> On 2019/04/15 16:43, Yuya Kusakabe wrote:
> ...
> >     > @@ -5095,6 +5096,13 @@ static struct sk_buff
> >     > *skb_reorder_vlan_header(struct sk_buff *skb)
> >     >                 memmove(skb_mac_header(skb) + VLAN_HLEN,
> >     skb_mac_header(skb),
> >     >                         mac_len - VLAN_HLEN - ETH_TLEN);
> >     >         }
> >     > +
> >     > +       meta_len = skb_metadata_len(skb);
> >     > +       if (meta_len) {
> >
> >     Since this is not used by non-XDP skb and skb path is slow-path for XDP
> >     anyway, should add unlikely here in favor of non-XDP case?
> >
> >
> > I referred to meta data support of the ixgbe driver. It doesn't use
> > "unlikely".
> > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176
>
> No need to conform to existing logic when we can improve it...

I understand.

> Anyway this is a minor point and I'm OK with either.
>
> Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>
> --
> Toshiaki Makita
>

Thank you!

-- 
Yuya Kusakabe

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Jesper Dangaard Brouer]

On Mon, 15 Apr 2019 15:05:36 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index ef2cd5712098..6bc663249c4c 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
[...]
> > @@ -5095,6 +5096,13 @@ static struct sk_buff
> > *skb_reorder_vlan_header(struct sk_buff *skb)
> >                 memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
> >                         mac_len - VLAN_HLEN - ETH_TLEN);
> >         }
> > +
> > +       meta_len = skb_metadata_len(skb);
> > +       if (meta_len) {  
> 
> Since this is not used by non-XDP skb and skb path is slow-path for XDP
> anyway, should add unlikely here in favor of non-XDP case?

I want to stress, that XDP is meant to cooperate with network stack. 
The XDP metadata is meant as a communication channel between XDP and
network-stack SKBs.  

Thus, there are use-cases where this is not considered a slow-path.
That said I don't care if this is marked unlikely(), as not many people
are using this metadata communication channel yet.

In one customer use-case I have seen, they wanted to pop VLAN tags
(Q-in-Q) at XDP layer, but keep some of the info in metadata for TC-bpf
hook. I don't think they kept/left any VLAN headers in the SKB.


> > +               meta = skb_metadata_end(skb) - meta_len;
> > +               memmove(meta + VLAN_HLEN, meta, meta_len);
> > +       };
> > +



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH] net: Fix missing meta data in skb with vlan packet

[Toshiaki Makita]

On 2019/04/15 18:57, Jesper Dangaard Brouer wrote:
> On Mon, 15 Apr 2019 15:05:36 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index ef2cd5712098..6bc663249c4c 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
> [...]
>>> @@ -5095,6 +5096,13 @@ static struct sk_buff
>>> *skb_reorder_vlan_header(struct sk_buff *skb)
>>>                 memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb),
>>>                         mac_len - VLAN_HLEN - ETH_TLEN);
>>>         }
>>> +
>>> +       meta_len = skb_metadata_len(skb);
>>> +       if (meta_len) {  
>>
>> Since this is not used by non-XDP skb and skb path is slow-path for XDP
>> anyway, should add unlikely here in favor of non-XDP case?
> 
> I want to stress, that XDP is meant to cooperate with network stack. 
> The XDP metadata is meant as a communication channel between XDP and
> network-stack SKBs.  
> 
> Thus, there are use-cases where this is not considered a slow-path.

Ah, that's true. Definitely there are cases where skb-path performance
is also important with XDP enabled. Sorry about the confusion.

> That said I don't care if this is marked unlikely(), as not many people
> are using this metadata communication channel yet.
> 
> In one customer use-case I have seen, they wanted to pop VLAN tags
> (Q-in-Q) at XDP layer, but keep some of the info in metadata for TC-bpf
> hook. I don't think they kept/left any VLAN headers in the SKB.
> 
> 
>>> +               meta = skb_metadata_end(skb) - meta_len;
>>> +               memmove(meta + VLAN_HLEN, meta, meta_len);
>>> +       };
>>> +

-- 
Toshiaki Makita