linux-kernel - Re: [PATCH v13 3/4] fw_cfg: write vmcoreinfo details

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]

Message-ID: <CAMxuvaxq9boTcJ2uDOJsYyo3=_WYtn1==kuq3QbPygBfVc+8Zw@mail.gmail.com>
Date:   Tue, 13 Feb 2018 16:16:08 +0100
From:   Marc-Andre Lureau <mlureau@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Marc-André Lureau <marcandre.lureau@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Sergio Lopez Pascual <slp@...hat.com>,
        Baoquan He <bhe@...hat.com>, "Somlo, Gabriel" <somlo@....edu>,
        xiaolong.ye@...el.com
Subject: Re: [PATCH v13 3/4] fw_cfg: write vmcoreinfo details

Hi

On Tue, Feb 13, 2018 at 3:27 PM, Michael S. Tsirkin <mst@...hat.com> wrote:
> On Tue, Feb 13, 2018 at 03:14:03PM +0100, Marc-Andre Lureau wrote:
>> Hi
>>
>> On Mon, Feb 12, 2018 at 10:00 PM, Michael S. Tsirkin <mst@...hat.com> wrote:
>> > On Mon, Feb 12, 2018 at 11:04:49AM +0100, Marc-Andre Lureau wrote:
>> >> >> +}
>> >> >> +
>> >> >> +/* qemu fw_cfg device is sync today, but spec says it may become async */
>> >> >> +static void fw_cfg_wait_for_control(struct fw_cfg_dma *d)
>> >> >> +{
>> >> >> +     do {
>> >> >> +             u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
>> >> >> +
>> >> >> +             if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
>> >> >> +                     return;
>> >> >> +
>> >> >> +             usleep_range(50, 100);
>> >> >> +     } while (true);
>> >> >
>> >> > And you need an smp rmb here.
>> >
>> > I'd just do rmb() in fact.
>> >
>> >> Could you explain? thanks
>> >
>> > See Documentation/memory-barriers.txt
>> > You know that control is valid, but following read of
>> > the structure could be reordered. So you need that barrier there.
>> > Same for write: wmb.
>>
>> Is this ok?
>> @@ -103,10 +104,14 @@ static ssize_t fw_cfg_dma_transfer(void
>> *address, u32 length, u32 control)
>>         dma = virt_to_phys(d);
>>
>>         iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
>> +       /* force memory to sync before notifying device via MMIO */
>> +       wmb();
>>         iowrite32be(dma, fw_cfg_reg_dma + 4);
>>
>>         fw_cfg_wait_for_control(d);
>>
>> +       /* do not reorder the read to d->control */
>> +       rmb();
>>         if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
>>                 ret = -EIO;
>>         }
>
> I think you need an rmb after the read of d->control.
>


There are two reads of d->control, one in fw_cfg_wait_for_control() to
wait for completion, and the other one here to handle error. Do you
mean that for clarity rmb() should be moved at the end of
fw_cfg_wait_for_control() instead?

thanks