netdev - Re: [PATCHv3 1/2] net: adding memory barrier to the poll and receive callbacks

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: <20090701065724.GA2852@jolsa.lab.eng.brq.redhat.com>
Date:	Wed, 1 Jul 2009 08:57:24 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Davide Libenzi <davidel@...ilserver.org>
Cc:	netdev@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	fbl@...hat.com, nhorman@...hat.com, davem@...hat.com,
	htejun@...il.com, jarkao2@...il.com, oleg@...hat.com,
	eric.dumazet@...il.com
Subject: Re: [PATCHv3 1/2] net: adding memory barrier to the poll and
	receive callbacks

On Tue, Jun 30, 2009 at 12:13:40PM -0700, Davide Libenzi wrote:
> On Tue, 30 Jun 2009, Jiri Olsa wrote:
> 
> > Adding memory barrier after the poll_wait function, paired with
> > receive callbacks. Adding fuctions sock_poll_wait and sock_has_sleeper
> > to wrap the memory barrier.
> > 
> > Without the memory barrier, following race can happen.
> > The race fires, when following code paths meet, and the tp->rcv_nxt 
> > and __add_wait_queue updates stay in CPU caches.
> > 
> > 
> > CPU1                         CPU2
> > 
> > sys_select                   receive packet
> >   ...                        ...
> >   __add_wait_queue           update tp->rcv_nxt
> >   ...                        ...
> >   tp->rcv_nxt check          sock_def_readable
> >   ...                        {
> >   schedule                      ...
> >                                 if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
> >                                         wake_up_interruptible(sk->sk_sleep)
> >                                 ...
> >                              }
> > 
> > If there was no cache the code would work ok, since the wait_queue and
> > rcv_nxt are opposit to each other.
> > 
> > Meaning that once tp->rcv_nxt is updated by CPU2, the CPU1 either already
> > passed the tp->rcv_nxt check and sleeps, or will get the new value for
> > tp->rcv_nxt and will return with new data mask.
> > In both cases the process (CPU1) is being added to the wait queue, so the
> > waitqueue_active (CPU2) call cannot miss and will wake up CPU1.
> > 
> > The bad case is when the __add_wait_queue changes done by CPU1 stay in its
> > cache, and so does the tp->rcv_nxt update on CPU2 side.  The CPU1 will then
> > endup calling schedule and sleep forever if there are no more data on the
> > socket.
> 
> > +static inline int sk_has_sleeper(struct sock *sk)
> > +{
> > +	/*
> > +	 * We need to be sure we are in sync with the
> > +	 * add_wait_queue modifications to the wait queue.
> > +	 *
> > +	 * This memory barrier is paired in the sock_poll_wait.
> > +	 */
> > +	smp_mb();
> > +	return sk->sk_sleep && waitqueue_active(sk->sk_sleep);
> > +}
> 
> Jiri, since this is a pretty tricky condition, would you mind to have a 
> reduced version of the patch comment added to the source code?
> Patch comments are not really useful when you're trying to make sense of 
> some code ;)
> 

well, to be honest I thought it was already reduced :) however I have
no problem to make it shorter.. any suggestions? 

"This memory barrier protects the add_wait_queue modifications.
It is paired in the sock_poll_wait."

or do you want only the 

"This memory barrier is paired in the sock_poll_wait."


jirka
> 
> - Davide
> 
> 
--
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