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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <b69077c93ec047845d4b22a57fa6f89b63c0639c.camel@redhat.com>
Date:   Mon, 22 Oct 2018 15:41:57 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Steffen Klassert <steffen.klassert@...unet.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 Mon, 2018-10-22 at 13:24 +0200, Steffen Klassert wrote:
> 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?

Oops, typo. Yes, sure, will address in the next iteration.

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

With HZ >= 1000, gro_flush happens at most once per jiffies: we can
have much more than 64 packets per segment, with appropriate pkt len.

> 
> > +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.

uhm.... 
AFAIU, we need to generated aggregated packets that UDP GSO is able to
process/segment. I was unable to get a nocsum packet segment (possibly
PEBKAC) so I enforced that condition on the rx path.

@Willem: did I see ghost here? is UDP_SEGMENT fine with no checksum
segment?

> > +
> > +	/* 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.

Thanks, will address in the next iteration.

Cheers,

Paolo