netdev - Re: [RFC PATCH v2 02/10] udp: implement GRO for plain UDP sockets.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20181022112451.GH3823@gauss3.secunet.de>
Date:   Mon, 22 Oct 2018 13:24:51 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Paolo Abeni <pabeni@...hat.com>
CC:     <netdev@...r.kernel.org>, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [RFC PATCH v2 02/10] udp: implement GRO for plain UDP sockets.

On Fri, Oct 19, 2018 at 04:25:12PM +0200, Paolo Abeni wrote:
>  
> +#define UDO_GRO_CNT_MAX 64

Maybe better UDP_GRO_CNT_MAX?

Btw. do we really need this explicit limit?
We should not get more than 64 packets during
one napi poll cycle.

> +static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
> +					       struct sk_buff *skb)
> +{
> +	struct udphdr *uh = udp_hdr(skb);
> +	struct sk_buff *pp = NULL;
> +	struct udphdr *uh2;
> +	struct sk_buff *p;
> +
> +	/* requires non zero csum, for simmetry with GSO */
> +	if (!uh->check) {
> +		NAPI_GRO_CB(skb)->flush = 1;
> +		return NULL;
> +	}

Why is the requirement of checksums different than in 
udp_gro_receive? It's not that I care much about UDP
packets without a checksum, but you would not need
to implement your own loop if the requirement could
be the same as in udp_gro_receive.

> +
> +	/* pull encapsulating udp header */
> +	skb_gro_pull(skb, sizeof(struct udphdr));
> +	skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> +
> +	list_for_each_entry(p, head, list) {
> +		if (!NAPI_GRO_CB(p)->same_flow)
> +			continue;
> +
> +		uh2 = udp_hdr(p);
> +
> +		/* Match ports only, as csum is always non zero */
> +		if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) {
> +			NAPI_GRO_CB(p)->same_flow = 0;
> +			continue;
> +		}
> +
> +		/* Terminate the flow on len mismatch or if it grow "too much".
> +		 * Under small packet flood GRO count could elsewhere grow a lot
> +		 * leading to execessive truesize values
> +		 */
> +		if (!skb_gro_receive(p, skb) &&
> +		    NAPI_GRO_CB(p)->count > UDO_GRO_CNT_MAX)

This allows to merge UDO_GRO_CNT_MAX + 1 packets.

> +			pp = p;
> +		else if (uh->len != uh2->len)
> +			pp = p;
> +
> +		return pp;
> +	}
> +
> +	/* mismatch, but we never need to flush */
> +	return NULL;
> +}