netdev - Re: [PATCH net] tcp/dccp: fix possible race __inet_lookup

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: <20191213175219.35353421@cakuba.netronome.com>
Date:   Fri, 13 Dec 2019 17:52:19 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Firo Yang <firo.yang@...e.com>
Subject: Re: [PATCH net] tcp/dccp: fix possible race
 __inet_lookup_established()

On Thu, 12 Dec 2019 19:47:37 +0100, Michal Kubecek wrote:
> > > >  /*
> > > >   * Sockets can be hashed in established or listening table
> > > > - */
> > > > + * We must use different 'nulls' end-of-chain value for listening
> > > > + * hash table, or we might find a socket that was closed and
> > > > + * reallocated/inserted into established hash table
> > > > +  */  
> > >
> > > Just a nitpick: I don't think this comment is still valid because
> > > listening sockets now have RCU protection so that listening socket
> > > cannot be freed and reallocated without RCU grace period. (But we still
> > > need disjoint ranges to handle the reallocation in the opposite
> > > direction.)  
> > 
> > Hi Michal
> > 
> > I am not a native English speaker, but I was trying to say :
> > 
> > A lookup in established sockets might go through a socket that
> > was in this bucket but has been closed, reallocated and became a listener.  
> 
> I'm not a native speaker either. What I wanted to point out was that the
> comment rather seems to talk about the other direction, i.e. looking up
> in listener hashtable and ending up in established due to reallocation.
> That was an issue back when the offset was introduced (and when there
> was a check of end marker value in __inet_lookup_listener()) but it
> cannot happen any more.
> 
> > Maybe the comment needs to be refined, but I am not sure how, considering
> > that most people reading it will not understand it anyway, given the
> > complexity of the nulls stuff.  
> 
> I guess it can stay as it is. After all, one needs to see the lookup
> code to understand the purpose of the offset and then it's easier to see
> in the code what is it for.

I need to remove a whitespace here:

WARNING: Block comments should align the * on each line
#98: FILE: include/net/inet_hashtables.h:111:
+ * reallocated/inserted into established hash table
+  */

So last chance to adjust the comment as well - perhaps we can
s/into/from/ on the last line? 

Or say "or we might find an established socket that was closed and
reallocated/inserted into different hash table" ?

IMHO confusing comment is worse than no comment at all :S