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

[Rafael Aquini <aquini@redhat.com>]

On Tue, Dec 17, 2013 at 01:27:49PM -0800, Davidlohr Bueso wrote:
> Ccing Manfred.
> 
> On Tue, 2013-12-17 at 17:03 -0200, Rafael Aquini wrote:
> > After the locking semantics for the SysV IPC API got improved, a couple of
> > IPC_RMID race windows were opened because we ended up dropping the
> > 'kern_ipc_perm.deleted' check performed way down in ipc_lock().
> > The spotted races got sorted out by re-introducing the old test within
> > the racy critical sections.
> > 
> > This patch introduces ipc_valid_object() to consolidate the way we cope with
> > IPC_RMID races by using the same abstraction across the API implementation.
> 
> This is certainly a good function to have. Some comments below.
> 
> > 
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > ---
> >  ipc/msg.c  |  7 ++++---
> >  ipc/sem.c  |  8 ++++----
> >  ipc/shm.c  | 16 ++++++++--------
> >  ipc/util.h | 13 +++++++++++++
> >  4 files changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/ipc/msg.c b/ipc/msg.c
> > index 558aa91..8983ea5 100644
> > --- a/ipc/msg.c
> > +++ b/ipc/msg.c
> > @@ -696,7 +696,7 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> >  			goto out_unlock0;
> >  
> >  		/* raced with RMID? */
> > -		if (msq->q_perm.deleted) {
> > +		if (!ipc_valid_object(&msq->q_perm)) {
> >  			err = -EIDRM;
> >  			goto out_unlock0;
> >  		}
> > @@ -731,7 +731,8 @@ long do_msgsnd(int msqid, long mtype, void __user *mtext,
> >  		ipc_lock_object(&msq->q_perm);
> >  
> >  		ipc_rcu_putref(msq, ipc_rcu_free);
> > -		if (msq->q_perm.deleted) {
> > +		/* raced with RMID? */
> > +		if (!ipc_valid_object(&msq->q_perm)) {
> >  			err = -EIDRM;
> >  			goto out_unlock0;
> >  		}
> > @@ -909,7 +910,7 @@ long do_msgrcv(int msqid, void __user *buf, size_t bufsz, long msgtyp, int msgfl
> >  		ipc_lock_object(&msq->q_perm);
> >  
> >  		/* raced with RMID? */
> > -		if (msq->q_perm.deleted) {
> > +		if (!ipc_valid_object(&msq->q_perm)) {
> >  			msg = ERR_PTR(-EIDRM);
> >  			goto out_unlock0;
> >  		}
> > diff --git a/ipc/sem.c b/ipc/sem.c
> > index db9d241..f4fad32 100644
> > --- a/ipc/sem.c
> > +++ b/ipc/sem.c
> > @@ -1282,7 +1282,7 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
> >  
> >  	sem_lock(sma, NULL, -1);
> >  
> > -	if (sma->sem_perm.deleted) {
> > +	if (!ipc_valid_object(&sma->sem_perm)) {
> >  		sem_unlock(sma, -1);
> >  		rcu_read_unlock();
> >  		return -EIDRM;
> > @@ -1342,7 +1342,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> >  		int i;
> >  
> >  		sem_lock(sma, NULL, -1);
> > -		if (sma->sem_perm.deleted) {
> > +		if (!ipc_valid_object(&sma->sem_perm)) {
> >  			err = -EIDRM;
> >  			goto out_unlock;
> >  		}
> > @@ -1435,7 +1435,7 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> >  		goto out_rcu_wakeup;
> >  
> >  	sem_lock(sma, NULL, -1);
> > -	if (sma->sem_perm.deleted) {
> > +	if (!ipc_valid_object(&sma->sem_perm)) {
> >  		err = -EIDRM;
> >  		goto out_unlock;
> >  	}
> > @@ -2068,7 +2068,7 @@ void exit_sem(struct task_struct *tsk)
> >  
> >  		sem_lock(sma, NULL, -1);
> >  		/* exit_sem raced with IPC_RMID, nothing to do */
> > -		if (sma->sem_perm.deleted) {
> > +		if (!ipc_valid_object(&sma->sem_perm)) {
> >  			sem_unlock(sma, -1);
> >  			rcu_read_unlock();
> >  			continue;
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index 7a51443..1bc68f1 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -975,6 +975,13 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> >  			goto out_unlock1;
> >  
> >  		ipc_lock_object(&shp->shm_perm);
> > +
> > +		/* check if shm_destroy() is tearing down shp */
> > +		if (!ipc_valid_object(&shp->shm_perm)) {
> > +			err = -EIDRM;
> > +			goto out_unlock0;
> > +		}
> > +
> >  		if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
> >  			kuid_t euid = current_euid();
> >  			if (!uid_eq(euid, shp->shm_perm.uid) &&
> > @@ -989,13 +996,6 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
> >  		}
> >  
> >  		shm_file = shp->shm_file;
> > -
> > -		/* check if shm_destroy() is tearing down shp */
> > -		if (shm_file == NULL) {
> > -			err = -EIDRM;
> > -			goto out_unlock0;
> > -		}
> 
> Ok, this seems safe, we can always rely on .deleted for validity since
> shm_destroy() ends up calling shm_rmid() which sets .deleted. So this
> change is really moving what we're checking against just a few
> instructions later.
>

Yep, I did change it cause it seems that there's no reason to delay the return
condition if we raced with shm_destroy(), anyways.
 

> > -
> >  		if (is_file_hugepages(shm_file))
> >  			goto out_unlock0;
> >  
> > @@ -1116,7 +1116,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
> >  	ipc_lock_object(&shp->shm_perm);
> >  
> >  	/* check if shm_destroy() is tearing down shp */
> > -	if (shp->shm_file == NULL) {
> > +	if (!ipc_valid_object(&shp->shm_perm)) {
> >  		ipc_unlock_object(&shp->shm_perm);
> >  		err = -EIDRM;
> >  		goto out_unlock;
> > diff --git a/ipc/util.h b/ipc/util.h
> > index 59d78aa..3a5f0d0 100644
> > --- a/ipc/util.h
> > +++ b/ipc/util.h
> > @@ -185,6 +185,19 @@ static inline void ipc_unlock(struct kern_ipc_perm *perm)
> >  	rcu_read_unlock();
> >  }
> >  
> > +/*
> > + * ipc_valid_object() - helper to sort out IPC_RMID races for codepaths
> > + * 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.
> > + */
> > +static inline bool ipc_valid_object(struct kern_ipc_perm *perm)
> > +{
> > +	assert_spin_locked(&perm->lock);
> 
> This is already guaranteed, we don't need it (not that it's much of an
> overhead in any case). IMO the comment above explicitly saying to call
> it with the ipc lock held is enough.
>

Good thought. I'll drop the assert and cover one extra case in sem.c where the
check can be done lockless (just spotted it).

Will submit a V2 soon with the change above! Thanks for the review.


> 
> > +	return perm->deleted == 0;
> 
> We should turn this into a bool (yes, that's another patch).
> 

Yep, I thought doing the scalar-2-bool conversion here and submit another patch
later, for a total boolean convertion, when (if) this one gets accepted.


Thanks for your thoughts!
-- Rafael

> > +}
> > +
> >  struct kern_ipc_perm *ipc_obtain_object_check(struct ipc_ids *ids, int id);
> >  int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
> >  			struct ipc_ops *ops, struct ipc_params *params);
> 
>