Re: [PATCH v4 11/14] Add start_renaming_two_dentries()

On Thu, 30 Oct 2025, Al Viro wrote:
> On Thu, Oct 30, 2025 at 10:31:11AM +1100, NeilBrown wrote:
> 
> > +++ b/fs/debugfs/inode.c
> 
> Why does debugfs_change_name() need any of that horror?  Seriously, WTF?
> This is strictly a name change on a filesystem that never, ever moves
> anything from one directory to another.

"horror" is clearly in the eye of the beholder, and not a helpful
description...

Is there anything in this use of start_renaming_two_dentries() which is
harmful?  I agree that not all of the functionality is needed in this
case, but some of it is.

Would you prefer we also add
   start_renaming_two_dentries_with_same_parent()
or similar?

> 
> IMO struct renamedata is a fucking eyesore, but that aside, this:
> 
> > @@ -539,22 +540,30 @@ static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
> >  	if (ret)
> >  		goto out;
> >  
> > -	lock_rename(tmp_parent, fsi->sb->s_root);
> > +	rd.old_parent = tmp_parent;
> > +	rd.new_parent = fsi->sb->s_root;
> >  
> >  	/* booleans */
> > -	d_exchange(tmp_bool_dir, fsi->bool_dir);
> > +	ret = start_renaming_two_dentries(&rd, tmp_bool_dir, fsi->bool_dir);
> > +	if (!ret) {
> > +		d_exchange(tmp_bool_dir, fsi->bool_dir);
> >  
> > -	swap(fsi->bool_num, bool_num);
> > -	swap(fsi->bool_pending_names, bool_names);
> > -	swap(fsi->bool_pending_values, bool_values);
> > +		swap(fsi->bool_num, bool_num);
> > +		swap(fsi->bool_pending_names, bool_names);
> > +		swap(fsi->bool_pending_values, bool_values);
> >  
> > -	fsi->bool_dir = tmp_bool_dir;
> > +		fsi->bool_dir = tmp_bool_dir;
> > +		end_renaming(&rd);
> > +	}
> >  
> >  	/* classes */
> > -	d_exchange(tmp_class_dir, fsi->class_dir);
> > -	fsi->class_dir = tmp_class_dir;
> > +	ret = start_renaming_two_dentries(&rd, tmp_class_dir, fsi->class_dir);
> > +	if (ret == 0) {
> > +		d_exchange(tmp_class_dir, fsi->class_dir);
> > +		fsi->class_dir = tmp_class_dir;
> >  
> > -	unlock_rename(tmp_parent, fsi->sb->s_root);
> > +		end_renaming(&rd);
> > +	}
> >  
> >  out:
> >  	sel_remove_old_bool_data(bool_num, bool_names, bool_values);
> 
> is very interesting - suddenly you get two non-overlapping scopes instead of one.
> Why is that OK?
> 

From the perspective of code performing lookup of these names, two
consecutive lookups would not be locked so they could see
inconsistencies anyway.
From the perspective of code changing these names, that is protected by
selinux_state.policy_mutex which is held across the combined operation.
A readdir could possibly see the old inum for one name and the new inum
for the other name.  I don't imagine this would be a problem.

I have added a comment to the commit message to highlight this.

Thanks,
NeilBrown