| 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: <A88CC419-113E-41DA-83F0-016BFE50B660@nvidia.com>
Date: Thu, 19 Jun 2025 22:13:19 -0400
From: Zi Yan <ziy@...dia.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.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>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH 3/5] mm/madvise: thread VMA range state through
madvise_behavior
On 19 Jun 2025, at 21:54, 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?
>
> <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.
>
>
>
>> @@ -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?
>
>
> <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.
Based on git history, it seems that no one paid attention to
madvise_inject_error() and the [start, start + len_in] has never been
changed since it was added back from 2009.
OK, it seems that Kirill (cc'd) moved start = untagged_addr(start); from
before madvise_inject_error() to after it at commit 428e106ae1ad
("mm: Introduce untagged_addr_remote()"). It changed code behavior.
So memory_failure should get the same range as others, meaning
madvise_do_behavior() can just take struct madvise_behavior
and the range can be set at the call sites.
--
Best Regards,
Yan, Zi
Powered by blists - more mailing lists