linux-kernel - Re: [PATCH 7/7] xen/evtchn: read producer index only once

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: <3b415b97-da42-c680-90e2-8b984934b846@suse.com>
Date:   Mon, 8 Feb 2021 13:26:25 +0100
From:   Jürgen Groß <jgross@...e.com>
To:     Jan Beulich <jbeulich@...e.com>
Cc:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org
Subject: Re: [PATCH 7/7] xen/evtchn: read producer index only once

On 08.02.21 13:23, Jan Beulich wrote:
> On 08.02.2021 13:15, Jürgen Groß wrote:
>> On 08.02.21 12:54, Jan Beulich wrote:
>>> On 08.02.2021 11:59, Jürgen Groß wrote:
>>>> On 08.02.21 11:51, Jan Beulich wrote:
>>>>> On 08.02.2021 11:41, Jürgen Groß wrote:
>>>>>> On 08.02.21 10:48, Jan Beulich wrote:
>>>>>>> On 06.02.2021 11:49, Juergen Gross wrote:
>>>>>>>> In evtchn_read() use READ_ONCE() for reading the producer index in
>>>>>>>> order to avoid the compiler generating multiple accesses.
>>>>>>>>
>>>>>>>> Signed-off-by: Juergen Gross <jgross@...e.com>
>>>>>>>> ---
>>>>>>>>      drivers/xen/evtchn.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/xen/evtchn.c b/drivers/xen/evtchn.c
>>>>>>>> index 421382c73d88..f6b199b597bf 100644
>>>>>>>> --- a/drivers/xen/evtchn.c
>>>>>>>> +++ b/drivers/xen/evtchn.c
>>>>>>>> @@ -211,7 +211,7 @@ static ssize_t evtchn_read(struct file *file, char __user *buf,
>>>>>>>>      			goto unlock_out;
>>>>>>>>      
>>>>>>>>      		c = u->ring_cons;
>>>>>>>> -		p = u->ring_prod;
>>>>>>>> +		p = READ_ONCE(u->ring_prod);
>>>>>>>>      		if (c != p)
>>>>>>>>      			break;
>>>>>>>
>>>>>>> Why only here and not also in
>>>>>>>
>>>>>>> 		rc = wait_event_interruptible(u->evtchn_wait,
>>>>>>> 					      u->ring_cons != u->ring_prod);
>>>>>>>
>>>>>>> or in evtchn_poll()? I understand it's not needed when
>>>>>>> ring_prod_lock is held, but that's not the case in the two
>>>>>>> afaics named places. Plus isn't the same then true for
>>>>>>> ring_cons and ring_cons_mutex, i.e. aren't the two named
>>>>>>> places plus evtchn_interrupt() also in need of READ_ONCE()
>>>>>>> for ring_cons?
>>>>>>
>>>>>> The problem solved here is the further processing using "p" multiple
>>>>>> times. p must not be silently replaced with u->ring_prod by the
>>>>>> compiler, so I probably should reword the commit message to say:
>>>>>>
>>>>>> ... in order to not allow the compiler to refetch p.
>>>>>
>>>>> I still wouldn't understand the change (and the lack of
>>>>> further changes) then: The first further use of p is
>>>>> outside the loop, alongside one of c. IOW why would c
>>>>> then not need treating the same as p?
>>>>
>>>> Its value wouldn't change, as ring_cons is being modified only at
>>>> the bottom of this function, and nowhere else (apart from the reset
>>>> case, but this can't run concurrently due to ring_cons_mutex).
>>>>
>>>>> I also still don't see the difference between latching a
>>>>> value into a local variable vs a "freestanding" access -
>>>>> neither are guaranteed to result in exactly one memory
>>>>> access afaict.
>>>>
>>>> READ_ONCE() is using a pointer to volatile, so any refetching by
>>>> the compiler would be a bug.
>>>
>>> Of course, but this wasn't my point. I was contrasting
>>>
>>> 		c = u->ring_cons;
>>> 		p = u->ring_prod;
>>>
>>> which you change with
>>>
>>> 		rc = wait_event_interruptible(u->evtchn_wait,
>>> 					      u->ring_cons != u->ring_prod);
>>>
>>> which you leave alone.
>>
>> Can you point out which problem might arise from that?
> 
> Not any particular active one. Yet enhancing some accesses
> but not others seems to me like a recipe for new problems
> down the road.

I already reasoned that the usage of READ_ONCE() is due to storing the
value in a local variable which needs to be kept constant during the
following processing (no refetches by the compiler). This reasoning
very clearly doesn't apply to the other accesses.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3092 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)