Received-SPF: pass (google.com: domain of linux-kernel+bounces-29650-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 18 Jan 2024 02:12:48 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>, akpm@linux-foundation.org,
	brauner@kernel.org, jack@suse.cz, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs: improve dump_mapping() robustness
Message-ID: <ZaiJIIrzUR7qPkjC@casper.infradead.org>
References: <937ab1f87328516821d39be672b6bc18861d9d3e.1705391420.git.baolin.wang@linux.alibaba.com>
 <20240118013857.GO1674809@ZenIV>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240118013857.GO1674809@ZenIV>

On Thu, Jan 18, 2024 at 01:38:57AM +0000, Al Viro wrote:
> On Tue, Jan 16, 2024 at 03:53:35PM +0800, Baolin Wang wrote:
> 
> > With checking the 'dentry.parent' and 'dentry.d_name.name' used by
> > dentry_name(), I can see dump_mapping() will output the invalid dentry
> > instead of crashing the system when this issue is reproduced again.
> 
> >  	dentry_ptr = container_of(dentry_first, struct dentry, d_u.d_alias);
> > -	if (get_kernel_nofault(dentry, dentry_ptr)) {
> > +	if (get_kernel_nofault(dentry, dentry_ptr) ||
> > +	    !dentry.d_parent || !dentry.d_name.name) {
> >  		pr_warn("aops:%ps ino:%lx invalid dentry:%px\n",
> >  				a_ops, ino, dentry_ptr);
> >  		return;
> 
> That's nowhere near enough.  Your ->d_name.name can bloody well be pointing
> to an external name that gets freed right under you.  Legitimately so.
> 
> Think what happens if dentry has a long name (longer than would fit into
> the embedded array) and gets renamed name just after you copy it into
> a local variable.  Old name will get freed.  Yes, freeing is RCU-delayed,
> but I don't see anything that would prevent your thread losing CPU
> and not getting it back until after the sucker's been freed.

Agreed that it's not enough.  It does usually work, and it's very
helpful when it does.  We've had it since 2018 (1c6fb1d89e73) and we've
been gradually making it more robust over time.  Part of my reason for
splitting dump_mapping() out of dump_page() was so that it would get
more review from people who understand the fs side of things ... and
that seems to have worked.

Can I trouble you to suggest a more robust solution?  Bear in mind that
dump_page() does get called on pointers which turn out not to even be
pointers to struct page so this is all very much best-effort, and giving
up and printing 'this is not a dentry" is always an option.