All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: Mark Salyzyn <salyzyn@android.com>,
	Hannes Frederic Sowa <hannes@redhat.com>
Cc: linux-kernel@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Al Viro <viro@zeniv.linux.org.uk>,
	David Howells <dhowells@redhat.com>,
	Ying Xue <ying.xue@windriver.com>, Christoph Hellwig <hch@lst.de>,
	netdev@vger.kernel.org
Subject: Re: net/unix: sk_socket can disappear when state is unlocked
Date: Fri, 22 May 2015 20:16:02 +0200	[thread overview]
Message-ID: <1432318562.3430833.275929105.372EB77C@webmail.messagingengine.com> (raw)
In-Reply-To: <555F583B.1010309@android.com>

On Fri, May 22, 2015, at 18:24, Mark Salyzyn wrote:
> On 05/22/2015 08:35 AM, Hannes Frederic Sowa wrote:
> > I still wonder if we need to actually recheck the condition and not
> > simply break out of unix_stream_data_wait:
> >
> > We return to the unix_stream_recvmsg loop and recheck the
> > sk_receive_queue. At this point sk_receive_queue is not really protected
> > with unix_state_lock against concurrent modification with unix_release,
> > as such we could end up concurrently dequeueing packets if socket is
> > DEAD.
> sock destroy(sic) is called before sock_orphan which sets SOCK_DEAD, so 
> the receive queue has already been drained.

I am still afraid that there is a race:

When we break out in unix_stream_data_wait we most of the time hit the
continue statement in unix_stream_recvmsg. Albeit we acquired state lock
again, we could end up in a situation where the sk_receive_queue is not
completely drained. We would miss the recheck of the sk_shutdown mask,
because it is possible we dequeue a non-null skb from the receive queue.
This is because unix_release_sock acquires state lock, sets appropriate
flags but the draining of the receive queue does happen without locks,
state lock is unlocked before that. So theoretically both, release_sock
and recvmsg could dequeue skbs concurrently in nondeterministic
behavior.

The fix would be to recheck SOCK_DEAD or even better, sk_shutdown right
after we reacquired state_lock and break out of the loop altogether,
maybe with -ECONNRESET.

Thanks,
Hannes

  reply	other threads:[~2015-05-22 18:16 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-21 16:25 net/unix: sk_socket can disappear when state is unlocked Mark Salyzyn
2015-05-22  9:50 ` Hannes Frederic Sowa
2015-05-22 14:51   ` Mark Salyzyn
2015-05-22 15:35     ` Hannes Frederic Sowa
2015-05-22 16:24       ` Mark Salyzyn
2015-05-22 18:16         ` Hannes Frederic Sowa [this message]
2015-05-22 19:59           ` Mark Salyzyn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1432318562.3430833.275929105.372EB77C@webmail.messagingengine.com \
    --to=hannes@stressinduktion.org \
    --cc=davem@davemloft.net \
    --cc=dhowells@redhat.com \
    --cc=hannes@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=salyzyn@android.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=ying.xue@windriver.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.