[4/4] x86/mm: drop redundant smp_wmb() from _put_final_page_type()
diff mbox series

Message ID 272862bd-0b05-9a66-c79e-76502b89dd38@suse.com
State New
Headers show
Series
  • x86/mm: XSA-299 / 309 / 310 follow-up
Related show

Commit Message

Jan Beulich Dec. 20, 2019, 2:20 p.m. UTC
get_page_light()'s use of cmpxchg() is a full barrier already anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper Dec. 20, 2019, 2:51 p.m. UTC | #1
On 20/12/2019 14:20, Jan Beulich wrote:
> get_page_light()'s use of cmpxchg() is a full barrier already anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

While true, is this actually a clever change to make?

The implementation of get_page_light() could plausibly change and no
longer be a full barrier, introducing a vulnerability here.  OTOH,
smp_wmb() is free.
Jan Beulich Dec. 20, 2019, 2:55 p.m. UTC | #2
On 20.12.2019 15:51, Andrew Cooper wrote:
> On 20/12/2019 14:20, Jan Beulich wrote:
>> get_page_light()'s use of cmpxchg() is a full barrier already anyway.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> While true, is this actually a clever change to make?
> 
> The implementation of get_page_light() could plausibly change and no
> longer be a full barrier, introducing a vulnerability here.  OTOH,
> smp_wmb() is free.

It's free at the CPU level, but not at the compiler one (where
it still is a barrier). I also don't think get_page_light()
could change as drastically as losing its LOCK-ed operation,
or if it did the author (and reviewer) would be well advised
to at least briefly audit use sites (one of the reasons I'm
leaving a comment).

Jan
Andrew Cooper Dec. 20, 2019, 2:59 p.m. UTC | #3
On 20/12/2019 14:55, Jan Beulich wrote:
> On 20.12.2019 15:51, Andrew Cooper wrote:
>> On 20/12/2019 14:20, Jan Beulich wrote:
>>> get_page_light()'s use of cmpxchg() is a full barrier already anyway.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> While true, is this actually a clever change to make?
>>
>> The implementation of get_page_light() could plausibly change and no
>> longer be a full barrier, introducing a vulnerability here.  OTOH,
>> smp_wmb() is free.
> It's free at the CPU level, but not at the compiler one (where
> it still is a barrier). I also don't think get_page_light()
> could change as drastically as losing its LOCK-ed operation,
> or if it did the author (and reviewer) would be well advised
> to at least briefly audit use sites (one of the reasons I'm
> leaving a comment).

The comment should be at get_page_light() then, saying "some callers
depend on this function being a full memory barrier", which will be far
more obvious for anyone who changes the behaviour.

With an adjustment along those lines, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Patch
diff mbox series

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2746,7 +2746,7 @@  static int _put_final_page_type(struct p
     else
     {
         BUG_ON(rc != -ERESTART);
-        smp_wmb();
+        /* get_page_light() includes a full barrier. */
         get_page_light(page);
         page->u.inuse.type_info |= PGT_partial;
     }