From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752555AbbKOSdf (ORCPT ); Sun, 15 Nov 2015 13:33:35 -0500 Received: from tiger.mobileactivedefense.com ([217.174.251.109]:39512 "EHLO tiger.mobileactivedefense.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751319AbbKOSda (ORCPT ); Sun, 15 Nov 2015 13:33:30 -0500 From: Rainer Weikusat To: Jason Baron Cc: Rainer Weikusat , Dmitry Vyukov , syzkaller , Michal Kubecek , Al Viro , "linux-fsdevel\@vger.kernel.org" , LKML , David Miller , Hannes Frederic Sowa , David Howells , Paul Moore , salyzyn@android.com, sds@tycho.nsa.gov, ying.xue@windriver.com, netdev , Kostya Serebryany , Alexander Potapenko , Andrey Konovalov , Sasha Levin , Julien Tinnes , Kees Cook , Mathias Krause Subject: Re: [PATCH] unix: avoid use-after-free in ep_remove_wait_queue In-Reply-To: <5646617C.9080506@akamai.com> (Jason Baron's message of "Fri, 13 Nov 2015 17:17:32 -0500") References: <20151012120249.GB16370@unicorn.suse.cz> <1444652071.27760.156.camel@edumazet-glaptop2.roam.corp.google.com> <563CC002.5050307@akamai.com> <87ziyrcg67.fsf@doppelsaurus.mobileactivedefense.com> <87fv0fnslr.fsf_-_@doppelsaurus.mobileactivedefense.com> <877flp34fl.fsf@doppelsaurus.mobileactivedefense.com> <87a8qhspfm.fsf@doppelsaurus.mobileactivedefense.com> <5646617C.9080506@akamai.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) Date: Sun, 15 Nov 2015 18:32:55 +0000 Message-ID: <87vb93dsfc.fsf@doppelsaurus.mobileactivedefense.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (tiger.mobileactivedefense.com [217.174.251.109]); Sun, 15 Nov 2015 18:33:06 +0000 (GMT) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Jason Baron writes: > On 11/13/2015 01:51 PM, Rainer Weikusat wrote: > > [...] > >> >> - if (unix_peer(other) != sk && unix_recvq_full(other)) { >> - if (!timeo) { >> - err = -EAGAIN; >> - goto out_unlock; >> - } >> + if (unix_peer(sk) == other && !unix_dgram_peer_recv_ready(sk, other)) { > > Remind me why the 'unix_peer(sk) == other' is added here? If the remote > is not connected we still want to make sure that we don't overflow the > the remote rcv queue, right? Good point. The check is actually wrong there as the original code would also check the limit in case of an unconnected send to a socket found via address lookup. It belongs into the 2nd if (were I originally put it). > > In terms of this added 'double' lock for both sk and other, where > previously we just held the 'other' lock. I think we could continue to > just hold the 'other' lock unless the remote queue is full, so something > like: > > if (unix_peer(other) != sk && unix_recvq_full(other)) { > bool need_wakeup = false; > > ....skipping the blocking case... > > err = -EAGAIN; > if (!other_connected) > goto out_unlock; > unix_state_unlock(other); > unix_state_lock(sk); That was my original idea. The problem with this is that the code starting after the _lock and running until the main code path unlock has to be executed in one go with the other lock held as the results of the tests above this one may become invalid as soon as the other lock is released. This means instead of continuing execution with the send code proper after the block in case other became receive-ready between the first and the second test (possible because _dgram_recvmsg does not take the unix state lock), the whole procedure starting with acquiring the other lock would need to be restarted. Given sufficiently unfavorable circumstances, this could even turn into an endless loop which couldn't be interrupted. (unless code for this was added). [...] > we currently wake the entire queue on every remote read even when we > have room in the rcv buffer. So this patch will cut down on ctxt > switching rate dramatically from what we currently have. In my opinion, this could be improved by making the throttling mechanism work like a flip flop: If the queue lenght hits the limit after a _sendmsg, set a "no more applicants" flag blocking future sends until cleared (checking the flag would replace the present check). After the receive queue ran empty (or almost empty), _dgram_sendmsg would clear the flag and do a wakeup. But this should be an independent patch (if implemented).