linux-kernel - Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user() to copy_folio_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Apr 2023 14:26:12 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     "zhangpeng (AS)" <zhangpeng362@...wei.com>
Cc:     Vishal Moola <vishal.moola@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        willy@...radead.org, sidhartha.kumar@...cle.com,
        muchun.song@...ux.dev, wangkefeng.wang@...wei.com,
        sunnanyong@...wei.com
Subject: Re: [PATCH v5 3/6] userfaultfd: convert copy_huge_page_from_user()
 to copy_folio_from_user()

On 04/08/23 12:43, zhangpeng (AS) wrote:
> On 2023/4/7 10:28, Vishal Moola wrote:
> 
> > On Fri, Mar 31, 2023 at 2:41 AM Peng Zhang <zhangpeng362@...wei.com> wrote:
> > > From: ZhangPeng <zhangpeng362@...wei.com>
> > > 
> > > Replace copy_huge_page_from_user() with copy_folio_from_user().
> > > copy_folio_from_user() does the same as copy_huge_page_from_user(), but
> > > takes in a folio instead of a page. Convert page_kaddr to kaddr in
> > > copy_folio_from_user() to do indenting cleanup.
> > > 
> > > Signed-off-by: ZhangPeng <zhangpeng362@...wei.com>
> > > Reviewed-by: Sidhartha Kumar <sidhartha.kumar@...cle.com>
> > > ---
> > > -                               bool allow_pagefault)
> > > +long copy_folio_from_user(struct folio *dst_folio,
> > > +                          const void __user *usr_src,
> > > +                          bool allow_pagefault)
> > >   {
> > > -       void *page_kaddr;
> > > +       void *kaddr;
> > >          unsigned long i, rc = 0;
> > > -       unsigned long ret_val = pages_per_huge_page * PAGE_SIZE;
> > > +       unsigned int nr_pages = folio_nr_pages(dst_folio);
> > > +       unsigned long ret_val = nr_pages * PAGE_SIZE;
> > >          struct page *subpage;
> > > 
> > > -       for (i = 0; i < pages_per_huge_page; i++) {
> > > -               subpage = nth_page(dst_page, i);
> > > -               page_kaddr = kmap_local_page(subpage);
> > > +       for (i = 0; i < nr_pages; i++) {
> > > +               subpage = folio_page(dst_folio, i);
> > > +               kaddr = kmap_local_page(subpage);
> > >                  if (!allow_pagefault)
> > >                          pagefault_disable();
> > > -               rc = copy_from_user(page_kaddr,
> > > -                               usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > > +               rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> > >                  if (!allow_pagefault)
> > >                          pagefault_enable();
> > > -               kunmap_local(page_kaddr);
> > > +               kunmap_local(kaddr);
> > > 
> > >                  ret_val -= (PAGE_SIZE - rc);
> > >                  if (rc)
> > >                          break;
> > > 
> > > -               flush_dcache_page(subpage);
> > > -
> > >                  cond_resched();
> > >          }
> > > +       flush_dcache_folio(dst_folio);
> > >          return ret_val;
> > >   }
> > Moving the flush_dcache_page() outside the loop to be
> > flush_dcache_folio() changes the behavior of the function.
> > 
> > Initially, if it fails to copy the entire page, the function breaks out
> > of the loop and returns the number of unwritten bytes without
> > flushing the page from the cache. Now if it fails, it will still flush
> > out the page it failed on, as well as any later pages it may not
> > have gotten to yet.
> 
> Agreed. If it fails, could we just not flush the folio?

I believe that should be OK.  If returning an error, nobody should be
depending on any part of the page being present or not in the cache.
-- 
Mike Kravetz

> Like this:
> long copy_folio_from_user(...)
> {
> 	...
> 	for (i = 0; i < nr_pages; i++) {
> 		...
> 		rc = copy_from_user(kaddr, usr_src + i * PAGE_SIZE, PAGE_SIZE);
> 		...
> 		ret_val -= (PAGE_SIZE - rc);
> 		if (rc)
> -                       break;
> +                       return ret_val;
> 		cond_resched();
> 	}
> 	flush_dcache_folio(dst_folio);
> 	return ret_val;
> }
> 
> Thanks for your review.
> 
> Best Regards,
> Peng
>