LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH -next] ipc/sem: prevent queue.status tearing in semop
@ 2018-07-17  5:26 Davidlohr Bueso
  2018-07-17  5:28 ` Davidlohr Bueso
  2018-07-18  3:55 ` Manfred Spraul
  0 siblings, 2 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2018-07-17  5:26 UTC (permalink / raw)
  To: akpm; +Cc: manfred, dave, linux-kernel, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop
  2018-07-17  5:26 [PATCH -next] ipc/sem: prevent queue.status tearing in semop Davidlohr Bueso
@ 2018-07-17  5:28 ` Davidlohr Bueso
  2018-07-18  3:55 ` Manfred Spraul
  1 sibling, 0 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2018-07-17  5:28 UTC (permalink / raw)
  To: akpm; +Cc: manfred, linux-kernel, Davidlohr Bueso

On Mon, 16 Jul 2018, Bueso wrote:

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop
  2018-07-17  5:26 [PATCH -next] ipc/sem: prevent queue.status tearing in semop Davidlohr Bueso
  2018-07-17  5:28 ` Davidlohr Bueso
@ 2018-07-18  3:55 ` Manfred Spraul
  2018-07-20 18:25   ` Davidlohr Bueso
  2018-07-30 22:08   ` Davidlohr Bueso
  1 sibling, 2 replies; 5+ messages in thread
From: Manfred Spraul @ 2018-07-18  3:55 UTC (permalink / raw)
  To: Davidlohr Bueso, akpm; +Cc: linux-kernel, Davidlohr Bueso

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);



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop
  2018-07-18  3:55 ` Manfred Spraul
@ 2018-07-20 18:25   ` Davidlohr Bueso
  2018-07-30 22:08   ` Davidlohr Bueso
  1 sibling, 0 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2018-07-20 18:25 UTC (permalink / raw)
  To: Manfred Spraul; +Cc: akpm, linux-kernel, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH -next] ipc/sem: prevent queue.status tearing in semop
  2018-07-18  3:55 ` Manfred Spraul
  2018-07-20 18:25   ` Davidlohr Bueso
@ 2018-07-30 22:08   ` Davidlohr Bueso
  1 sibling, 0 replies; 5+ messages in thread
From: Davidlohr Bueso @ 2018-07-30 22:08 UTC (permalink / raw)
  To: Manfred Spraul; +Cc: akpm, linux-kernel, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-17  5:26 [PATCH -next] ipc/sem: prevent queue.status tearing in semop Davidlohr Bueso
2018-07-17  5:28 ` Davidlohr Bueso
2018-07-18  3:55 ` Manfred Spraul
2018-07-20 18:25   ` Davidlohr Bueso
2018-07-30 22:08   ` Davidlohr Bueso

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox