netdev - Re: [PATCH net-next] gro: relax ID check in inet_gro

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:	Thu, 21 Mar 2013 02:59:49 -0700
From:	Maciej Żenczykowski <maze@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Dmitry Kravkov <dmitry@...adcom.com>,
	Eilon Greenstein <eilong@...adcom.com>,
	Pravin B Shelar <pshelar@...ira.com>,
	"H.K. Jerry Chu" <hkchu@...gle.com>
Subject: Re: [PATCH net-next] gro: relax ID check in inet_gro_receive()

Ack.

I've never understood the usefulness of the 'IP ID increments by one'
check in the GRO TCP path anyway.

TCP packets are DF.
AFAICT, the IP identifier field does not really serve a useful purpose
for non-fragment-ed/able packets.

The only possible exception I can think of has to do with
broken/non-spec-compliant stuff which fragments DF packets, or removes
the DF flag.
But that stuff shouldn't really work (and often doesn't) anyway.

Maciej Żenczykowski, Kernel Networking Developer @ Google

On Wed, Mar 20, 2013 at 9:52 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> GRE TSO support doesn't increment the ID in the inner IP header.
>
> Remove the ID check in inet_gro_receive() so that GRO can properly
> aggregate GRE encapsulated TCP packets, instead of forcing
> a flush for every packet.
>
> Testing the IP ID is not really needed anyway for proper GRO operation.
>
> We can use more readable (and faster) code to access tot_len and
> frag_off fields.
>
> Tested on a bnx2x setup after commit a848ade408b6b
> (bnx2x: add CSUM and TSO support for encapsulation protocols)
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Dmitry Kravkov <dmitry@...adcom.com>
> Cc: Eilon Greenstein <eilong@...adcom.com>
> Cc: Pravin B Shelar <pshelar@...ira.com>
> Cc: H.K. Jerry Chu <hkchu@...gle.com>
> Cc: Maciej Żenczykowski <maze@...gle.com>
> ---
>  net/ipv4/af_inet.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 9e5882c..302a47e 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1355,7 +1355,6 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>         const struct iphdr *iph;
>         unsigned int hlen;
>         unsigned int off;
> -       unsigned int id;
>         int flush = 1;
>         int proto;
>
> @@ -1381,9 +1380,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>         if (unlikely(ip_fast_csum((u8 *)iph, 5)))
>                 goto out_unlock;
>
> -       id = ntohl(*(__be32 *)&iph->id);
> -       flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id ^ IP_DF));
> -       id >>= 16;
> +       flush = ntohs(iph->tot_len) ^ skb_gro_len(skb);
> +
> +       flush |= (__force u16)iph->frag_off ^ htons(IP_DF);
>
>         for (p = *head; p; p = p->next) {
>                 struct iphdr *iph2;
> @@ -1400,11 +1399,9 @@ static struct sk_buff **inet_gro_receive(struct sk_buff **head,
>                         continue;
>                 }
>
> -               /* All fields must match except length and checksum. */
>                 NAPI_GRO_CB(p)->flush |=
>                         (iph->ttl ^ iph2->ttl) |
> -                       (iph->tos ^ iph2->tos) |
> -                       ((u16)(ntohs(iph2->id) + NAPI_GRO_CB(p)->count) ^ id);
> +                       (iph->tos ^ iph2->tos);
>
>                 NAPI_GRO_CB(p)->flush |= flush;
>         }
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html