Re: [PATCH] mm/gup_test: fix race with PIN_LONGTERM_TEST ioctls

[John Hubbard]

On 6/7/26 7:50 PM, Yunhui Cui wrote:
> The PIN_LONGTERM_TEST helpers keep their state in global variables that
> are protected by pin_longterm_test_mutex when accessed from ioctl().
> However, gup_test_release() calls pin_longterm_test_stop() without
> holding that mutex.
> 
> This can race with PIN_LONGTERM_TEST_STOP and let two callers operate on
> the same pages array concurrently, corrupting the test state and possibly
> freeing it twice:

Let's add here that there are *no* such callers in the kernel, today.

> 
>  CPU 0                              CPU 1
>  -----                              -----
>  ioctl(PIN_LONGTERM_TEST_STOP)
>    mutex_lock(&pin_longterm_test_mutex)
>    pin_longterm_test_stop()
>      if (pin_longterm_test_pages)
>        kvfree(pin_longterm_test_pages)
> 
>                                     close()
>                                       gup_test_release()
>                                         pin_longterm_test_stop()
>                                           if (pin_longterm_test_pages)
>                                             kvfree(pin_longterm_test_pages)
> 
>      pin_longterm_test_pages = NULL
>    mutex_unlock(&pin_longterm_test_mutex)
> 
> Protect the release path with the same mutex so that stop and release
> cannot run pin_longterm_test_stop() concurrently.
> 
> Fixes: c77369b437f9 ("mm/gup_test: start/stop/read functionality for PIN LONGTERM test")
> Cc: stable@xxxxxxxxxxxxxxx

umm, no, to "Cc: stable". This is the sort of thing that gives AI
a bad name. Specifically:

* Nothing in tree can possibly hit this race condition.

* This fix is purely static code analysis hygiene: correcting
  a theoretical problem that does not actually provide any
  sort of vulnerability fix in the kernel.

So claiming that the fix must go to stable is AI just making
overly grandiose claims, which I'm getting used to seeing lately,
but it still irritates.

> Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
> ---
>  mm/gup_test.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/gup_test.c b/mm/gup_test.c
> index 9dd48db897b95..d1c2b1014f0ef 100644
> --- a/mm/gup_test.c
> +++ b/mm/gup_test.c
> @@ -373,7 +373,9 @@ static long gup_test_ioctl(struct file *filep, unsigned int cmd,
>  
>  static int gup_test_release(struct inode *inode, struct file *file)
>  {
> +	mutex_lock(&pin_longterm_test_mutex);
>  	pin_longterm_test_stop();
> +	mutex_unlock(&pin_longterm_test_mutex);
>  
>  	return 0;
>  }

With "Cc: stable", removed, please feel free to add:

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

thanks,
-- 
John Hubbard