linux-kernel - Re: [PATCH V4 8/9] virtio: harden vring IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 May 2022 10:44:14 +0200
From:   Cornelia Huck <cohuck@...hat.com>
To:     Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Marc Zyngier <maz@...nel.org>,
        Halil Pasic <pasic@...ux.ibm.com>,
        eperezma <eperezma@...hat.com>, Cindy Lu <lulu@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH V4 8/9] virtio: harden vring IRQ

On Wed, May 11 2022, Jason Wang <jasowang@...hat.com> wrote:

> On Tue, May 10, 2022 at 7:32 PM Michael S. Tsirkin <mst@...hat.com> wrote:
>>
>> On Sat, May 07, 2022 at 03:19:53PM +0800, Jason Wang wrote:
>> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> > index d8a2340f928e..23f1694cdbd5 100644
>> > --- a/include/linux/virtio_config.h
>> > +++ b/include/linux/virtio_config.h
>> > @@ -256,6 +256,18 @@ void virtio_device_ready(struct virtio_device *dev)
>> >       unsigned status = dev->config->get_status(dev);
>> >
>> >       BUG_ON(status & VIRTIO_CONFIG_S_DRIVER_OK);
>> > +
>> > +     /*
>> > +      * The virtio_synchronize_cbs() makes sure vring_interrupt()
>> > +      * will see the driver specific setup if it sees vq->broken
>> > +      * as false.
>> > +      */
>> > +     virtio_synchronize_cbs(dev);
>>
>> since you mention vq->broken above, maybe add
>>         "set vq->broken to false"
>
> Ok.
>
>>
>> > +     __virtio_unbreak_device(dev);
>> > +     /*
>> > +      * The transport is expected ensure the visibility of
>>
>> to ensure
>
> Will fix.
>
>>
>> > +      * vq->broken
>>
>> let's add: "visibility by vq callbacks"
>
> Sure.
>
>>
>> > before setting VIRTIO_CONFIG_S_DRIVER_OK.
>> > +      */
>>
>>
>> Can I see some analysis of existing transports showing
>> this is actually the case for them?
>
> Yes.
>
>> And maybe add a comment near set_status to document the
>> requirement.
>
> For PCI and MMIO, we can quote the memory-barriers.txt or explain that
> wmb() is not needed before the MMIO writel().
> For CCW, it looks not obvious, it looks to me the IO was submitted via
> __ssch() which has an inline assembly.  Cornelia and Hali, could you
> help me to understand if and how did virtio_ccw_set_status() can
> ensure the visibility of the previous driver setup and vq->broken
> here?

I'm not sure I completely understand the question here, but let me try:

virtio_ccw_set_status() uses a channel command to set the status, with
the interesting stuff done inside ccw_io_helper(). That function
- takes the subchannel lock, disabling interrupts
- does the ssch; this instruction will fail if there's already another
  I/O in progress, or an interrupt is pending for the subchannel; on
  success, it is guaranteed that we'll get an interrupt eventually
- unlock the subchannel, and wait for the interupt handler to eventually
  process the interrupt, so I guess it should see the vq->broken value?

If the I/O fails, virtio_ccw_set_status() will revert its internal
status to the old value.


>
> Thanks
>
>>
>> >       dev->config->set_status(dev, status | VIRTIO_CONFIG_S_DRIVER_OK);
>> >  }