Re: [PATCH] fbdev: defio: fix the pagelist corruption

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Chuansheng,

On Thu, Mar 17, 2022 at 7:17 AM Chuansheng Liu <chuansheng.liu@intel.com> wrote:
> Easily hit the below list corruption:
> ==
> list_add corruption. prev->next should be next (ffffffffc0ceb090), but
> was ffffec604507edc8. (prev=ffffec604507edc8).
> WARNING: CPU: 65 PID: 3959 at lib/list_debug.c:26
> __list_add_valid+0x53/0x80
> CPU: 65 PID: 3959 Comm: fbdev Tainted: G     U
> RIP: 0010:__list_add_valid+0x53/0x80
> Call Trace:
>  <TASK>
>  fb_deferred_io_mkwrite+0xea/0x150
>  do_page_mkwrite+0x57/0xc0
>  do_wp_page+0x278/0x2f0
>  __handle_mm_fault+0xdc2/0x1590
>  handle_mm_fault+0xdd/0x2c0
>  do_user_addr_fault+0x1d3/0x650
>  exc_page_fault+0x77/0x180
>  ? asm_exc_page_fault+0x8/0x30
>  asm_exc_page_fault+0x1e/0x30
> RIP: 0033:0x7fd98fc8fad1
> ==
>
> Figure out the race happens when one process is adding &page->lru into
> the pagelist tail in fb_deferred_io_mkwrite(), another process is
> re-initializing the same &page->lru in fb_deferred_io_fault(), which is
> not protected by the lock.
>
> This fix is to init all the page lists one time during initialization,
> it not only fixes the list corruption, but also avoids INIT_LIST_HEAD()
> redundantly.
>
> Fixes: 105a940416fc ("fbdev/defio: Early-out if page is already
> enlisted")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -220,6 +219,8 @@ static void fb_deferred_io_work(struct work_struct *work)
>  void fb_deferred_io_init(struct fb_info *info)
>  {
>         struct fb_deferred_io *fbdefio = info->fbdefio;
> +       struct page *page;
> +       int i;

unsigned int i;

>         BUG_ON(!fbdefio);
>         mutex_init(&fbdefio->lock);
> @@ -227,6 +228,12 @@ void fb_deferred_io_init(struct fb_info *info)
>         INIT_LIST_HEAD(&fbdefio->pagelist);
>         if (fbdefio->delay == 0) /* set a default of 1 s */
>                 fbdefio->delay = HZ;
> +
> +       /* initialize all the page lists one time */
> +       for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
> +               page = fb_deferred_io_page(info, i);
> +               INIT_LIST_HEAD(&page->lru);
> +       }
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_init);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds