From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from zeniv.linux.org.uk (zeniv.linux.org.uk [62.89.141.173]) (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 7055E137930 for ; Mon, 9 Dec 2024 20:27:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=62.89.141.173 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733776061; cv=none; b=FNGXTKS/Wg3jPgfTzychBWAY4kmAHFLXVNZZQSCjY5iC3c1L5UYcdlTyB9xSMIVOBAbIDUDdKRGzilOm1uBO4fEuycYL442B6e6QK3MdGRsgCTrEIgwgn3mnUOV1GEUkqdivztmvyZvRIoeWldBlI05MWg0nTcnMnS2fA5L3BNU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733776061; c=relaxed/simple; bh=PifocyfzOg7jQs4ZEJjXsJp38rDuKrS8V8TYCDhvk0E=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sXEglgH74rBodilBI1JUBFB+AenOKUjJa/u9IurTRxfeMDcg8t1J6j11YK+oNOZuCwdLeFyFgUvVAO6Ie+53bxYT021O7rjuB66qyZRfViMLnGWRd8+P6dkE5E/ttrx71cki2BmfERNf/CQ7+L1eUM19ZNP1IBDOldjrxsFA0ng= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zeniv.linux.org.uk; spf=none smtp.mailfrom=ftp.linux.org.uk; dkim=pass (2048-bit key) header.d=linux.org.uk header.i=@linux.org.uk header.b=bDvXITxW; arc=none smtp.client-ip=62.89.141.173 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=zeniv.linux.org.uk Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ftp.linux.org.uk Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linux.org.uk header.i=@linux.org.uk header.b="bDvXITxW" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=linux.org.uk; s=zeniv-20220401; h=Sender:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=AVm6hZuE0NcAYBQ/uiyX0TZXpt5kwQI7d8ka7/lH0NY=; b=bDvXITxWL0gEuyRGQE8eTw0Ws4 OIZ4Mj29XB6WEN2z88w5KHdsWD2poQKtbc5TvPWQ1xRMqnnaDas5wRKKC3bwSqc4fmpos/zzKkA7k gA+6ZwQJTIntV1b1pbcKpxKkoy50gB65FNVgnbvE9mH4vn12FbMimgvIatbLdJCfLFnzPD0nQIC6E h/gkQYE7M9ycXrd576egiBq/AICI6ZNo2aJ6tBFz0Zmdy0vJnoOAb0PzDS7f5ENUcncRPKbvICvQy ZpwAyQbn7A3Ub3GY9fXuV3DgU0IBKL4mIYFII3TuckqBVxHOdIWLzzCXfc6xbnltnmhdr6+tvNKbd VNGVerKw==; Received: from viro by zeniv.linux.org.uk with local (Exim 4.98 #2 (Red Hat Linux)) id 1tKkLw-00000006fB5-2F9w; Mon, 09 Dec 2024 20:27:36 +0000 Date: Mon, 9 Dec 2024 20:27:36 +0000 From: Al Viro To: Linus Torvalds Cc: linux-fsdevel@vger.kernel.org Subject: Re: [PATCH][RFC] make take_dentry_name_snapshot() lockless Message-ID: <20241209202736.GZ3387508@ZenIV> References: <20241209035251.GV3387508@ZenIV> Precedence: bulk X-Mailing-List: linux-fsdevel@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: Sender: Al Viro On Mon, Dec 09, 2024 at 11:06:48AM -0800, Linus Torvalds wrote: > On Sun, 8 Dec 2024 at 19:52, Al Viro wrote: > > > > + struct external_name *p; > > + p = container_of(name->name.name, struct external_name, name[0]); > > + // get a valid reference > > + if (unlikely(!atomic_inc_not_zero(&p->u.count))) > > + goto retry; > > Oh - this is very much *not* safe. > > The other comment I had was really about "that's bad for performance". > But this is actually actively buggy. > > If the external name ref has gone down to zero, we can *not* do that > > atomic_inc_not_zero(..) > > thing any more, because the recount is in a union with the rcu_head > for delaying the free. D'oh. Right you are; missed it... > In other words: the *name* will exist for the duration of the > rcu_read_lock() we hold, but that "p->u.count" will not. When the > refcount has gone to zero, the refcount is no longer usable. > > IOW, you may be happily incrementing what is now a RCU list head > rather than a count. > > So NAK. This cannot work. > > It's probably easily fixable by just not using a union in struct > external_name, and just having separate fields for the refcount and > the rcu_head, but in the current state your patch is fundamentally and > dangerously buggy. Agreed. And yes, separating the fields (and slapping a comment explaining why they can not be combined) would be the easiest solution - any attempts to be clever here would be too brittle for no good reason.