| 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: <cb48d187-6fa5-41f5-9fc9-3f424f105785@lucifer.local>
Date: Tue, 11 Mar 2025 12:17:40 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
"Liam R. Howlett" <howlett@...il.com>,
David Hildenbrand <david@...hat.com>,
Shakeel Butt <shakeel.butt@...ux.dev>,
Vlastimil Babka <vbabka@...e.cz>, kernel-team@...a.com,
linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 5/9] mm/madvise: define and use madvise_behavior struct
for madvise_do_behavior()
On Mon, Mar 10, 2025 at 10:23:14AM -0700, SeongJae Park wrote:
> To implement batched tlb flushes for MADV_DONTNEED[_LOCKED] and
> MADV_FREE, an mmu_gather object in addition to the behavior integer need
> to be passed to the internal logics. Using a struct can make it easy
> without increasing the number of parameters of all code paths towards
> the internal logic. Define a struct for the purpose and use it on the
> code path that starts from madvise_do_behavior() and ends on
> madvise_dontneed_free().
Oh a helper struct! I like these!
Nitty but...
I wonder if we should just add the the mmu_gather field immediately even if
it isn't used yet?
Also I feel like this patch and 6 should be swapped around, as you are
laying the groundwork here for patch 7 but then doing something unrelated
in 6, unless I'm missing something.
Also maybe add a bit in commit msg about changing the madvise_walk_vmas()
visitor type signature (I wonder if that'd be better as a typedef tbh?)
However, this change looks fine aside from nits (and you know, helper
struct and I'm sold obviously ;) so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Signed-off-by: SeongJae Park <sj@...nel.org>
> ---
> mm/madvise.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 469c25690a0e..ba2a78795207 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -890,11 +890,16 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
> return true;
> }
>
> +struct madvise_behavior {
> + int behavior;
> +};
> +
> static long madvise_dontneed_free(struct vm_area_struct *vma,
> struct vm_area_struct **prev,
> unsigned long start, unsigned long end,
> - int behavior)
> + struct madvise_behavior *madv_behavior)
Nitty, but not sure about the need for 'madv_' here. I think keeping this as
'behavior' is fine, as the type is very clear.
> {
> + int behavior = madv_behavior->behavior;
> struct mm_struct *mm = vma->vm_mm;
>
> *prev = vma;
> @@ -1249,8 +1254,10 @@ static long madvise_guard_remove(struct vm_area_struct *vma,
> static int madvise_vma_behavior(struct vm_area_struct *vma,
> struct vm_area_struct **prev,
> unsigned long start, unsigned long end,
> - unsigned long behavior)
> + void *behavior_arg)
> {
> + struct madvise_behavior *arg = behavior_arg;
> + int behavior = arg->behavior;
> int error;
> struct anon_vma_name *anon_name;
> unsigned long new_flags = vma->vm_flags;
> @@ -1270,7 +1277,7 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> case MADV_FREE:
> case MADV_DONTNEED:
> case MADV_DONTNEED_LOCKED:
> - return madvise_dontneed_free(vma, prev, start, end, behavior);
> + return madvise_dontneed_free(vma, prev, start, end, arg);
> case MADV_NORMAL:
> new_flags = new_flags & ~VM_RAND_READ & ~VM_SEQ_READ;
> break;
> @@ -1487,10 +1494,10 @@ static bool process_madvise_remote_valid(int behavior)
> */
> static
> int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> - unsigned long end, unsigned long arg,
> + unsigned long end, void *arg,
> int (*visit)(struct vm_area_struct *vma,
> struct vm_area_struct **prev, unsigned long start,
> - unsigned long end, unsigned long arg))
> + unsigned long end, void *arg))
> {
> struct vm_area_struct *vma;
> struct vm_area_struct *prev;
> @@ -1548,7 +1555,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> static int madvise_vma_anon_name(struct vm_area_struct *vma,
> struct vm_area_struct **prev,
> unsigned long start, unsigned long end,
> - unsigned long anon_name)
> + void *anon_name)
> {
> int error;
>
> @@ -1557,7 +1564,7 @@ static int madvise_vma_anon_name(struct vm_area_struct *vma,
> return -EBADF;
>
> error = madvise_update_vma(vma, prev, start, end, vma->vm_flags,
> - (struct anon_vma_name *)anon_name);
> + anon_name);
>
> /*
> * madvise() returns EAGAIN if kernel resources, such as
> @@ -1589,7 +1596,7 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> if (end == start)
> return 0;
>
> - return madvise_walk_vmas(mm, start, end, (unsigned long)anon_name,
> + return madvise_walk_vmas(mm, start, end, anon_name,
> madvise_vma_anon_name);
> }
> #endif /* CONFIG_ANON_VMA_NAME */
> @@ -1673,8 +1680,10 @@ static bool is_madvise_populate(int behavior)
> }
>
> static int madvise_do_behavior(struct mm_struct *mm,
> - unsigned long start, size_t len_in, int behavior)
> + unsigned long start, size_t len_in,
> + struct madvise_behavior *madv_behavior)
> {
> + int behavior = madv_behavior->behavior;
> struct blk_plug plug;
> unsigned long end;
> int error;
> @@ -1688,7 +1697,7 @@ static int madvise_do_behavior(struct mm_struct *mm,
> if (is_madvise_populate(behavior))
> error = madvise_populate(mm, start, end, behavior);
> else
> - error = madvise_walk_vmas(mm, start, end, behavior,
> + error = madvise_walk_vmas(mm, start, end, madv_behavior,
> madvise_vma_behavior);
> blk_finish_plug(&plug);
> return error;
> @@ -1769,13 +1778,14 @@ static int madvise_do_behavior(struct mm_struct *mm,
> int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> {
> int error;
> + struct madvise_behavior madv_behavior = {.behavior = behavior};
>
> if (madvise_should_skip(start, len_in, behavior, &error))
> return error;
> error = madvise_lock(mm, behavior);
> if (error)
> return error;
> - error = madvise_do_behavior(mm, start, len_in, behavior);
> + error = madvise_do_behavior(mm, start, len_in, &madv_behavior);
> madvise_unlock(mm, behavior);
>
> return error;
> @@ -1792,6 +1802,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> {
> ssize_t ret = 0;
> size_t total_len;
> + struct madvise_behavior madv_behavior = {.behavior = behavior};
>
> total_len = iov_iter_count(iter);
>
> @@ -1807,7 +1818,8 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
> if (madvise_should_skip(start, len_in, behavior, &error))
> ret = error;
> else
> - ret = madvise_do_behavior(mm, start, len_in, behavior);
> + ret = madvise_do_behavior(mm, start, len_in,
> + &madv_behavior);
> /*
> * An madvise operation is attempting to restart the syscall,
> * but we cannot proceed as it would not be correct to repeat
> --
> 2.39.5
Powered by blists - more mailing lists