Re: [PATCH v2] ipc: introduce ipc_valid_object() helper to sort out IPC_RMID races

[Manfred Spraul <manfred@colorfullife.com>]

Hi Rafael,

On 12/18/2013 06:34 PM, Rafael Aquini wrote:
> Folks,
>
> Before I re-submit the v3 with the commentary changes requested, I'm pasting
> here what I'm planning to amend to v2 patch:
> ---
> diff --git a/ipc/sem.c b/ipc/sem.c
> index ed0057a..23379b6 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1846,6 +1846,14 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __u
>   
>          error = -EIDRM;
>          locknum = sem_lock(sma, sops, nsops);
> +       /*
> +        * We eventually might perform the following check in a lockless
> +        * fashion here, considering ipc_valid_object() locking constraints.
> +        * If nsops == 1 and there's no contention for sem_perm.lock, then
> +        * only a per-semaphore lock is held and it's OK to go on the check
> +        * below. More details on the fine grained locking scheme entangled
> +        * here, and why it's RMID race safe on comments at sem_lock()
> +        */
>          if (!ipc_valid_object(&sma->sem_perm))
>                  goto out_unlock_free;
>          /*
> diff --git a/ipc/util.h b/ipc/util.h
> index 071ed58..d05b708 100644
> --- a/ipc/util.h
> +++ b/ipc/util.h
> @@ -190,7 +190,8 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
>    * where the respective ipc_ids.rwsem is not being held down.
>    * Checks whether the ipc object is still around or if it's gone already, as
>    * ipc_rmid() may have already freed the ID while the ipc lock was spinning.
> - * Needs to be called with kern_ipc_perm.lock held.
> + * Needs to be called with kern_ipc_perm.lock held -- exception made for one
> + * checkpoint case at sys_semtimedop() as noted in code commentary.
>    */
>   static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
>   {
> ---
>
> Do we need to change somthing else?
> Looking forward your thoughts!
Acked-by: Manfred Spraul <manfred@colorfullife.com>

--
     Manfred