Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751530AbdLBGDe (ORCPT <rfc822;w@1wt.eu>);
        Sat, 2 Dec 2017 01:03:34 -0500
Received: from mail-wr0-f196.google.com ([209.85.128.196]:40514 "EHLO
        mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751218AbdLBGDd (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 2 Dec 2017 01:03:33 -0500
X-Google-Smtp-Source: AGs4zMb3dx90sOQds2XOEJjxYjycTKpCvDknosAIHobRfilp1+oIa9qmsR4ztwezICKyeoLTUjaUYQ==
Subject: Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency
To: Davidlohr Bueso <dave@stgolabs.net>,
        Philippe Mikoyan <philippe.mikoyan@skat.systems>
Cc: akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, edgar.kaziakhmedov@virtuozzo.com
References: <20171130061224.25466-1-philippe.mikoyan@skat.systems>
 <20171130061224.25466-3-philippe.mikoyan@skat.systems>
 <20171201172007.q2rqmo4jqaxb63tk@linux-n805>
From: Manfred Spraul <manfred@colorfullife.com>
Message-ID: <bcf68a7f-d860-2c7e-69ed-341eb81c69e6@colorfullife.com>
Date: Sat, 2 Dec 2017 07:03:30 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171201172007.q2rqmo4jqaxb63tk@linux-n805>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1933
Lines: 56

Hi,

On 12/01/2017 06:20 PM, Davidlohr Bueso wrote:
> On Thu, 30 Nov 2017, Philippe Mikoyan wrote:
>
>> As described in the title, this patch fixes <ipc>id_ds inconsistency
>> when <ipc>ctl_stat runs concurrently with some ds-changing function,
>> e.g. shmat, msgsnd or whatever.
>>
>> For instance, if shmctl(IPC_STAT) is running concurrently with shmat,
>> following data structure can be returned:
>> {... shm_lpid = 0, shm_nattch = 1, ...}
>
The patch appears to be good. I'll try to perform some tests, but I'm 
not sure when I will be able to.
Especially: I don't know the shm code good enough to immediately check 
the change you make to nattach.

And, perhaps as a side information:
There appears to be a use-after-free in shm, I now got a 2nd mail from 
syzbot:
http://lkml.iu.edu/hypermail/linux/kernel/1702.3/02480.html


> Hmm yeah that's pretty fishy, also shm_atime = 0, no?
>
> So I think this patch is fine as we can obviously race at a user level.
> This is another justification for converting the ipc lock to rwlock;
> performance wise they are the pretty much the same (being queued)...
> but that's irrelevant to this patch. I like that you manage to do
> security and such checks still only under rcu, like all ipc calls
> work; *_stat() is no longer special.
>
I don't like rwlock, they add complexity without reducing the cache line 
pressure.

What I would like to try is to create a mutex_lock_rcu() function, and 
then convert everything to a mutex.

As pseudocode::
     rcu_lock();
     idr_lookup();
     mutex_trylock();
     if (failed) {
         getref();
         rcu_unlock();
         mutex_lock();
         putref();
     } else {
         rcu_unlock();
     }

Obviously, the getref then within the mutex framework, i.e. only if 
mutex_lock() really sleeps.
If the code in ipc gets significantly simpler, then perhaps convert it 
to an rw mutex.