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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 15 Apr 2019 17:06:48 +0900
From:   Yuya Kusakabe <yuya.kusakabe@...il.com>
To:     Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Cc:     davem@...emloft.net, taketarou2@...il.com, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH] net: Fix missing meta data in skb with vlan packet

Hi,

Thank you for the review.

On Mon, Apr 15, 2019 at 3:06 PM Toshiaki Makita
<makita.toshiaki@....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@...il.com>
> > Signed-off-by: Takeru Hayasaka <taketarou2@...il.com>
> > Co-developed-by: Takeru Hayasaka <taketarou2@...il.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