Re: [PATCH 2/2] ipc: Fix ipc data structures inconsistency

[Davidlohr Bueso <dave@stgolabs.net>]

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