netdev - Re: [PATCH RFC 3/3] udp: Support UDP fraglist GRO/GSO.

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: <7e001c0f99f688da4be762528ddc9287f49797fc.camel@redhat.com>
Date:   Tue, 08 Jan 2019 16:00:01 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>,
        netdev@...r.kernel.org
Cc:     Willem de Bruijn <willemb@...gle.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH RFC 3/3] udp: Support UDP fraglist GRO/GSO.

On Fri, 2018-12-21 at 08:53 +0100, Steffen Klassert wrote:
> @@ -403,10 +428,17 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  
>  	rcu_read_lock();
>  	sk = (*lookup)(skb, uh->source, uh->dest);
> -	if (!sk)
> -		goto out_unlock;
> +	if (!sk) {
> +		NAPI_GRO_CB(skb)->is_flist = 1;
> +		pp = call_gro_receive(udp_gro_receive_segment, head, skb);
> +		rcu_read_unlock();
> +		return pp;
> +	}
> +
> +	if (!udp_sk(sk)->gro_receive) {
> +		if (!udp_sk(sk)->gro_enabled)
> +			NAPI_GRO_CB(skb)->is_flist = 1;
>  
> -	if (udp_sk(sk)->gro_enabled) {
>  		pp = call_gro_receive(udp_gro_receive_segment, head, skb);
>  		rcu_read_unlock();
>  		return pp;

I think we could still avoid the lookup when no vxlan/GRO sockets are
present moving the lookup into udp{4,6}_gro_receive. Very roughly
something alike:

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index f79f1b5b2f9e..b0c0983eac6b 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -420,20 +420,16 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 INDIRECT_CALLABLE_DECLARE(struct sock *udp6_lib_lookup_skb(struct sk_buff *skb,
                                                   __be16 sport, __be16 dport));
 struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
-                               struct udphdr *uh, udp_lookup_t lookup)
+                               struct udphdr *uh, struct sock *sk)
 {
        struct sk_buff *pp = NULL;
        struct sk_buff *p;
        struct udphdr *uh2;
        unsigned int off = skb_gro_offset(skb);
        int flush = 1;
-       struct sock *sk;
 
-       rcu_read_lock();
-       sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
-                               udp4_lib_lookup_skb, skb, uh->source, uh->dest);
-       if (!sk) {
-               NAPI_GRO_CB(skb)->is_flist = 1;
+       if (!sk || !udp_sk(sk)->gro_receive) {
+               NAPI_GRO_CB(skb)->is_flist = sk ? !udp_sk(sk)->gro_enabled: 1;
                pp = call_gro_receive(udp_gro_receive_segment, head, skb);
                rcu_read_unlock();
                return pp;
@@ -506,7 +502,12 @@ struct sk_buff *udp4_gro_receive(struct list_head *head, struct sk_buff *skb)
                                             inet_gro_compute_pseudo);
 skip:
        NAPI_GRO_CB(skb)->is_ipv6 = 0;
-       return udp_gro_receive(head, skb, uh, udp4_lib_lookup_skb);
+       rcu_read_lock();
+       sk = static_branch_unlikely(&udp_encap_needed_key) ?
+                       udp4_lib_lookup_skb(skb, uh->source, uh->dest) : NULL;
+       pp = udp_gro_receive(head, skb, uh, sk);
+       rcu_read_unlock();
+       return pp;
 
 flush:
        NAPI_GRO_CB(skb)->flush = 1;
---

Regardless of the above, I think we should drop the later check for
gro_receive:

--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -450,8 +450,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
        if (NAPI_GRO_CB(skb)->encap_mark ||
            (skb->ip_summed != CHECKSUM_PARTIAL &&
             NAPI_GRO_CB(skb)->csum_cnt == 0 &&
-            !NAPI_GRO_CB(skb)->csum_valid) ||
-           !udp_sk(sk)->gro_receive)
+            !NAPI_GRO_CB(skb)->csum_valid))
                goto out_unlock;
 
        /* mark that this skb passed once through the tunnel gro layer */
---

Finally this will cause GRO/GSO for local UDP packets delivery to non
GSO_SEGMENT sockets. That could be possibly a win or a regression: we
save on netfilter/IP stack traversal, but we add additional work, some
performances figures would probably help.

Cheers,

Paolo