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

[Davidlohr Bueso]

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@xxxxxxx>

> 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