Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 26 Sep 2022 10:57:59 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Florian Westphal <fw@strlen.de>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org, vbabka@suse.cz,
        akpm@linux-foundation.org, urezki@gmail.com,
        netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
        Martin Zaharinov <micron10@gmail.com>
Subject: Re: [PATCH mm] mm: fix BUG with kvzalloc+GFP_ATOMIC
Message-ID: <YzFplwSxwwsLpzzX@dhcp22.suse.cz>
References: <20220923103858.26729-1-fw@strlen.de>
 <Yy20toVrIktiMSvH@dhcp22.suse.cz>
 <20220923133512.GE22541@breakpoint.cc>
 <YzFZf0Onm6/UH7/I@dhcp22.suse.cz>
 <20220926075639.GA908@breakpoint.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220926075639.GA908@breakpoint.cc>
Precedence: bulk

On Mon 26-09-22 09:56:39, Florian Westphal wrote:
> Michal Hocko <mhocko@suse.com> wrote:
> > > kvzalloc(GFP_ATOMIC) was perfectly fine, is this illegal again?
> > 
> > kvmalloc has never really supported GFP_ATOMIC semantic.
> 
> It did, you added it:
> ce91f6ee5b3b ("mm: kvmalloc does not fallback to vmalloc for incompatible gfp flags")

Yes, I am very well aware of this commit and I have to say I wasn't
really supper happy about it TBH. Linus has argued this will result in a
saner code and in some cases this was true.

Later on we really had to add support some extensions beyond
GFP_KERNEL. Your change would break those GFP_NOFAIL and NOFS
usecases. GFP_NOWAIT and GFP_ATOMIC are explicitly documented as
unsupported. One we can do to continue in ce91f6ee5b3b sense is to
do this instead

diff --git a/mm/util.c b/mm/util.c
index 0837570c9225..a27b3fce1f0e 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -618,6 +618,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
 	 */
 	if (ret || size <= PAGE_SIZE)
 		return ret;
+	
+	/* non-sleeping allocations are not supported by vmalloc */
+	if (!gfpflags_allow_blocking(flags))
+		return NULL;
 
 	/* Don't even allow crazy sizes */
 	if (unlikely(size > INT_MAX)) {

A better option to me seems to be reworking the rhashtable_insert_rehash
to not rely on an atomic allocation. I am not familiar with that code
but it seems to me that the only reason this allocation mode is used is
due to rcu locking around rhashtable_try_insert. Is there any reason we
cannot drop the rcu lock, allocate with the full GFP_KERNEL allocation
power and retry with the pre allocated object? rhashtable_insert_slow is
already doing that to implement its never fail semantic.
-- 
Michal Hocko
SUSE Labs