netdev - Re: [PATCH net] gro: Make GRO aware of lightweight tunnels.

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: <CAEh+42g+bc-+=qjZ=Ha8580aZf5=o+2CS6pM1m-gKu47sbPTxw@mail.gmail.com>
Date:	Wed, 20 Jan 2016 17:47:48 -0800
From:	Jesse Gross <jesse@...nel.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Thomas Graf <tgraf@...g.ch>, John <john.phillips5@....com>
Subject: Re: [PATCH net] gro: Make GRO aware of lightweight tunnels.

On Wed, Jan 20, 2016 at 4:48 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Wed, 2016-01-20 at 16:27 -0800, Jesse Gross wrote:
>> GRO is currently not aware of tunnel metadata generated by lightweight
>> tunnels and stored in the dst. This leads to two possible problems:
>>  * Incorrectly merging two frames that have different metadata.
>>  * Leaking of allocated metadata from merged frames.
>>
>> This avoids those problems by comparing the tunnel information before
>> merging, similar to how we handle other metadata (such as vlan tags),
>> and releasing any state when we are done.
>>
>> Reported-by: John <john.phillips5@....com>
>> Fixes: 2e15ea39 ("ip_gre: Add support to collect tunnel metadata.")
>> Signed-off-by: Jesse Gross <jesse@...nel.org>
>> ---
>>  include/net/dst_metadata.h | 23 +++++++++++++++++++++++
>>  net/core/dev.c             |  9 +++++++--
>>  2 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
>> index 6816f0f..c3de935 100644
>> --- a/include/net/dst_metadata.h
>> +++ b/include/net/dst_metadata.h
>> @@ -44,6 +44,29 @@ static inline bool skb_valid_dst(const struct sk_buff *skb)
>>       return dst && !(dst->flags & DST_METADATA);
>>  }
>>
>> +static inline int skb_metadata_dst_cmp(struct sk_buff *skb_a,
>> +                                    struct sk_buff *skb_b)
>> +{
>> +     const struct metadata_dst *a = skb_metadata_dst(skb_a);
>> +     const struct metadata_dst *b = skb_metadata_dst(skb_b);
>> +
>> +     if (!a != !b)
>> +             return 1;
>> +
>> +     if (!a)
>> +             return 0;
>> +
>
> It is adding 4 conditional test per flow in GRO engine for the fast
> path...
>
> With up to 8 flows in GRO (per RX queue), it is a total of 32 added
> conditional tests.
>
> You should shortcut to one test :
>
> if (!(skb_a->_skb_refdst | skb_b->_skb_refdst)
>         return 0;

Thanks, that's a good idea. I'll send out a v2 soon.

Just to merge the two threads together, all of protocols that would be
affected by this also have "normal" GRO handlers that will run when
the packet is first received. There's no argument that that is
preferable if it is available. However, GRO cells do provide a
performance benefit in other situations so it would be nice to avoid
disabling it if possible.