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

[yunhui cui]

Hi David,

On Fri, Jun 12, 2026 at 3:36 PM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> On 6/8/26 04:50, 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:
> >
> >  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)
>
> Okay, thinking about this some more ...
>
> I think what's really required here is that we have two separate "struct file",
> because otherwise release() cannot race with unlocked_ioctl().
>
> Which is something we didn't expect when we added this functionality.
>
> I think the proper way to handle this is by moving the state to the
> "struct file", to actually cleanly allow concurrent usage.
>
> So instead, I think we should do the following (untested):
>
> From 29e3d6fe00c4bd843d11bb548efa89bca478436c Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
> Date: Fri, 12 Jun 2026 09:22:25 +0200
> Subject: [PATCH] mm/gup_test: keep longterm pin state per file
>
> The pin longterm test currently stores its data globally, shared among
> multiple concurrent users of the interface (multiple open file
> descriptors -> multiple "struct file"'s). That makes
> the gup_test interface problematic to use concurrently: two users, such
> as concurrent selftest runs, can interfere with the same longterm
> pin state.
>
> While this has not been observed as a problem so far in practice, let's
> just handle it cleanly. There could be a way to trigger selftest
> failures by e.g., running the cow.c and gup_longerm.c selftests
> concurrently, but we usually run them sequentially. Let's add a "Fixes"
> tag to be safe.
>
> Fixes: c77369b437f9 ("mm/gup_test: start/stop/read functionality for PIN LONGTERM test")
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
>  mm/gup_test.c | 93 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 61 insertions(+), 32 deletions(-)
>
> diff --git a/mm/gup_test.c b/mm/gup_test.c
> index 9dd48db897b9..16916056677e 100644
> --- a/mm/gup_test.c
> +++ b/mm/gup_test.c
> @@ -8,6 +8,12 @@
>  #include <linux/highmem.h>
>  #include "gup_test.h"
>
> +struct gup_test_data {
> +       struct mutex longterm_mutex;
> +       struct page **longterm_pages;
> +       unsigned long longterm_nr_pages;
> +};
> +
>  static void put_back_pages(unsigned int cmd, struct page **pages,
>                            unsigned long nr_pages, unsigned int gup_test_flags)
>  {
> @@ -204,23 +210,20 @@ static int __gup_test_ioctl(unsigned int cmd,
>         return ret;
>  }
>
> -static DEFINE_MUTEX(pin_longterm_test_mutex);
> -static struct page **pin_longterm_test_pages;
> -static unsigned long pin_longterm_test_nr_pages;
> -
> -static inline void pin_longterm_test_stop(void)
> +static inline void pin_longterm_test_stop(struct gup_test_data *data)
>  {
> -       if (pin_longterm_test_pages) {
> -               if (pin_longterm_test_nr_pages)
> -                       unpin_user_pages(pin_longterm_test_pages,
> -                                        pin_longterm_test_nr_pages);
> -               kvfree(pin_longterm_test_pages);
> -               pin_longterm_test_pages = NULL;
> -               pin_longterm_test_nr_pages = 0;
> +       if (data->longterm_pages) {
> +               if (data->longterm_nr_pages)
> +                       unpin_user_pages(data->longterm_pages,
> +                                        data->longterm_nr_pages);
> +               kvfree(data->longterm_pages);
> +               data->longterm_pages = NULL;
> +               data->longterm_nr_pages = 0;
>         }
>  }
>
> -static inline int pin_longterm_test_start(unsigned long arg)
> +static inline int pin_longterm_test_start(struct gup_test_data *data,
> +               unsigned long arg)
>  {
>         long nr_pages, cur_pages, addr, remaining_pages;
>         int gup_flags = FOLL_LONGTERM;
> @@ -229,7 +232,7 @@ static inline int pin_longterm_test_start(unsigned long arg)
>         int ret = 0;
>         bool fast;
>
> -       if (pin_longterm_test_pages)
> +       if (data->longterm_pages)
>                 return -EINVAL;
>
>         if (copy_from_user(&args, (void __user *)arg, sizeof(args)))
> @@ -259,12 +262,12 @@ static inline int pin_longterm_test_start(unsigned long arg)
>                 return -EINTR;
>         }
>
> -       pin_longterm_test_pages = pages;
> -       pin_longterm_test_nr_pages = 0;
> +       data->longterm_pages = pages;
> +       data->longterm_nr_pages = 0;
>
> -       while (nr_pages - pin_longterm_test_nr_pages) {
> -               remaining_pages = nr_pages - pin_longterm_test_nr_pages;
> -               addr = args.addr + pin_longterm_test_nr_pages * PAGE_SIZE;
> +       while (nr_pages - data->longterm_nr_pages) {
> +               remaining_pages = nr_pages - data->longterm_nr_pages;
> +               addr = args.addr + data->longterm_nr_pages * PAGE_SIZE;
>
>                 if (fast)
>                         cur_pages = pin_user_pages_fast(addr, remaining_pages,
> @@ -273,11 +276,11 @@ static inline int pin_longterm_test_start(unsigned long arg)
>                         cur_pages = pin_user_pages(addr, remaining_pages,
>                                                    gup_flags, pages);
>                 if (cur_pages < 0) {
> -                       pin_longterm_test_stop();
> +                       pin_longterm_test_stop(data);
>                         ret = cur_pages;
>                         break;
>                 }
> -               pin_longterm_test_nr_pages += cur_pages;
> +               data->longterm_nr_pages += cur_pages;
>                 pages += cur_pages;
>         }
>
> @@ -286,19 +289,20 @@ static inline int pin_longterm_test_start(unsigned long arg)
>         return ret;
>  }
>
> -static inline int pin_longterm_test_read(unsigned long arg)
> +static inline int pin_longterm_test_read(struct gup_test_data *data,
> +               unsigned long arg)
>  {
>         __u64 user_addr;
>         unsigned long i;
>
> -       if (!pin_longterm_test_pages)
> +       if (!data->longterm_pages)
>                 return -EINVAL;
>
>         if (copy_from_user(&user_addr, (void __user *)arg, sizeof(user_addr)))
>                 return -EFAULT;
>
> -       for (i = 0; i < pin_longterm_test_nr_pages; i++) {
> -               void *addr = kmap_local_page(pin_longterm_test_pages[i]);
> +       for (i = 0; i < data->longterm_nr_pages; i++) {
> +               void *addr = kmap_local_page(data->longterm_pages[i]);
>                 unsigned long ret;
>
>                 ret = copy_to_user((void __user *)(unsigned long)user_addr, addr,
> @@ -314,25 +318,26 @@ static inline int pin_longterm_test_read(unsigned long arg)
>  static long pin_longterm_test_ioctl(struct file *filep, unsigned int cmd,
>                                     unsigned long arg)
>  {
> +       struct gup_test_data *data = filep->private_data;
>         int ret = -EINVAL;
>
> -       if (mutex_lock_killable(&pin_longterm_test_mutex))
> +       if (mutex_lock_killable(&data->longterm_mutex))
>                 return -EINTR;
>
>         switch (cmd) {
>         case PIN_LONGTERM_TEST_START:
> -               ret = pin_longterm_test_start(arg);
> +               ret = pin_longterm_test_start(data, arg);
>                 break;
>         case PIN_LONGTERM_TEST_STOP:
> -               pin_longterm_test_stop();
> +               pin_longterm_test_stop(data);
>                 ret = 0;
>                 break;
>         case PIN_LONGTERM_TEST_READ:
> -               ret = pin_longterm_test_read(arg);
> +               ret = pin_longterm_test_read(data, arg);
>                 break;
>         }
>
> -       mutex_unlock(&pin_longterm_test_mutex);
> +       mutex_unlock(&data->longterm_mutex);
>         return ret;
>  }
>
> @@ -371,15 +376,39 @@ static long gup_test_ioctl(struct file *filep, unsigned int cmd,
>         return 0;
>  }
>
> +static int gup_test_open(struct inode *inode, struct file *file)
> +{
> +       struct gup_test_data *data;
> +       int ret;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       ret = nonseekable_open(inode, file);
> +       if (ret) {
> +               kfree(data);
> +               return ret;
> +       }
> +
> +       mutex_init(&data->longterm_mutex);
> +       file->private_data = data;
> +       return 0;
> +}
> +
>  static int gup_test_release(struct inode *inode, struct file *file)
>  {
> -       pin_longterm_test_stop();
> +       struct gup_test_data *data = file->private_data;
> +
> +       pin_longterm_test_stop(data);
> +       kfree(data);
> +       file->private_data = NULL;
>
>         return 0;
>  }
>
>  static const struct file_operations gup_test_fops = {
> -       .open = nonseekable_open,
> +       .open = gup_test_open,
>         .unlocked_ioctl = gup_test_ioctl,
>         .compat_ioctl = compat_ptr_ioctl,
>         .release = gup_test_release,
> --
> 2.43.0
>
>
>
> Can you review+test that change? Thanks!

Sorry for the late reply.
I tested your patch and it works fine for me.
Tested-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>

>
> --
> Cheers,
>
> David

 Thanks,
 Yunhui