Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752071AbdLARYJ (ORCPT <rfc822;w@1wt.eu>);
        Fri, 1 Dec 2017 12:24:09 -0500
Received: from mx2.suse.de ([195.135.220.15]:57053 "EHLO mx2.suse.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751208AbdLARYH (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 1 Dec 2017 12:24:07 -0500
Date: Fri, 1 Dec 2017 09:20:07 -0800
From: Davidlohr Bueso <dave@stgolabs.net>
To: Philippe Mikoyan <philippe.mikoyan@skat.systems>
Cc: akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
        manfred@colorfullife.com, linux-kernel@vger.kernel.org,
        edgar.kaziakhmedov@virtuozzo.com
Subject: Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency
Message-ID: <20171201172007.q2rqmo4jqaxb63tk@linux-n805>
References: <20171130061224.25466-1-philippe.mikoyan@skat.systems>
 <20171130061224.25466-3-philippe.mikoyan@skat.systems>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20171130061224.25466-3-philippe.mikoyan@skat.systems>
User-Agent: NeoMutt/20170421 (1.8.2)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1696
Lines: 45

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, ...}

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.

With a nit below:

Reviewed-by: Davidlohr Bueso <dbueso@suse.de>

>diff --git a/ipc/util.c b/ipc/util.c
>index 78755873cc5b..8d3c3946c825 100644
>--- a/ipc/util.c
>+++ b/ipc/util.c
>@@ -22,9 +22,12 @@
>  *	    tree.
>  *	    - perform initial checks (capabilities, auditing and permission,
>  *	      etc).
>- *	    - perform read-only operations, such as STAT, INFO commands.
>+ *	    - perform read-only operations, such as INFO command, that
>+ *	      do not demand atomicity
>  *	      acquire the ipc lock (kern_ipc_perm.lock) through
>  *	      ipc_lock_object()
>+ *		- perform read-only operatoins that demand atomicity,
                                          ^^ typo
>+ *		  such as STAT command.
>  *		- perform data updates, such as SET, RMID commands and
>  *		  mechanism-specific operations (semop/semtimedop,
>  *		  msgsnd/msgrcv, shmat/shmdt).

Thanks,
Davidlohr