netdev - Re: [PATCH net] tcp: ensure proper barriers in lockless contexts

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: <1447557701.22599.57.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Sat, 14 Nov 2015 19:21:41 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net] tcp: ensure proper barriers in lockless contexts

On Sun, 2015-11-15 at 09:20 +0800, Herbert Xu wrote:
> Eric Dumazet <eric.dumazet@...il.com> wrote:
> > From: Eric Dumazet <edumazet@...gle.com>
> > 
> > Some functions access TCP sockets without holding a lock and
> > might output non consistent data, depending on compiler and or
> > architecture.
> > 
> > tcp_diag_get_info(), tcp_get_info(), tcp_poll(), get_tcp4_sock() ...
> 
> For the information gathering ones such as tcp_diag_get_info I'm
> wondering whether we really need these memory barriers.  After all,
> if it's truly lockless then surely the TCP socket state can change
> again after you load the state the first time, in which case the
> barrier becomes completely meaningless.

Not a big deal, no crash or kernel instability, just making the
information a bit more consistent.

They are not truly needed, but the patch avoids some discrepancies when
an observer gets the data, for a minimum cost ( compiler barrier() in
most cases)

Like : 

       state = sk_state_load(sp);
       if (state == TCP_LISTEN)
               rx_queue = sp->sk_ack_backlog;
       else
               /* Because we don't lock the socket,
                * we might find a transient negative value.
                */
               rx_queue = max_t(int, tp->rcv_nxt - tp->copied_seq, 0);

or :

       if (sk_state_load(sk) == TCP_LISTEN) {
                r->idiag_rqueue = sk->sk_ack_backlog;
                r->idiag_wqueue = sk->sk_max_ack_backlog;


This patch makes sure sk_ack_backlog is written before sk_state is set
to TCP_LISTEN, otherwise one could see a '0' listener backlog.

We had a spurious kernel log for a similar issue that was solved in
commit f985c65c908f6b26c30019a83dc5ea295f5fcf62
("tcp: avoid spurious SYN flood detection at listen() time")

Sure, we can live with a spurious log, but experience showed that taking
care of this early could avoid lot of hassle, say in 12 months when
people start using linux-4.4 

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html