linux-kernel - Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata

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]

Message-ID: <5b817d49-eefa-51c9-3b51-01f1dba17d42@intel.com>
Date:   Fri, 19 May 2023 18:35:44 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
CC:     Larysa Zaremba <larysa.zaremba@...el.com>, <brouer@...hat.com>,
        <bpf@...r.kernel.org>, Stanislav Fomichev <sdf@...gle.com>,
        "Alexei Starovoitov" <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Andrii Nakryiko" <andrii@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "Martin KaFai Lau" <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>,
        <xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
        <intel-wired-lan@...ts.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32

From: Jesper Dangaard Brouer <jbrouer@...hat.com>
Date: Tue, 16 May 2023 17:35:27 +0200

> 
> 
> On 16/05/2023 14.37, Alexander Lobakin wrote:
>> From: Larysa Zaremba<larysa.zaremba@...el.com>
>> Date: Mon, 15 May 2023 19:08:39 +0200
>>
>>> On Mon, May 15, 2023 at 06:17:02PM +0200, Jesper Dangaard Brouer wrote:
>>>>
>>>> On 12/05/2023 17.26, Larysa Zaremba wrote:
>>>>> From: Aleksander Lobakin<aleksander.lobakin@...el.com>
>>>>>
>>>>> When using XDP hints, metadata sometimes has to be much bigger
>>>>> than 32 bytes. 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 maximum possible value is 255.
>>>>
>>>> I'm confused, IIRC the metadata area isn't stored "in skb_shared_info".
>>>> The maximum possible size is limited by the XDP headroom, which is also
>>>> shared/limited with/by xdp_frame.  I must be reading the sentence
>>>> wrong,
>>>> somehow.
>>
>> skb_shared_info::meta_size  is u8. Since metadata gets carried from
>> xdp_buff to skb, this check is needed (it's compile-time constant
>> anyway).
>> Check for headroom is done separately already (two sentences below).
>>
> 
> Damn, argh, for SKBs the "meta_len" is stored in skb_shared_info, which
> is located on another cacheline.
> That is a sure way to KILL performance! :-(

Have you read the code? I use type_max(typeof_member(shinfo, meta_len)),
what performance are you talking about?

The whole xdp_metalen_invalid() gets expanded into:

	return (metalen % 4) || metalen > 255;

at compile-time. All those typeof shenanigans are only to not open-code
meta_len's type/size/max.

> 
> But only use for SKBs that gets created from xdp with metadata, right?
> 
> 
> 
>>> It's not 'metadata is stored as u8', it's 'metadata size is stored as
>>> u8' :)
>>> Maybe I should rephrase it better in v2.
> 
> Yes, a rephrase will be good.
> 
> --Jesper
> 
> 
> 
> static inline u8 skb_metadata_len(const struct sk_buff *skb)
> {
>     return skb_shinfo(skb)->meta_len;
> }
> 

Thanks,
Olek