[PATCH] xvmalloc: adjust XVFREE() ordering

[PATCH] xvmalloc: adjust XVFREE() ordering

[Jan Beulich]

What c4f427ec879e ("xen: Swap order of actions in the FREE*() macros") did
should have been done right away when XVFREE() was introduced.

Amends: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xen/xvmalloc.h
+++ b/xen/include/xen/xvmalloc.h
@@ -71,8 +71,9 @@ void *_xvrealloc(void *va, size_t size,
 
 /* Free an allocation, and zero the pointer to it. */
 #define XVFREE(p) do { \
-    xvfree(p);         \
+    void *_ptr_ = (p); \
     (p) = NULL;        \
+    xvfree(_ptr_);     \
 } while ( false )
 
 static inline void *_xvmalloc_array(

Re: [PATCH] xvmalloc: adjust XVFREE() ordering

[Roger Pau Monné]

On Thu, Apr 16, 2026 at 04:32:54PM +0200, Jan Beulich wrote:
> What c4f427ec879e ("xen: Swap order of actions in the FREE*() macros") did
> should have been done right away when XVFREE() was introduced.
> 
> Amends: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

No intention to stir controversy, but I feel like this should better
use the Fixes tag, as it's not an omission or similar, but code fix.

Thanks, Roger.

Re: [PATCH] xvmalloc: adjust XVFREE() ordering

[Jan Beulich]

On 16.04.2026 16:39, Roger Pau Monné wrote:
> On Thu, Apr 16, 2026 at 04:32:54PM +0200, Jan Beulich wrote:
>> What c4f427ec879e ("xen: Swap order of actions in the FREE*() macros") did
>> should have been done right away when XVFREE() was introduced.
>>
>> Amends: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> No intention to stir controversy, but I feel like this should better
> use the Fixes tag, as it's not an omission or similar, but code fix.

I would have used Fixes: if I was able to spot a use where the difference
would actually matter in existing code. I can switch, but in the absence
thereof I deemed Amends: (marginally) more precise.

Jan

Re: [PATCH] xvmalloc: adjust XVFREE() ordering

[Andrew Cooper]

On 16/04/2026 3:50 pm, Jan Beulich wrote:
> On 16.04.2026 16:39, Roger Pau Monné wrote:
>> On Thu, Apr 16, 2026 at 04:32:54PM +0200, Jan Beulich wrote:
>>> What c4f427ec879e ("xen: Swap order of actions in the FREE*() macros") did
>>> should have been done right away when XVFREE() was introduced.
>>>
>>> Amends: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> Thanks.
>
>> No intention to stir controversy, but I feel like this should better
>> use the Fixes tag, as it's not an omission or similar, but code fix.
> I would have used Fixes: if I was able to spot a use where the difference
> would actually matter in existing code. I can switch, but in the absence
> thereof I deemed Amends: (marginally) more precise.

It's speculative defence in depth.

Zeroing the pointer in memory before freeing the block is strictly
better than the other way around.

For the local CPU it prevents speculative use-after-free (the store
queue is always in program order), and whether it does this for remote
CPUs depends on how strong the platform memory ordering is.

This all came from the Ghostrace paper.

~Andrew

Re: [PATCH] xvmalloc: adjust XVFREE() ordering

[Jan Beulich]

On 16.04.2026 16:32, Jan Beulich wrote:
> What c4f427ec879e ("xen: Swap order of actions in the FREE*() macros") did
> should have been done right away when XVFREE() was introduced.
> 
> Amends: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

On Arm64, and only for alpine-3.18-gcc-debug-arm64-boot-cpupools, this
reproducibly causes:

diff -u  ./.xen-syms.1.o.sym  ./.xen-syms.2.o.sym
--- ./.xen-syms.1.o.sym
+++ ./.xen-syms.2.o.sym
@@ -7,11 +7,11 @@
 0000000000000000 l    d  .bss	0000000000000000 .bss
 0000000000000000 l    d  .rodata	0000000000000000 .rodata
 0000000000000000 l    d  .note.GNU-stack	0000000000000000 .note.GNU-stack
-0000000000000000 g     O .rodata	0000000000004dd8 .hidden symbols_addresses
-0000000000004dd8 g     O .rodata	0000000000000004 .hidden symbols_num_addrs
-0000000000004ddc g     O .rodata	0000000000007721 .hidden symbols_names
-000000000000c500 g     O .rodata	0000000000000028 .hidden symbols_markers
-000000000000c528 g     O .rodata	00000000000003b5 .hidden symbols_token_table
-000000000000c8de g     O .rodata	0000000000000200 .hidden symbols_token_index
+0000000000000000 g     O .rodata	0000000000004de8 .hidden symbols_addresses
+0000000000004de8 g     O .rodata	0000000000000004 .hidden symbols_num_addrs
+0000000000004dec g     O .rodata	0000000000007749 .hidden symbols_names
+000000000000c538 g     O .rodata	0000000000000028 .hidden symbols_markers
+000000000000c560 g     O .rodata	00000000000003b5 .hidden symbols_token_table
+000000000000c916 g     O .rodata	0000000000000200 .hidden symbols_token_index

There are only two uses of XVFREE() in code that is built for Arm. I'll
see if I can repro locally with the exact same .config, but I fear my
chances are slim.

It feels wrong to push the patch nevertheless, but it also feels wrong to
have to keep it out.

Jan

> --- a/xen/include/xen/xvmalloc.h
> +++ b/xen/include/xen/xvmalloc.h
> @@ -71,8 +71,9 @@ void *_xvrealloc(void *va, size_t size,
>  
>  /* Free an allocation, and zero the pointer to it. */
>  #define XVFREE(p) do { \
> -    xvfree(p);         \
> +    void *_ptr_ = (p); \
>      (p) = NULL;        \
> +    xvfree(_ptr_);     \
>  } while ( false )
>  
>  static inline void *_xvmalloc_array(