Move first read of gt->gt_version inside the critical region of the
rwlock, otherwise concurrent gnttab operations (silly as they would be)
may get mutually confused as to the actual current version.

Fixes: c1488502c949("grant-tables: do not fail attempts to...")
Reported-by: Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
This is far from a problem in practice, because kernels invoke this once and
then are done. Still, correctness mandates correctness.

There are a number of lockless reads of gt_version (e.g: right after unlock),
but they aren't very worrying because they are effectively snapshots of the
instantaneous version. I'd feel better if they were all atomic_read(), but all
Xen ports guarantee atomic access on aligned 4 octet fields, so I couldn't be
bothered to go chase them.
---
 xen/common/grant_table.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2dda1abd3f..ac9fed6001 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3184,11 +3184,12 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     if ( op.version == 2 && gt->max_version == 1 )
         goto out; /* Behave as before set_version was introduced. */
 
+    grant_write_lock(gt);
+
     res = 0;
     if ( gt->gt_version == op.version )
-        goto out;
+        goto out_unlock;
 
-    grant_write_lock(gt);
     /*
      * Make sure that the grant table isn't currently in use when we
      * change the version number, except for the first 8 entries which

base-commit: aaa34f23ac65b75c94d069e407a2698602f18d56
-- 
2.43.0

On 22.05.2026 12:57, Alejandro Vallejo wrote:
> Move first read of gt->gt_version inside the critical region of the
> rwlock, otherwise concurrent gnttab operations (silly as they would be)
> may get mutually confused as to the actual current version.
> 
> Fixes: c1488502c949("grant-tables: do not fail attempts to...")
> Reported-by: Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>
> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>

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

> There are a number of lockless reads of gt_version (e.g: right after unlock),
> but they aren't very worrying because they are effectively snapshots of the
> instantaneous version. I'd feel better if they were all atomic_read(), but all
> Xen ports guarantee atomic access on aligned 4 octet fields, so I couldn't be
> bothered to go chase them.

Sooner or later we will want to deal with all (latent) problems of this kind.

Jan

On Fri May 22, 2026 at 1:31 PM CEST, Jan Beulich wrote:
> On 22.05.2026 12:57, Alejandro Vallejo wrote:
>> Move first read of gt->gt_version inside the critical region of the
>> rwlock, otherwise concurrent gnttab operations (silly as they would be)
>> may get mutually confused as to the actual current version.
>> 
>> Fixes: c1488502c949("grant-tables: do not fail attempts to...")
>> Reported-by: Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>
>> Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks

>
>> There are a number of lockless reads of gt_version (e.g: right after unlock),
>> but they aren't very worrying because they are effectively snapshots of the
>> instantaneous version. I'd feel better if they were all atomic_read(), but all
>> Xen ports guarantee atomic access on aligned 4 octet fields, so I couldn't be
>> bothered to go chase them.
>
> Sooner or later we will want to deal with all (latent) problems of this kind.

Quite. Concurrency is hard enough without making assumptions about the
underlying ISAs :/

Cheers,
Alejandro