All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@kernel.org>,
	david.kaplan@amd.com, mst@redhat.com, f.hetzelt@tu-berlin.de,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	konrad.wilk@oracle.com, Will Deacon <will@kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>
Subject: Re: [PATCH 7/9] virtio-pci: harden INTX interrupts
Date: Tue, 14 Sep 2021 13:09:31 +0200	[thread overview]
Message-ID: <87tuinct1w.ffs@tglx> (raw)
In-Reply-To: <YUCBZjjk77q8JS4f@hirez.programming.kicks-ass.net>

On Tue, Sep 14 2021 at 13:03, Peter Zijlstra wrote:
> On Mon, Sep 13, 2021 at 11:36:24PM +0200, Thomas Gleixner wrote:
> Here you rely on the UNLOCK+LOCK pattern because we have two adjacent
> critical sections (or rather, the same twice), which provides RCtso
> ordering, which is sufficient to make the below store:
>
>> 
>>         intx_soft_enabled = true;
>
> a RELEASE. still, I would suggest writing it at least using
> WRITE_ONCE() with a comment on.

Right. forgot about that.

> 	disable_irq();
> 	/*
> 	 * The above disable_irq() provides TSO ordering and as such
> 	 * promotes the below store to store-release.
> 	 */
> 	WRITE_ONCE(intx_soft_enabled, true);
> 	enable_irq();
>
>> In this case synchronize_irq() prevents the subsequent store to
>> intx_soft_enabled to leak into the __disable_irq(desc) section which in
>> turn makes it impossible for an interrupt handler to observe
>> intx_soft_enabled == true before the prerequisites which preceed the
>> call to disable_irq() are visible.
>> 
>> Of course the memory ordering wizards might disagree, but if they do,
>> then we have a massive chase of ordering problems vs. similar constructs
>> all over the tree ahead of us.
>
> Your case, UNLOCK s + LOCK s, is fully documented to provide RCtso
> ordering. The more general case of: UNLOCK r + LOCK s, will shortly
> appear in documentation near you. Meaning we can forget about the
> details an blanket state that any UNLOCK followed by a LOCK (on the same
> CPU) will provide TSO ordering.

I think we also should document the disable/synchronize_irq() scheme
somewhere.

Thanks,

        tglx

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Jason Wang <jasowang@redhat.com>,
	mst@redhat.com, virtualization@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org, f.hetzelt@tu-berlin.de,
	david.kaplan@amd.com, konrad.wilk@oracle.com,
	Will Deacon <will@kernel.org>, Boqun Feng <boqun.feng@gmail.com>,
	"Paul E. McKenney" <paulmck@kernel.org>
Subject: Re: [PATCH 7/9] virtio-pci: harden INTX interrupts
Date: Tue, 14 Sep 2021 13:09:31 +0200	[thread overview]
Message-ID: <87tuinct1w.ffs@tglx> (raw)
In-Reply-To: <YUCBZjjk77q8JS4f@hirez.programming.kicks-ass.net>

On Tue, Sep 14 2021 at 13:03, Peter Zijlstra wrote:
> On Mon, Sep 13, 2021 at 11:36:24PM +0200, Thomas Gleixner wrote:
> Here you rely on the UNLOCK+LOCK pattern because we have two adjacent
> critical sections (or rather, the same twice), which provides RCtso
> ordering, which is sufficient to make the below store:
>
>> 
>>         intx_soft_enabled = true;
>
> a RELEASE. still, I would suggest writing it at least using
> WRITE_ONCE() with a comment on.

Right. forgot about that.

> 	disable_irq();
> 	/*
> 	 * The above disable_irq() provides TSO ordering and as such
> 	 * promotes the below store to store-release.
> 	 */
> 	WRITE_ONCE(intx_soft_enabled, true);
> 	enable_irq();
>
>> In this case synchronize_irq() prevents the subsequent store to
>> intx_soft_enabled to leak into the __disable_irq(desc) section which in
>> turn makes it impossible for an interrupt handler to observe
>> intx_soft_enabled == true before the prerequisites which preceed the
>> call to disable_irq() are visible.
>> 
>> Of course the memory ordering wizards might disagree, but if they do,
>> then we have a massive chase of ordering problems vs. similar constructs
>> all over the tree ahead of us.
>
> Your case, UNLOCK s + LOCK s, is fully documented to provide RCtso
> ordering. The more general case of: UNLOCK r + LOCK s, will shortly
> appear in documentation near you. Meaning we can forget about the
> details an blanket state that any UNLOCK followed by a LOCK (on the same
> CPU) will provide TSO ordering.

I think we also should document the disable/synchronize_irq() scheme
somewhere.

