linux-kernel - Re: [PATCH 3/3] mm/memory-failure: inline add_to_kill_anon

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]

Message-ID: <2aa34178-7002-467e-bbdc-c1f7b22dd310@lucifer.local>
Date: Fri, 18 Apr 2025 10:18:25 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Ye Liu <ye.liu@...ux.dev>
Cc: akpm@...ux-foundation.org, linmiaohe@...wei.com, nao.horiguchi@...il.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Ye Liu <liuye@...inos.cn>, David Hildenbrand <david@...hat.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Harry Yoo <harry.yoo@...cle.com>,
        Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH 3/3] mm/memory-failure: inline add_to_kill_anon_file()
 logic at call sites

On Fri, Apr 18, 2025 at 03:52:26PM +0800, Ye Liu wrote:
> From: Ye Liu <liuye@...inos.cn>
>
> The add_to_kill_anon_file() helper function only checked for -EFAULT
> return values before calling __add_to_kill(). Removes the unnecessary
> wrapper and moves the addr == -EFAULT checks directly into
> collect_procs_anon() and collect_procs_file().
>
> This ensures that error handling is performed close to where the address
> is derived (via page_mapped_in_vma() or page_address_in_vma()), rather
> than being obscured inside a helper function.

Yeah, no.

You're duplicating logic here (the old classic - what if we change logic in one
call site but not the other etc.) and removing self-documentation of what's
going on here.

This is actually a net negative imo. Please drop this patch from the series +
send a v2 for the 2 good patches (with correct cc's ;)

Thanks!

>
> No functional changes are introduced.
>
> Signed-off-by: Ye Liu <liuye@...inos.cn>
> ---
>  mm/memory-failure.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index b91a33fb6c69..ec0041c95b27 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -486,15 +486,6 @@ static void __add_to_kill(struct task_struct *tsk, const struct page *p,
>  	list_add_tail(&tk->nd, to_kill);
>  }
>
> -static void add_to_kill_anon_file(struct task_struct *tsk, const struct page *p,
> -		struct vm_area_struct *vma, struct list_head *to_kill,
> -		unsigned long addr)
> -{
> -	if (addr == -EFAULT)
> -		return;
> -	__add_to_kill(tsk, p, vma, to_kill, addr);
> -}
> -
>  #ifdef CONFIG_KSM
>  static bool task_in_to_kill_list(struct list_head *to_kill,
>  				 struct task_struct *tsk)
> @@ -634,7 +625,8 @@ static void collect_procs_anon(const struct folio *folio,
>  			if (vma->vm_mm != t->mm)
>  				continue;
>  			addr = page_mapped_in_vma(page, vma);
> -			add_to_kill_anon_file(t, page, vma, to_kill, addr);
> +			if (addr != -EFAULT)
> +				__add_to_kill(t, page, vma, to_kill, addr);
>  		}
>  	}
>  	rcu_read_unlock();
> @@ -674,7 +666,8 @@ static void collect_procs_file(const struct folio *folio,
>  			if (vma->vm_mm != t->mm)
>  				continue;
>  			addr = page_address_in_vma(folio, page, vma);
> -			add_to_kill_anon_file(t, page, vma, to_kill, addr);
> +			if (addr != -EFAULT)
> +				__add_to_kill(t, page, vma, to_kill, addr);
>  		}
>  	}
>  	rcu_read_unlock();
> --
> 2.25.1
>