linux-kernel - Re: [PATCH 1/4] mm/filemap: return early if failed to allocate memory for split

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: <ZfnBTnXTIGck4lVR@casper.infradead.org>
Date: Tue, 19 Mar 2024 16:46:06 +0000
From: Matthew Wilcox <willy@...radead.org>
To: Kairui Song <kasong@...cent.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] mm/filemap: return early if failed to allocate
 memory for split

On Tue, Mar 19, 2024 at 05:27:30PM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@...cent.com>
> 
> xas_split_alloc could fail with NOMEM, and in such case, it should abort
> early instead of keep going and fail the xas_split below.
> 
> Signed-off-by: Kairui Song <kasong@...cent.com>

The usual way of programming with an xa_state is to not do any error
handling; the xas_ functions check for error and become no-ops, so we
only have to check at the end.

I think this case is worth making an exception for because we avoid
acquiring/releasing the lock.  Not that we should be getting ENOMEM
very often; we only allocate about 8 nodes usually and we get 7
nodes/page.

Acked-by: Matthew Wilcox (Oracle) <willy@...radead.org>

>  mm/filemap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 7437b2bd75c1..f07ea0b97698 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -880,9 +880,12 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>  		unsigned int order = xa_get_order(xas.xa, xas.xa_index);
>  		void *entry, *old = NULL;
>  
> -		if (order > folio_order(folio))
> +		if (order > folio_order(folio)) {
>  			xas_split_alloc(&xas, xa_load(xas.xa, xas.xa_index),
>  					order, gfp);
> +			if (xas_error(&xas))
> +				goto error;
> +		}
>  		xas_lock_irq(&xas);
>  		xas_for_each_conflict(&xas, entry) {
>  			old = entry;
> -- 
> 2.43.0
>