From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 B527E18E050; Fri, 2 May 2025 07:33:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746171201; cv=none; b=lHupkyrVF3SuwFvd69fbkPP33z/gYDkjX+HyuGNxbnoupUmWABMVq1u+n1pV07U1DlU5ln8Ajuk7lk2K+WdbTUuv9r/sHUrAX+E/xr3/j0IcRcrNVpkEeyHBjFJEPAjWpJ5dAmaokS9i72+/UsgeukTakPcjKtOEXfGlbhjUgzI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746171201; c=relaxed/simple; bh=RHZAl8gKjBWKHRR3oRdcYJeuSxDITPOEBdfRxQvq5qQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GQNVprkuJWlgp0D+dpVRSkvx7RY/2/8CgMHwfRDstyuaXWNzz68NLJhnkzPl6h6HgTFB57EDoUSH5V4VKDPtaAMhmXvoO+MCMjiUvBObqxPJKq3rviVzeu/NP8Mp+9a5g3zrW16AcO7WPHnC0v38sssyOxCfvlfXVtPx7q5ix5Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=bqlTauB3; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="bqlTauB3" Received: by smtp.kernel.org (Postfix) with ESMTPSA id F25E8C4CEE4; Fri, 2 May 2025 07:33:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1746171201; bh=RHZAl8gKjBWKHRR3oRdcYJeuSxDITPOEBdfRxQvq5qQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bqlTauB3vLClzkmpRYqC6anjX4GxZJcGxwF9HLfrD0xrlzBJFKLrOYwnk7ZfIErZ0 zUrIlzzHyNe256IYEs5lmH97+i8o5Lgm6UYruBXhZ43dmWgYkK2JD4NadQIAQ2Woqk whGc3BJ2xaoAZfDK0nwFjp4Ovy75t1XWcd8TdBBGX2hFnadEnaMno559lz+dqi2p22 TuUzj7mBgWk0T6g6+dekWFJDOSWpjGIbSfvXsTqMjrKrVaAHCHbwsPmyhcFcc5OOtM o9w2kp3KUVDUKDpual/ahLRh3rKpO+T6WP5Z2acIGRQV5VNcS+u5r1s5EIHzMvB9gn lQfM9Pmh+WsBQ== Date: Fri, 2 May 2025 09:33:15 +0200 From: Danilo Krummrich To: Greg Kroah-Hartman Cc: Matthew Maurer , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , "Rafael J. Wysocki" , Sami Tolvanen , Timur Tabi , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH v3 1/4] rust: debugfs: Bind DebugFS directory creation Message-ID: References: <20250501-debugfs-rust-v3-0-850869fab672@google.com> <20250501-debugfs-rust-v3-1-850869fab672@google.com> <2025050230-browsing-backstab-8de9@gregkh> <2025050205-reassign-entire-4078@gregkh> Precedence: bulk X-Mailing-List: rust-for-linux@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: <2025050205-reassign-entire-4078@gregkh> On Fri, May 02, 2025 at 09:11:37AM +0200, Greg Kroah-Hartman wrote: > On Fri, May 02, 2025 at 09:05:25AM +0200, Danilo Krummrich wrote: > > On Fri, May 02, 2025 at 09:00:07AM +0200, Greg Kroah-Hartman wrote: > > > On Fri, May 02, 2025 at 08:37:40AM +0200, Danilo Krummrich wrote: > > > > On Thu, May 01, 2025 at 10:47:41PM +0000, Matthew Maurer wrote: > > > > > +/// Handle to a DebugFS directory that will stay alive after leaving scope. > > > > > +#[repr(transparent)] > > > > > +pub struct SubDir(ManuallyDrop); > > > > > > > > I think it's not very intuitive if the default is that a SubDir still exists > > > > after it has been dropped. I think your first approach being explicit about this > > > > with keep() consuming the SubDir was much better; please keep this approach. > > > > > > Wait, let's step back. Why do we care about the difference between a > > > "subdir" and a "dir"? They both are the same thing, and how do you > > > describe a subdir of a subdir? :) > > > > We care about the difference, because Dir originally had keep() which drops the > > Dir instance without actually removing it. For subdirs this is fine, since > > they'll be cleaned up when the parent is removed. > > But does that mean a subdir can not be cleaned up without dropping the > parent first? For many subsystems, they make a "root" debugfs > directory, and then add/remove subdirs all the time within that. In the following I will call the first top level directory created by a module / driver "root". The logic I propose is that "root" is of type Dir, which means there is no keep() and if dropped the whole tree under root is removed. A subdir created under a Dir is of type SubDir and has the keep() method and if called consumes the type instance and subsequently can only ever be removed by dropping root. Alternatively a SubDir can be converted into a Dir, and hence don't has keep() anymore and if dropped will be removed. So, the result is that we still can add / remove subdirs as we want. The advantage is that we don't have keep() for root, which would be a dedicated API for driver / modules to create bugs. If a driver / module would call keep() on the root, it would not only mean that we leak the root directory, but also all subdirs and files that we called keep() on. This becomes even more problematic if we start attaching data to files. Think of an Arc that is attached to a file, which keeps driver data alive just because we leaked the root. > > However, we don't want users to be able to call keep() on the directory that has > > been created first, since if that's done we loose our root anchor to ever free > > the tree, which almost always would be a bug. > > Then do a call to debugfs_lookup_and_remove() which is what I really > recommend doing for any C user anyway. That way no dentry is ever > "stored" anywhere. > > Anyway, if Dir always has an implicit keep() call in it, then I guess > this is ok. Let's see how this shakes out with some real-world users. > We can always change it over time if it gets unwieldy. I really advise against it, Rust allows us to model such subtile differences properly (and easily) with the type system to avoid bugs. Let's take advantage of that. Using debugfs_lookup_and_remove() wouldn't change anything, since we want to attach the lifetime of a directory to a corresponding object. (Otherwise we're back to where we are with C, i.e. the user has to remember to call the correct thing at the correct time, rather than letting the type system take care instead.) So, instead of debugfs_remove() we'd call debugfs_lookup_and_remove() from Dir::drop(), which only changes what we store in Dir, i.e. struct dentry pointer vs. CString.