Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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
> >> ---
> >> 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 = >sem_base[semnum];
> >>
> >>ipc_assert_locked_object(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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 manf...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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
---
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 = >sem_base[semnum];
ipc_assert_locked_object(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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
> ---
> 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 = >sem_base[semnum];
>
> ipc_assert_locked_object(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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
---
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 = >sem_base[semnum];
ipc_assert_locked_object(>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 = >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) {
+
[PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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 manf...@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) {
+
Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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 manf...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ipc/sem.c: synchronize semop and semctl with IPC_RMID
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 manf...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
