[PATCH -mm] ipc,sem: fix locking in semctl_main

[PATCH -mm] ipc,sem: fix locking in semctl_main

[Rik van Riel]

Andrew, here is the patch I promised :)

Everywhere else in the code, we check sma->sem_perm.deleted under
the semaphore array lock, so we should do teh same here.
The easy fix is to simply not drop and re-take the lock, but keep
it locked.

This patch can be folded into ipcsem-fine-grained-locking-for-semtimedop.patch

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 ipc/sem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 5711616..efdaee6 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1243,10 +1243,9 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 				err = -EIDRM;
 				goto out_free;
 			}
-			sem_unlock(sma, -1);
-		}
+		} else
+			sem_lock(sma, NULL, -1);
 
-		sem_lock(sma, NULL, -1);
 		for (i = 0; i < sma->sem_nsems; i++)
 			sem_io[i] = sma->sem_base[i].semval;
 		sem_unlock(sma, -1);

Re: [PATCH -mm] ipc,sem: fix locking in semctl_main

[Davidlohr Bueso]

On Tue, 2013-04-23 at 12:29 -0400, Rik van Riel wrote:
> Andrew, here is the patch I promised :)
> 
> Everywhere else in the code, we check sma->sem_perm.deleted under
> the semaphore array lock, so we should do teh same here.
> The easy fix is to simply not drop and re-take the lock, but keep
> it locked.
> 

CCing Linus, Sedat and Emmanuel.

This looks very much like a patch I asked Sedat to test during the
weekend, which according to him didn't solve his issue. The patch is
correct in any case.

> This patch can be folded into ipcsem-fine-grained-locking-for-semtimedop.patch
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>

Acked-by: Davidlohr Bueso <davidlohr.bueso@hp.com>

> ---
>  ipc/sem.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 5711616..efdaee6 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1243,10 +1243,9 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>  				err = -EIDRM;
>  				goto out_free;
>  			}
> -			sem_unlock(sma, -1);
> -		}
> +		} else
> +			sem_lock(sma, NULL, -1);
>  
> -		sem_lock(sma, NULL, -1);
>  		for (i = 0; i < sma->sem_nsems; i++)
>  			sem_io[i] = sma->sem_base[i].semval;
>  		sem_unlock(sma, -1);

Re: [PATCH -mm] ipc,sem: fix locking in semctl_main

[Sedat Dilek]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

On Tue, Apr 23, 2013 at 7:36 PM, Davidlohr Bueso <davidlohr.bueso@hp.com> wrote:
> On Tue, 2013-04-23 at 12:29 -0400, Rik van Riel wrote:
>> Andrew, here is the patch I promised :)
>>
>> Everywhere else in the code, we check sma->sem_perm.deleted under
>> the semaphore array lock, so we should do teh same here.
>> The easy fix is to simply not drop and re-take the lock, but keep
>> it locked.
>>
>
> CCing Linus, Sedat and Emmanuel.
>
> This looks very much like a patch I asked Sedat to test during the
> weekend, which according to him didn't solve his issue. The patch is
> correct in any case.
>

Your patch looked a bit different (checked for ipc-lock already taken,
both patches - original and refreshed againt -next attached).

- Sedat -

>> This patch can be folded into ipcsem-fine-grained-locking-for-semtimedop.patch
>>
>> Signed-off-by: Rik van Riel <riel@redhat.com>
>
> Acked-by: Davidlohr Bueso <davidlohr.bueso@hp.com>
>
>> ---
>>  ipc/sem.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index 5711616..efdaee6 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -1243,10 +1243,9 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
>>                               err = -EIDRM;
>>                               goto out_free;
>>                       }
>> -                     sem_unlock(sma, -1);
>> -             }
>> +             } else
>> +                     sem_lock(sma, NULL, -1);
>>
>> -             sem_lock(sma, NULL, -1);
>>               for (i = 0; i < sma->sem_nsems; i++)
>>                       sem_io[i] = sma->sem_base[i].semval;
>>               sem_unlock(sma, -1);
>
>

[-- Attachment #2: ipc-fix.patch --]
[-- Type: application/octet-stream, Size: 721 bytes --]

From: Davidlohr Bueso <davidlohr.bueso@hp.com> 

Subject: ipc: sem: Check if the ipc lock has been already taken

Original patch from [1].

[1] http://marc.info/?t=136631457900005&r=1&w=2

diff --git a/ipc/sem.c b/ipc/sem.c
index 5711616..1dfc3c1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1243,10 +1243,11 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 				err = -EIDRM;
 				goto out_free;
 			}
-			sem_unlock(sma, -1);
 		}
 
-		sem_lock(sma, NULL, -1);
+		/* has the ipc lock already been taken? */
+		if(nsems <= SEMMSL_FAST)
+			sem_lock(sma, NULL, -1);
 		for (i = 0; i < sma->sem_nsems; i++)
 			sem_io[i] = sma->sem_base[i].semval;
 		sem_unlock(sma, -1);

[-- Attachment #3: ipc-fix-v2.patch --]
[-- Type: application/octet-stream, Size: 836 bytes --]

From: Davidlohr Bueso <davidlohr.bueso@hp.com> 

Subject: ipc, sem: Check if the ipc lock has been already taken

Original patch from [1].

[ dileks: v2: Adapted to fit next-20130419 plus 3 ipc-sem-next patches from [2] ]

[1] http://marc.info/?t=136631457900005&r=1&w=2
[2] http://marc.info/?l=linux-next&m=136640233601913&w=2

diff --git a/ipc/sem.c b/ipc/sem.c
index acada9f..d90da47 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1259,10 +1259,11 @@ static int semctl_main(struct ipc_namespace *ns, int semid, int semnum,
 				err = -EIDRM;
 				goto out_free;
 			}
-			sem_unlock(sma, -1);
 		}
 
-		sem_lock(sma, NULL, -1);
+		/* has the ipc lock already been taken? */
+		if (nsems <= SEMMSL_FAST)
+			sem_lock(sma, NULL, -1);
 		for (i = 0; i < sma->sem_nsems; i++)
 			sem_io[i] = sma->sem_base[i].semval;
 		sem_unlock(sma, -1);