[PATCH -next] ipc/sem: prevent queue.status tearing in semop

[PATCH -next] ipc/sem: prevent queue.status tearing in semop

[Davidlohr Bueso]

In order for load/store tearing to work, _all_ accesses to
the variable in question need to be done around READ and
WRITE_ONCE() macros. Ensure everyone does so for q->status
variable for semtimedop().

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 6cbbf34a44ac..ccab4e51d351 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -2125,7 +2125,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
 	}
 
 	do {
-		queue.status = -EINTR;
+		WRITE_ONCE(queue.status, -EINTR);
 		queue.sleeper = current;
 
 		__set_current_state(TASK_INTERRUPTIBLE);
-- 
2.16.4

Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop

[Davidlohr Bueso]

On Mon, 16 Jul 2018, Bueso wrote:

>In order for load/store tearing to work, _all_ accesses to
                                ^ prevention

Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop

[Manfred Spraul]

Hello Davidlohr,

On 07/17/2018 07:26 AM, Davidlohr Bueso wrote:
> In order for load/store tearing to work, _all_ accesses to
> the variable in question need to be done around READ and
> WRITE_ONCE() macros. Ensure everyone does so for q->status
> variable for semtimedop().
What is the background of the above rule?

sma->use_global_lock is sometimes used with smp_load_acquire(), 
sometimes without.
So far, I assumed that this is safe.

The same applies for nf_conntrack_locks_all, in nf_conntrack_all_lock()
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>   ipc/sem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 6cbbf34a44ac..ccab4e51d351 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -2125,7 +2125,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
>   	}
>   
>   	do {
> -		queue.status = -EINTR;
> +		WRITE_ONCE(queue.status, -EINTR);
>   		queue.sleeper = current;
>   
>   		__set_current_state(TASK_INTERRUPTIBLE);

Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop

[Davidlohr Bueso]

On Wed, 18 Jul 2018, Manfred Spraul wrote:

>Hello Davidlohr,
>
>On 07/17/2018 07:26 AM, Davidlohr Bueso wrote:
>>In order for load/store tearing to work, _all_ accesses to
>>the variable in question need to be done around READ and
>>WRITE_ONCE() macros. Ensure everyone does so for q->status
>>variable for semtimedop().
>What is the background of the above rule?

The fact that it's done under the ipc lock, doesn't mean that
the compiler won't try to get smart. If we have lockless accesses
we musn't see partial stores/loads.

>
>sma->use_global_lock is sometimes used with smp_load_acquire(), 
>sometimes without.
>So far, I assumed that this is safe.
>
>The same applies for nf_conntrack_locks_all, in nf_conntrack_all_lock()

Oh, yeah I remember _that_ saga. I'll have a look but iirc it
seemd ok back then.

Thanks,
Davidlohr

Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop

[Davidlohr Bueso]

On Wed, 18 Jul 2018, Manfred Spraul wrote:

>sma->use_global_lock is sometimes used with smp_load_acquire(), 
>sometimes without.
>So far, I assumed that this is safe.
>
>The same applies for nf_conntrack_locks_all, in nf_conntrack_all_lock()

So the netfilter code is safe wrt tearing as _all_ accesses are done with
barriers and/or under spinlock.

However, this isn't always the case for sma->use_global_lock, albeit harmless.

- sem_lock(): It doesn't matter if we get the first check right as we
end up rechecking with locks held.

	/*
	 * Initial check for use_global_lock. Just an optimization,
	 * no locking, no memory barrier.
	 */
	if (!sma->use_global_lock) {

- complexmode_enter/tryleave() are called under the ipc object lock, so that
is safe:

spin_lock()
complexmode_enter()
...
complexmode_tryleave()
spin_unlock()

- newary(): Init, no concurrency, of course.

So while I also like READ/WRITE_ONCE() calls in that it helps document the
code, I don't think we need/want want this. There's a comment there in the
first place.

Thanks,
Davidlohr