linux-kernel - Re: [v2 PATCH] mm: gup: do not call try_grab

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHbLzkqtDxcqDH-ujiqsY6tp49vkyU8RQTrmjT6oA4Cc-cdsxw@mail.gmail.com>
Date: Thu, 27 Jun 2024 16:39:11 -0700
From: Yang Shi <shy828301@...il.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Peter Xu <peterx@...hat.com>, Yang Shi <yang@...amperecomputing.com>, yangge1116@....com, 
	david@...hat.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [v2 PATCH] mm: gup: do not call try_grab_folio() in slow path

On Thu, Jun 27, 2024 at 4:32 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
>
> On Thu, 27 Jun 2024 19:19:40 -0400 Peter Xu <peterx@...hat.com> wrote:
>
> > Yang,
> >
> > On Thu, Jun 27, 2024 at 03:14:13PM -0700, Yang Shi wrote:
> > > The try_grab_folio() is supposed to be used in fast path and it elevates
> > > folio refcount by using add ref unless zero.  We are guaranteed to have
> > > at least one stable reference in slow path, so the simple atomic add
> > > could be used.  The performance difference should be trivial, but the
> > > misuse may be confusing and misleading.
> >
> > This first paragraph is IMHO misleading itself..
> >
> > I think we should mention upfront the important bit, on the user impact.
> >
> > Here IMO the user impact should be: Linux may fail longterm pin in some
> > releavnt paths when applied over CMA reserved blocks.  And if to extend a
> > bit, that include not only slow-gup but also the new memfd pinning, because
> > both of them used try_grab_folio() which used to be only for fast-gup.
>
> It's still unclear how users will be affected.  What do the *users*
> see?  If it's a slight slowdown, do we need to backport this at all?

I think Peter meant the warning reported by yangge?

Peter also mentioned the patch subject is misleading. I agree. So
how's about "mm: gup: stop abusing try_grab_folio()"?

>
> >
> > The patch itself looks mostly ok to me.
> >
> > There's still some "cleanup" part mangled together, e.g., the real meat
> > should be avoiding the folio_is_longterm_pinnable() check in relevant
> > paths.  The rest (e.g. switch slow-gup / memfd pin to use folio_ref_add()
> > not try_get_folio(), and renames) could be good cleanups.
> >
> > So a smaller fix might be doable, but again I don't have a strong opinion
> > here.
>
> The smaller the better for backporting, of course.

I view the fix to the warning as just by-product of the clean up. The
whole patch is naturally integral IMHO. We can generate a smaller fix
if it is too hard to backport. However, it should be ok since we just
need to backport to 6.6+.

>