| 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
| ||
|
Message-ID: <2718d196-dd11-404e-906a-962629923be6@lucifer.local>
Date: Fri, 20 Jun 2025 06:17:45 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Zi Yan <ziy@...dia.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...hat.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
Dev Jain <dev.jain@....com>, Barry Song <baohua@...nel.org>,
Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Lance Yang <ioworker0@...il.com>, SeongJae Park <sj@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [PATCH 3/5] mm/madvise: thread VMA range state through
madvise_behavior
On Thu, Jun 19, 2025 at 09:54:11PM -0400, Zi Yan wrote:
> On 19 Jun 2025, at 16:26, Lorenzo Stoakes wrote:
>
> > Rather than updating start and a confusing local parameter 'tmp' in
> > madvise_walk_vmas(), instead store the current range being operated upon in
> > the struct madvise_behavior helper object in a range pair and use this
> > consistently in all operations.
> >
> > This makes it clearer what is going on and opens the door to further
> > cleanup now we store state regarding what is currently being operated upon
> > here.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> > mm/madvise.c | 101 ++++++++++++++++++++++++++++-----------------------
> > 1 file changed, 55 insertions(+), 46 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 47485653c2a1..6faa38b92111 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -58,17 +58,26 @@ enum madvise_lock_mode {
> > MADVISE_VMA_READ_LOCK,
> > };
> >
> > +struct madvise_behavior_range {
> > + unsigned long start, end;
> > +};
> > +
>
> Declare members separately?
Can do, but this is one of those subject things where everyone has different
views, if I did it the other way no doubt somebody else would comment about
declaring together :P
I think as a range here it's not a big deal unless you feel strongly about it?
>
> <snip>
>
> > @@ -1425,10 +1437,11 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> > /*
> > * Error injection support for memory error handling.
> > */
> > -static int madvise_inject_error(unsigned long start, unsigned long end,
> > - struct madvise_behavior *madv_behavior)
> > +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
> > {
> > unsigned long size;
> > + unsigned long start = madv_behavior->range.start;
> > + unsigned long end = madv_behavior->range.end;
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EPERM;
> > @@ -1482,8 +1495,7 @@ static bool is_memory_failure(struct madvise_behavior *madv_behavior)
> >
> > #else
> >
> > -static int madvise_inject_error(unsigned long start, unsigned long end,
> > - struct madvise_behavior *madv_behavior)
> > +static int madvise_inject_error(struct madvise_behavior *madv_behavior)
> > {
> > return 0;
> > }
>
> OK, now I get why you pass struct madvise_behavior to madvise_inject_error()
> in Patch 2. The changes make sense to me now. Maybe delay that conversation
> in this one.
I think it's valuable there because otherwise all the function invocations were
inconsistent, but after 2/5 become completely consistent. I mention this in the
commit message and I think it's valuable so you're not doing:
if (foo)
bar(x, y, z)
if (blah)
baz(y, x, z)
etc.
When you quickly read through it's easy to get confused/lost as to what's going
on, whereas if they all have the same signatures it's very clear you're
offloading the heavy lifting to each function.
>
>
>
> > @@ -1565,20 +1577,20 @@ static bool process_madvise_remote_valid(int behavior)
> > * If a VMA read lock could not be acquired, we return NULL and expect caller to
> > * fallback to mmap lock behaviour.
> > */
> > -static struct vm_area_struct *try_vma_read_lock(struct mm_struct *mm,
> > - struct madvise_behavior *madv_behavior,
> > - unsigned long start, unsigned long end)
> > +static
> > +struct vm_area_struct *try_vma_read_lock(struct madvise_behavior *madv_behavior)
> > {
> > + struct mm_struct *mm = madv_behavior->mm;
>
> Is the struct mm_struct removal missed in Patch 2?
Yeah, I will go back and put it in on respin.
>
>
> <snip>
>
> > @@ -1846,22 +1854,23 @@ static int madvise_do_behavior(unsigned long start, size_t len_in,
> > struct madvise_behavior *madv_behavior)
> > {
> > struct blk_plug plug;
> > - unsigned long end;
> > int error;
> > + struct madvise_behavior_range *range = &madv_behavior->range;
> >
> > if (is_memory_failure(madv_behavior)) {
> > - end = start + len_in;
> > - return madvise_inject_error(start, end, madv_behavior);
> > + range->start = start;
> > + range->end = start + len_in;
> > + return madvise_inject_error(madv_behavior);
> > }
> >
> > - start = get_untagged_addr(madv_behavior->mm, start);
> > - end = start + PAGE_ALIGN(len_in);
> > + range->start = get_untagged_addr(madv_behavior->mm, start);
> > + range->end = range->start + PAGE_ALIGN(len_in);
> >
> > blk_start_plug(&plug);
> > if (is_madvise_populate(madv_behavior))
> > - error = madvise_populate(start, end, madv_behavior);
> > + error = madvise_populate(madv_behavior);
> > else
> > - error = madvise_walk_vmas(start, end, madv_behavior);
> > + error = madvise_walk_vmas(madv_behavior);
> > blk_finish_plug(&plug);
> > return error;
> > }
>
> We almost can pass just struct madvise_behavior to madvise_do_behavior().
> I wonder why memory_failure behaves differently.
There's complexity around the start, end stuff (Barry bumped into some of this)
and I don't want to mess with that in this series. This series is meant to have
no functional changes.
>
> --
> Best Regards,
> Yan, Zi
Powered by blists - more mailing lists