configfs cleanups and fixes#1429
Open
vfsci-bot[bot] wants to merge 14 commits into
Open
Conversation
added 14 commits
May 19, 2026 07:45
Normally ->s_dentry is cleared when dentry it's pointing to becomes
negative (on eviction, realistically). However, that only happens
if dentry gets to be positive in the first place; in case of inode
allocation failure dentry never becomes positive, so ->d_iput()
is not called at all.
We do part of what normally would've been done by configfs_d_iput()
(dropping the reference to configfs_dirent) manually, but we do
not clear ->s_dentry there. Sloppy as it is, it does not matter in
case of configfs_create_{dir,link}() - there configfs_dirent does
not survive dropping the sole reference to it.
However, for configfs_lookup() it *does* survive, with a dangling
pointer to soon to be freed dentry sitting it its ->s_dentry.
Subsequent getdents(2) in that directory will end up dereferencing
that pointer in order to pick the inode number. Use after free...
This is the minimal fix; the right approach is to set the linkage
between dentry and configfs_dirent only after we know that we have
an inode, but that takes more surgery and the bug had been there
since 2006, so...
Fixes: 3d0f89b ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Note that neither ->make_group() nor ->make_item() are allowed to modify the string passed to them - the argument is const char *. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
The only thing it uses the argument for is its ->d_fsdata and all callers have that already available. Note that in the recursive call we are dealing with a (sub)directory configfs_dirent, and for those ->s_dentry->d_fsdata points back to configfs_dirent itself. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Again, the only thing it uses dentry for is dentry->d_fsdata; for the recursive call the situation is the same as with configfs_detach_prep() and the same observation about ->s_dentry->d_fsdata applies. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Again, the only thing it uses the argument for is its ->d_fsdata and callers already have that - as the matter of fact, they are passing ->s_dentry of that configfs_dirent, so that the function could get it back as ->d_fsdata of that. With nothing else in dentry even looked at... configfs_dirent in question is a directory one - in this case those are subdirectories of root (aka roots of "subsystem" trees). Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
same story as with configfs_detach_prep() this function is undoing. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
... where it folds with configfs_detach_item() into a call of configfs_detach_group(). Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
... where it folds with configfs_remove_dir() into a call of configfs_detach_item(). Note that at the early failure exit (before we'd added any children) we were not calling detach_attrs() only because there it would've been a no-op - nothing added, nothing there to be removed. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
... and deal with grabbing/dropping it in the sole caller. After that configfs_remove_dir() becomes an unconditional call of remove_dir(), so we can fold them together. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
... and there's no need to grab/drop it, or check for NULL - none of the callers would even get there with NULL dentry and all of them have the sucker pinned Note that if sd is a directory configfs_dirent, we have sd->s_element pointing to config_item with item->ci_dentry equal to sd->s_dentry. Which is the only reason why detach_groups() gets away with using the latter for locking the inode and the former for removal. Aren't redundant data structures wonderful, for obfuscation if nothing else? Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
currently we have a weird situation where
* symlinks and roots of subtrees created by mkdir are pinned once
* subdirectories of subtrees created by mkdir are pinned twice
* roots of subtrees created by register_{group,subsystem} are pinned
twice.
It makes things harder to follow for no good reason. The goal is to
encapsulate the unbalanced dget/dput into d_{make,discard}_persisitent()
and, preferably, allow a use of simple_recursive_removal() or analogue
thereof. So let's regularize that and pin things only once.
create_default_group() and configfs_register_subsystem() don't need to
keep their reference around on success - configfs_create_dir() has pinned
the sucker already. So we can drop the reference passed to
configfs_create_dir() (via configfs_attach_group(), etc.) both on success
and on failure. On the removal side we no longer have the double references,
so we need an explicit dget() to compensate.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
on the removal side we can (finally) get rid of __simple_unlink() and __simple_rmdir() kludges now that dentries in question are properly marked persistent - simple_unlink() and simple_rmdir() will do the right thing for those. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Fold into the only remaining user, don't bother with the timestamps of parent - we are going to rmdir it shortly anyway, which will override those. Fix the locking of inode, while we are at it - updating the link count and timestamps ought to be done with the inode locked. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
... and do *not* do it in ->lookup() case. stat foo/bar should not update mtime of foo, TYVM... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Series: https://patchwork.kernel.org/project/linux-fsdevel/list/?series=1097096
Submitter: Al Viro
Version: 1
Patches: 14/14
Message-ID:
<20260519070633.2025485-1-viro@zeniv.linux.org.uk>Base: vfs.base.ci
Lore: https://lore.kernel.org/linux-fsdevel/20260519070633.2025485-1-viro@zeniv.linux.org.uk
Automated by ml2pr