[PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID

[PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID

[Manfred Spraul]

After acquiring the semlock spinlock, the operations must test that the
array is still valid.

- semctl() and exit_sem() would walk stale linked lists (ugly, but should
  be ok: all lists are empty)

- semtimedop() would sleep forever - and if woken up due to a signal -
  access memory after free.

The patch standardizes the tests for .deleted, so that all tests in one
function leave the function with the same approach.

Right now, it's a mixture of "goto cleanup", some cleanup and then
"goto further_cleanup" and all cleanup+"return -EIDRM" - that makes the
review much harder.

Davidlohr: Could you please review the patch?
I did some stress test, but probably I didn't hit exactly the modified
lines.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 ipc/sem.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 19c8b98..a2fa795 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1229,6 +1229,12 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
 
 	sem_lock(sma, NULL, -1);
 
+	if (sma->sem_perm.deleted) {
+		sem_unlock(sma, -1);
+		rcu_read_unlock();
+		return -EIDRM;
+	}
+
 	curr = &sma->sem_base[semnum];
 
 	ipc_assert_locked_object(&sma->sem_perm);
@@ -1285,10 +1291,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 		sem_lock(sma, NULL, -1);
 		if(nsems > SEMMSL_FAST) {
 			if (!ipc_rcu_getref(sma)) {
-				sem_unlock(sma, -1);
-				rcu_read_unlock();
 				err = -EIDRM;
-				goto out_free;
+				goto out_unlock;
 			}
 			sem_unlock(sma, -1);
 			rcu_read_unlock();
@@ -1301,10 +1305,13 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 			rcu_read_lock();
 			sem_lock_and_putref(sma);
 			if (sma->sem_perm.deleted) {
-				sem_unlock(sma, -1);
-				rcu_read_unlock();
 				err = -EIDRM;
-				goto out_free;
+				goto out_unlock;
+			}
+		} else {
+			if (sma->sem_perm.deleted) {
+				err = -EIDRM;
+				goto out_unlock;
 			}
 		}
 		for (i = 0; i < sma->sem_nsems; i++)
@@ -1322,8 +1329,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 		struct sem_undo *un;
 
 		if (!ipc_rcu_getref(sma)) {
-			rcu_read_unlock();
-			return -EIDRM;
+			err = -EIDRM;
+			goto out_rcu_wakeup;
 		}
 		rcu_read_unlock();
 
@@ -1351,10 +1358,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 		rcu_read_lock();
 		sem_lock_and_putref(sma);
 		if (sma->sem_perm.deleted) {
-			sem_unlock(sma, -1);
-			rcu_read_unlock();
 			err = -EIDRM;
-			goto out_free;
+			goto out_unlock;
 		}
 
 		for (i = 0; i < nsems; i++)
@@ -1378,6 +1383,10 @@ 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) {
+		err = -EIDRM;
+		goto out_unlock;
+	}
 	curr = &sma->sem_base[semnum];
 
 	switch (cmd) {
@@ -1783,6 +1792,10 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 	if (error)
 		goto out_rcu_wakeup;
 
+	error = -EIDRM;
+	locknum = sem_lock(sma, sops, nsops);
+	if (sma->sem_perm.deleted)
+		goto out_unlock_free;
 	/*
 	 * semid identifiers are not unique - find_alloc_undo may have
 	 * allocated an undo structure, it was invalidated by an RMID
@@ -1790,8 +1803,6 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
 	 * This case can be detected checking un->semid. The existence of
 	 * "un" itself is guaranteed by rcu.
 	 */
-	error = -EIDRM;
-	locknum = sem_lock(sma, sops, nsops);
 	if (un && un->semid == -1)
 		goto out_unlock_free;
 
@@ -1999,6 +2010,12 @@ 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) {
+			sem_unlock(sma, -1);
+			rcu_read_unlock();
+			continue;
+		}
 		un = __lookup_undo(ulp, semid);
 		if (un == NULL) {
 			/* exit_sem raced with IPC_RMID+semget() that created
-- 
1.8.3.1

Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID

[Davidlohr Bueso]

Hi Manfred,

On Mon, 2013-09-30 at 11:13 +0200, Manfred Spraul wrote:
> After acquiring the semlock spinlock, the operations must test that the
> array is still valid.
> 
> - semctl() and exit_sem() would walk stale linked lists (ugly, but should
>   be ok: all lists are empty)
> 
> - semtimedop() would sleep forever - and if woken up due to a signal -
>   access memory after free.

Yep, that was next on my list - I had a patch for semtimedop() but was
waiting to rebase it on top of your previous changes. Anyway thanks for
sending this.

> 
> The patch standardizes the tests for .deleted, so that all tests in one
> function leave the function with the same approach.
> 
> Right now, it's a mixture of "goto cleanup", some cleanup and then
> "goto further_cleanup" and all cleanup+"return -EIDRM" - that makes the
> review much harder.
> 
> Davidlohr: Could you please review the patch?
> I did some stress test, but probably I didn't hit exactly the modified
> lines.

This shouldn't affect performance, if that's what you mean. One more
read in the critical region won't make any difference. The patch looks
good, just one doubt below.


> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
>  ipc/sem.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 19c8b98..a2fa795 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1229,6 +1229,12 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
>  
>  	sem_lock(sma, NULL, -1);
>  
> +	if (sma->sem_perm.deleted) {
> +		sem_unlock(sma, -1);
> +		rcu_read_unlock();
> +		return -EIDRM;
> +	}
> +
>  	curr = &sma->sem_base[semnum];
>  
>  	ipc_assert_locked_object(&sma->sem_perm);
> @@ -1285,10 +1291,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>  		sem_lock(sma, NULL, -1);
>  		if(nsems > SEMMSL_FAST) {
>  			if (!ipc_rcu_getref(sma)) {
> -				sem_unlock(sma, -1);
> -				rcu_read_unlock();
>  				err = -EIDRM;
> -				goto out_free;
> +				goto out_unlock;
>  			}
>  			sem_unlock(sma, -1);
>  			rcu_read_unlock();
> @@ -1301,10 +1305,13 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>  			rcu_read_lock();
>  			sem_lock_and_putref(sma);
>  			if (sma->sem_perm.deleted) {
> -				sem_unlock(sma, -1);
> -				rcu_read_unlock();
>  				err = -EIDRM;
> -				goto out_free;
> +				goto out_unlock;
> +			}
> +		} else {
> +			if (sma->sem_perm.deleted) {
> +				err = -EIDRM;
> +				goto out_unlock;
>  			}

I'm a bit lost here. Why should we only check the existence of the sem
if nsems <= SEMMSL_FAST? Shouldn't the same should apply either way? 

Thanks,
Davidlohr

Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID

[Manfred Spraul]

Hi Davidlohr,

On 09/30/2013 07:54 PM, Davidlohr Bueso wrote:
> Hi Manfred,
>
> On Mon, 2013-09-30 at 11:13 +0200, Manfred Spraul wrote:
>> After acquiring the semlock spinlock, the operations must test that the
>> array is still valid.
>>
>> - semctl() and exit_sem() would walk stale linked lists (ugly, but should
>>    be ok: all lists are empty)
>>
>> - semtimedop() would sleep forever - and if woken up due to a signal -
>>    access memory after free.
> Yep, that was next on my list - I had a patch for semtimedop() but was
> waiting to rebase it on top of your previous changes. Anyway thanks for
> sending this.
>
>> The patch standardizes the tests for .deleted, so that all tests in one
>> function leave the function with the same approach.
>>
>> Right now, it's a mixture of "goto cleanup", some cleanup and then
>> "goto further_cleanup" and all cleanup+"return -EIDRM" - that makes the
>> review much harder.
>>
>> Davidlohr: Could you please review the patch?
>> I did some stress test, but probably I didn't hit exactly the modified
>> lines.
> This shouldn't affect performance, if that's what you mean.
All goto's must go to the correct target, free everything, unlock 
everything, do not unlock twice, ...

>   One more
> read in the critical region won't make any difference. The patch looks
> good, just one doubt below.
>
>
>> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
>> ---
>>   ipc/sem.c | 43 ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index 19c8b98..a2fa795 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -1229,6 +1229,12 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
>>   
>>   	sem_lock(sma, NULL, -1);
>>   
>> +	if (sma->sem_perm.deleted) {
>> +		sem_unlock(sma, -1);
>> +		rcu_read_unlock();
>> +		return -EIDRM;
>> +	}
>> +
>>   	curr = &sma->sem_base[semnum];
>>   
>>   	ipc_assert_locked_object(&sma->sem_perm);
>> @@ -1285,10 +1291,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>>   		sem_lock(sma, NULL, -1);
>>   		if(nsems > SEMMSL_FAST) {
>>   			if (!ipc_rcu_getref(sma)) {
>> -				sem_unlock(sma, -1);
>> -				rcu_read_unlock();
>>   				err = -EIDRM;
>> -				goto out_free;
>> +				goto out_unlock;
>>   			}
>>   			sem_unlock(sma, -1);
>>   			rcu_read_unlock();
>> @@ -1301,10 +1305,13 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>>   			rcu_read_lock();
>>   			sem_lock_and_putref(sma);
>>   			if (sma->sem_perm.deleted) {
>> -				sem_unlock(sma, -1);
>> -				rcu_read_unlock();
>>   				err = -EIDRM;
^^^^^^^^^^^^^^^^^^ check if nsems > SEMMSL_FAST
>> -				goto out_free;
>> +				goto out_unlock;
>> +			}
>> +		} else {
>> +			if (sma->sem_perm.deleted) {
>> +				err = -EIDRM;
>> +				goto out_unlock;
>>   			}
> I'm a bit lost here. Why should we only check the existence of the sem
> if nsems <= SEMMSL_FAST? Shouldn't the same should apply either way?
It is checked in both branches:
- the check for "nsems > SEMMSL_FAST" was always there, due to the 
kmalloc, the lock is dropped.

--
     Manfred

Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID

[Davidlohr Bueso]

On Tue, 2013-10-01 at 06:22 +0200, Manfred Spraul wrote:
> Hi Davidlohr,
> 
> On 09/30/2013 07:54 PM, Davidlohr Bueso wrote:
> > Hi Manfred,
> >
> > On Mon, 2013-09-30 at 11:13 +0200, Manfred Spraul wrote:
> >> After acquiring the semlock spinlock, the operations must test that the
> >> array is still valid.
> >>
> >> - semctl() and exit_sem() would walk stale linked lists (ugly, but should
> >>    be ok: all lists are empty)
> >>
> >> - semtimedop() would sleep forever - and if woken up due to a signal -
> >>    access memory after free.
> > Yep, that was next on my list - I had a patch for semtimedop() but was
> > waiting to rebase it on top of your previous changes. Anyway thanks for
> > sending this.
> >
> >> The patch standardizes the tests for .deleted, so that all tests in one
> >> function leave the function with the same approach.
> >>
> >> Right now, it's a mixture of "goto cleanup", some cleanup and then
> >> "goto further_cleanup" and all cleanup+"return -EIDRM" - that makes the
> >> review much harder.
> >>
> >> Davidlohr: Could you please review the patch?
> >> I did some stress test, but probably I didn't hit exactly the modified
> >> lines.
> > This shouldn't affect performance, if that's what you mean.
> All goto's must go to the correct target, free everything, unlock 
> everything, do not unlock twice, ...
> 
> >   One more
> > read in the critical region won't make any difference. The patch looks
> > good, just one doubt below.
> >
> >
> >> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> >> ---
> >>   ipc/sem.c | 43 ++++++++++++++++++++++++++++++-------------
> >>   1 file changed, 30 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/ipc/sem.c b/ipc/sem.c
> >> index 19c8b98..a2fa795 100644
> >> --- a/ipc/sem.c
> >> +++ b/ipc/sem.c
> >> @@ -1229,6 +1229,12 @@ static int semctl_setval(struct ipc_namespace *ns, int semid, int semnum,
> >>   
> >>   	sem_lock(sma, NULL, -1);
> >>   
> >> +	if (sma->sem_perm.deleted) {
> >> +		sem_unlock(sma, -1);
> >> +		rcu_read_unlock();
> >> +		return -EIDRM;
> >> +	}
> >> +
> >>   	curr = &sma->sem_base[semnum];
> >>   
> >>   	ipc_assert_locked_object(&sma->sem_perm);
> >> @@ -1285,10 +1291,8 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> >>   		sem_lock(sma, NULL, -1);
> >>   		if(nsems > SEMMSL_FAST) {
> >>   			if (!ipc_rcu_getref(sma)) {
> >> -				sem_unlock(sma, -1);
> >> -				rcu_read_unlock();
> >>   				err = -EIDRM;
> >> -				goto out_free;
> >> +				goto out_unlock;
> >>   			}
> >>   			sem_unlock(sma, -1);
> >>   			rcu_read_unlock();
> >> @@ -1301,10 +1305,13 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
> >>   			rcu_read_lock();
> >>   			sem_lock_and_putref(sma);
> >>   			if (sma->sem_perm.deleted) {
> >> -				sem_unlock(sma, -1);
> >> -				rcu_read_unlock();
> >>   				err = -EIDRM;
> ^^^^^^^^^^^^^^^^^^ check if nsems > SEMMSL_FAST
> >> -				goto out_free;
> >> +				goto out_unlock;
> >> +			}
> >> +		} else {
> >> +			if (sma->sem_perm.deleted) {
> >> +				err = -EIDRM;
> >> +				goto out_unlock;
> >>   			}
> > I'm a bit lost here. Why should we only check the existence of the sem
> > if nsems <= SEMMSL_FAST? Shouldn't the same should apply either way?
> It is checked in both branches:
> - the check for "nsems > SEMMSL_FAST" was always there, due to the 
> kmalloc, the lock is dropped.

Right but the same 'race with rmid' logic could apply independently of
nsems: between looking up the sma, doing the checks and taking the lock,
it could have been removed underneath us, so we should check for its
existence right after taking the lock, something like this instead:

 	case GETALL:
	{
		ushort __user *array = p;
		int i;

		sem_lock(sma, NULL, -1);

		if (sma->sem_perm.deleted) {
			sem_unlock(sma, -1);
			rcu_read_unlock();
			return -EIDRM;
		}

In this GETALL case we drop the lock later down the road for
ipc_alloc(), and then check for sem_perm.deleted right after we take it
again. That seems fine, but doesn't take away the fact of the race
mentioned above.

Thanks,
Davidlohr