Thanks,

        tglx


  reply	other threads:[~2021-09-14 11:09 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-13  5:53 [PATCH 0/9] More virtio hardening Jason Wang
2021-09-13  5:53 ` Jason Wang
2021-09-13  5:53 ` [PATCH 1/9] virtio-blk: validate num_queues during probe Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  7:48   ` Stefano Garzarella
2021-09-13  7:48     ` Stefano Garzarella
2021-09-14  2:29     ` Jason Wang
2021-09-14  2:29       ` Jason Wang
2021-09-13 12:05   ` Stefan Hajnoczi
2021-09-13 12:05     ` Stefan Hajnoczi
2021-09-13  5:53 ` [PATCH 2/9] virtio: add doc for validate() method Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  5:53 ` [PATCH 3/9] virtio-console: switch to use .validate() Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  5:53 ` [PATCH 4/9] virtio_console: validate max_nr_ports before trying to use it Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  5:53 ` [PATCH 5/9] virtio_config: introduce a new ready method Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  5:53 ` [PATCH 6/9] virtio_pci: harden MSI-X interrupts Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  6:03   ` Michael S. Tsirkin
2021-09-13  6:03     ` Michael S. Tsirkin
2021-09-13  6:08     ` Jason Wang
2021-09-13  6:08       ` Jason Wang
2021-09-13  6:28       ` Michael S. Tsirkin
2021-09-13  6:28         ` Michael S. Tsirkin
2021-09-13  6:34         ` Jason Wang
2021-09-13  6:34           ` Jason Wang
2021-09-13  6:37           ` Michael S. Tsirkin
2021-09-13  6:37             ` Michael S. Tsirkin
2021-09-13  6:43             ` Jason Wang
2021-09-13  6:43               ` Jason Wang
2021-09-13  7:01               ` Michael S. Tsirkin
2021-09-13  7:01                 ` Michael S. Tsirkin
2021-09-13  7:15                 ` Jason Wang
2021-09-13  7:15                   ` Jason Wang
2021-09-13  6:50             ` Michael S. Tsirkin
2021-09-13  6:50               ` Michael S. Tsirkin
2021-09-13  7:07               ` Jason Wang
2021-09-13  7:07                 ` Jason Wang
2021-09-13 19:38                 ` Thomas Gleixner
2021-09-13 19:38                   ` Thomas Gleixner
2021-09-13 20:54                   ` Michael S. Tsirkin
2021-09-13 20:54                     ` Michael S. Tsirkin
2021-09-13 22:31                     ` Thomas Gleixner
2021-09-13 22:31                       ` Thomas Gleixner
2021-09-14  2:20                       ` Jason Wang
2021-09-14  2:20                         ` Jason Wang
2021-09-14  8:29                     ` Thomas Gleixner
2021-09-14  8:29                       ` Thomas Gleixner
2021-09-13  5:53 ` [PATCH 7/9] virtio-pci: harden INTX interrupts Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  6:33   ` Michael S. Tsirkin
2021-09-13  6:33     ` Michael S. Tsirkin
2021-09-13  6:36     ` Jason Wang
2021-09-13  6:36       ` Jason Wang
2021-09-13  6:41       ` Michael S. Tsirkin
2021-09-13  6:41         ` Michael S. Tsirkin
2021-09-13  6:45         ` Jason Wang
2021-09-13  6:45           ` Jason Wang
2021-09-13  7:02           ` Michael S. Tsirkin
2021-09-13  7:02             ` Michael S. Tsirkin
2021-09-13  7:17             ` Jason Wang
2021-09-13  7:17               ` Jason Wang
2021-09-13 21:36   ` Thomas Gleixner
2021-09-13 21:36     ` Thomas Gleixner
2021-09-13 22:01     ` Michael S. Tsirkin
2021-09-13 22:01       ` Michael S. Tsirkin
2021-09-13 22:20       ` Thomas Gleixner
2021-09-13 22:20         ` Thomas Gleixner
2021-09-14  2:50     ` Jason Wang
2021-09-14  2:50       ` Jason Wang
2021-09-14  9:34     ` Boqun Feng
2021-09-14  9:34       ` Boqun Feng
2021-09-14 11:03     ` Peter Zijlstra
2021-09-14 11:03       ` Peter Zijlstra
2021-09-14 11:09       ` Thomas Gleixner [this message]
2021-09-14 11:09         ` Thomas Gleixner
2021-09-13  5:53 ` [PATCH 8/9] virtio_ring: fix typos in vring_desc_extra Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  5:53 ` [PATCH 9/9] virtio_ring: validate used buffer length Jason Wang
2021-09-13  5:53   ` Jason Wang
2021-09-13  6:36   ` Michael S. Tsirkin
2021-09-13  6:36     ` Michael S. Tsirkin
2021-09-13  6:40     ` Jason Wang
2021-09-13  6:40       ` Jason Wang
2021-09-13  6:57       ` Michael S. Tsirkin
2021-09-13  6:57         ` Michael S. Tsirkin
2021-09-13  7:13         ` Jason Wang
2021-09-13  7:13           ` Jason Wang
2021-10-05  7:42 ` [PATCH 0/9] More virtio hardening Michael S. Tsirkin
2021-10-05  7:42   ` Michael S. Tsirkin
2021-10-11  7:36   ` Jason Wang
2021-10-11  7:36     ` Jason Wang
2021-10-11 12:36     ` Michael S. Tsirkin
2021-10-11 12:36       ` Michael S. Tsirkin
2021-10-12  2:43       ` Jason Wang
2021-10-12  2:43         ` Jason Wang
2021-10-12  5:44         ` Michael S. Tsirkin
2021-10-12  5:44           ` Michael S. Tsirkin
2021-10-12  6:11           ` Jason Wang
2021-10-12  6:11             ` Jason Wang
2021-10-12  6:35             ` Michael S. Tsirkin
2021-10-12  6:35               ` Michael S. Tsirkin
2021-10-12  6:43               ` Jason Wang
2021-10-12  6:43                 ` Jason Wang
2021-10-12  7:03                 ` Michael S. Tsirkin
2021-10-12  7:03                   ` Michael S. Tsirkin
2021-10-12  8:46                   ` Jason Wang
2021-10-12  8:46                     ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tuinct1w.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=boqun.feng@gmail.com \
    --cc=david.kaplan@amd.com \
    --cc=f.hetzelt@tu-berlin.de \
    --cc=konrad.wilk@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.