net: Fix missing meta data in skb with vlan packet
diff mbox series

Message ID CAGCJULOjetohzdyuMN=jfKPMx8_a4WUa2r=9whNvK22xNqhqNw@mail.gmail.com
State Superseded
Delegated to: David Miller
Headers show
Series
  • net: Fix missing meta data in skb with vlan packet
Related show

Commit Message

Yuya Kusakabe April 13, 2019, 12:16 p.m. UTC
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(-)

 }
--
2.20.1

Comments

Toshiaki Makita April 15, 2019, 6:05 a.m. UTC | #1
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
> 
>
Yuya Kusakabe April 15, 2019, 8:06 a.m. UTC | #2
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.
Toshiaki Makita April 15, 2019, 8:07 a.m. UTC | #3
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>
Yuya Kusakabe April 15, 2019, 8:28 a.m. UTC | #4
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!
Toshiaki Makita April 15, 2019, 9:48 a.m. UTC | #5
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
Jesper Dangaard Brouer April 15, 2019, 9:57 a.m. UTC | #6
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);
> > +       };
> > +
Toshiaki Makita April 15, 2019, 10:13 a.m. UTC | #7
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);
>>> +       };
>>> +

Patch
diff mbox series

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;