netdev - Re: [PATCH v4 net-next 00/19] inet: frags: bring rhashtables to IP defrag

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:   Mon, 28 May 2018 09:09:17 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Alexander Aring <aring@...atatu.com>,
        Tariq Toukan <tariqt@...lanox.com>
Cc:     David Miller <davem@...emloft.net>, edumazet@...gle.com,
        netdev@...r.kernel.org, fw@...len.de, herbert@...dor.apana.org.au,
        tgraf@...g.ch, brouer@...hat.com, alex.aring@...il.com,
        stefan@....samsung.com, ktkhai@...tuozzo.com,
        eric.dumazet@...il.com, Moshe Shemesh <moshe@...lanox.com>,
        Eran Ben Elisha <eranbe@...lanox.com>
Subject: Re: [PATCH v4 net-next 00/19] inet: frags: bring rhashtables to IP
 defrag



On 05/28/2018 07:52 AM, Alexander Aring wrote:

> as somebody who had similar issues with this patch series I can tell you
> about what happened for the 6LoWPAN fragmentation.
> 
> The issue sounds similar, but there is too much missing information here
> to say something about if you have exactly the issue which we had.
> 
> Our problem:
> 
> The patch series uses memcmp() to compare hash keys, we had some padding
> bytes in our hash key and it occurs that we had sometimes random bytes
> in this structure when it's put on stack. We solved it by a struct
> foo_key bar = {}, which in case of gcc it _seems_ it makes a whole
> memset(bar, 0, ..) on the structure.
> 
> I asked on the netdev mailinglist how to deal with this problem in
> general, because = {} works in case of gcc, others compilers may have a
> different handling or even gcc will changes this behaviour in future.
> I got no reply so I did what it works for me. :-)
> 
> At least maybe a memcmp() on structures should never be used, it should
> be compared by field. I would recommend this way when the compiler is
> always clever enough to optimize it in some cases, but I am not so a
> compiler expert to say anything about that.
> 
> I checked the hash key structures for x86_64 and pahole, so far I didn't
> find any padding bytes there, but it might be different on
> architectures or ?compiler?.
> 
> Additional useful information to check if you running into the same problem
> would be:
> 
>  - Which architecture do you use?
> 
>  - Do you have similar problems with a veth setup?
> 
> You could also try this:
> 
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index b939b94e7e91..40ece9ab8b12 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -142,19 +142,19 @@ static void ip6_frag_expire(struct timer_list *t)
>  static struct frag_queue *
>  fq_find(struct net *net, __be32 id, const struct ipv6hdr *hdr, int iif)
>  {
> -       struct frag_v6_compare_key key = {
> -               .id = id,
> -               .saddr = hdr->saddr,
> -               .daddr = hdr->daddr,
> -               .user = IP6_DEFRAG_LOCAL_DELIVER,
> -               .iif = iif,
> -       };
> +       struct frag_v6_compare_key key = {};
>         struct inet_frag_queue *q;
>  
>         if (!(ipv6_addr_type(&hdr->daddr) & (IPV6_ADDR_MULTICAST |
>                                             IPV6_ADDR_LINKLOCAL)))
>                 key.iif = 0;
>  
> +       key.id = id;
> +       key.saddr = hdr->saddr;
> +       key.daddr = hdr->daddr;
> +       key.user = IP6_DEFRAG_LOCAL_DELIVER;
> +       key.iif = iif;
> +
>         q = inet_frag_find(&net->ipv6.frags, &key);
>         if (!q)
>                 return NULL;
> 
> - Alex
> 

Hi Alex.

This patch makes no sense, since struct frag_v6_compare_key has no hole.

Only 6LoWPAN had a problem really, because of its way of having unions (and holes).

Also note that your patch would break the case when we force key.iif to be zero.


Tariq, here are my test results : No drops for me.

# ./netperf -H 2607:f8b0:8099:e18:: -t UDP_STREAM
MIGRATED UDP STREAM TEST from ::0 (::) port 0 AF_INET6 to 2607:f8b0:8099:e18:: () port 0 AF_INET6
Socket  Message  Elapsed      Messages                
Size    Size     Time         Okay Errors   Throughput
bytes   bytes    secs            #      #   10^6bits/sec

212992   65507   10.00      202117      0    10592.00
212992           10.00           0              0.00

Somehow, you might send packets too fast and receiver has a problem with that ?
For particular needs, you might need to adjust :

/proc/sys/net/ipv6/ip6frag_time  (to 2 seconds instead of the default of 60)
/proc/sys/net/ipv6/ip6frag_low_thresh
/proc/sys/net/ipv6/ip6frag_high_thresh

Once your receiver has filled its capacity with frags, the default of 60 seconds to garbage collect
might be the reason you notice a problem.

Check :
grep FRAG6 /proc/net/sockstat6

On Google servers we multiply by 25 the limits for ipv6 frags memory usage :

/proc/sys/net/ipv6/ip6frag_high_thresh:104857600  (instead of 4MB)
/proc/sys/net/ipv6/ip6frag_low_thresh:78643200  (instead of 3 MB)

When using 64KB datagrams, note that the truesize of the datagram would be about 44 * 2 = 88 KB,
so after ~40 lost packets in the network, you no longer can accept ipv6 fragments, until garbage
collector evicted old datagrams.