From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DC1B33914FD for ; Thu, 30 Jul 2026 07:43:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785397389; cv=none; b=PtfiAdU+6HUbrVPNCXwpr5Tsdu4okFjRfx3AU1SQN2edWTt0fE1dENvUnxeCPwesomNYrM2LFFyjFxDHhskmK8Rgfiye2tkHEMQueF7WPpsxys919UVTJ9Z5uCAuNmKc2HfDJh1Z1tYp4NP6b/tXUyaiOUJgNgiUUqVPdQsGbvY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785397389; c=relaxed/simple; bh=CJj8GwI0ceNNDZqeYPJrGR1L4pu6yT0saAG+5cebqK0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=k3Kxe61hjMTVHfpwP7pQk4LRVdWdooyY1eWhqu/uSxdZJRnfmc74ZEGltJXBe9QhN0wEXyrC7XHIBjUewp4XtZxqt8mIWzV8o50od1Kvlbj6ApbwPn6bvYoG4J5HGsFso3uyz6bx4CkDt8wmf6QZNcdYSNqyDqcvpdi/k/6lzaI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=S7kdUwet; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="S7kdUwet" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B641D1F000E9; Thu, 30 Jul 2026 07:43:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1785397387; bh=pDaJ/+nIgmvu6q0pTtI9cFKoi+A+/DB+ehPxk3gGQPo=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=S7kdUwetvww4fJ0G3Bdg5GUTedAKkvPoQaJewy0YPJGJOVFkEvvV2daTtrz53T4AW zwvZgYrYcX/R1TlstajiEiNBWIB4D5mRK9LDjQFBtw6WryYG0JemrTKfGZ9WLyqEID Q93JjVAm39Y5DVbBOmHOO+Pn9JcG88/aIeT0A8Z10B3Src+vfsCNHQbXL2lmH9FP9t Cg+73GjuYaswY1fN8JtyuqzzCCb78gWn4RPkfMBjWrtfdre2PYXYPzZ6y41aJLnVSq dDnmvsn/8K5GDJ+eR2HkB4ZpTkCXGaJq3FrSxCJkg21umtCsrGGKtiInJph2SGD9Ac a+kP0sF0xrX4Q== Date: Thu, 30 Jul 2026 08:42:49 +0100 From: "Lorenzo Stoakes (ARM)" To: Avi Weiss Cc: Andrew Morton , David Hildenbrand , "Liam R. Howlett" , Vlastimil Babka , Mike Rapoport , Suren Baghdasaryan , Michal Hocko , Soheil Hassas Yeganeh , Arjun Roy , Eric Dumazet , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: return -ENOMEM for page-table allocation failure in insert_pages() Message-ID: References: <20260730071323.964321-1-thnkslprpt@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260730071323.964321-1-thnkslprpt@gmail.com> On Thu, Jul 30, 2026 at 10:13:23AM +0300, Avi Weiss wrote: > walk_to_pmd() returns NULL only when p4d_alloc(), pud_alloc(), or > pmd_alloc() fails. These are page-table allocation failures, but > insert_pages() currently reports them as -EFAULT. > > Return -ENOMEM instead, consistent with the subsequent pte_alloc() > failure and with the single-page insert_page() path, which reports > failure of the same page-table allocation chain as -ENOMEM. > > Address and range validation failures in vm_insert_pages() continue to > return -EFAULT. Keep the later -EFAULT return for > pte_offset_map_lock(), which is not an allocation failure. > > Fixes: 8cd3984d81d5 ("mm/memory.c: add vm_insert_pages()") Hmm :) This isn't really a fix. Anybody relying on this returning -ENOMEM vs. -EFAULT here is in a state of sin anyway (unless you can point to specific users who are broken). Drop the tag. > Signed-off-by: Avi Weiss This is correct, walk > --- > mm/memory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memory.c b/mm/memory.c > index ff338c2abe92..d8eddca8e251 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -2436,7 +2436,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr, > unsigned long pages_to_write_in_pmd; > int ret; > more: > - ret = -EFAULT; > + ret = -ENOMEM; > pmd = walk_to_pmd(mm, addr); walk_to_pmd() is horribly named, it's allocating... populate_to_pmd() would be better can you rename it? > if (!pmd) > goto out; Looking down: pages_to_write_in_pmd = min_t(unsigned long, remaining_pages_total, PTRS_PER_PTE - pte_index(addr)); /* Allocate the PTE if necessary; takes PMD lock once only. */ ret = -ENOMEM; <------------------------------ set it again? if (pte_alloc(mm, pmd)) goto out; The way this function is written is horrible in general, I hate 'preset default return value' as a pattern. So could you instead change it so the ret is set at the point of error, and while you're at it rename ret to err, e.g.: int err = 0; ... pmd = populate_to_pmd(mm, addr); if (!pmd) { err = -ENOMEM; goto out; } etc. Also ignoring pte_alloc()'s error code (which will be -ENOMEM anyway) and setting manually is stupid further down so: - ret = -ENOMEM; - if (pte_alloc(mm, pmd)) - goto out; + err = pte_alloc(mm, pmd); + if (err) + goto out; Obviously: for (pte = start_pte; pte_idx < batch_size; ++pte, ++pte_idx) { int err = insert_page_in_batch_locked(vma, pte, addr, pages[curr_page_idx], prot); -> for (pte = start_pte; pte_idx < batch_size; ++pte, ++pte_idx) { err = ... Remove horrible ret = err and ret = 0 assignment later. All this would improve it a lot thanks! :) > -- > 2.43.0 > Cheers, Lorenzo