Re: [PATCH v3 34/50] selinuxfs: new helper for attaching files to tree

[Chris Mason <clm@meta.com>]

On 11/11/25 4:49 AM, Al Viro wrote:
> On Tue, Nov 11, 2025 at 07:53:18AM +0000, bot+bpf-ci@kernel.org wrote:
> 
>> Can this leak the parent directory's reference count? The parent inode's
>> link count is incremented with inc_nlink(d_inode(dir)) before calling
>> sel_attach(). When sel_attach()->d_alloc_name() fails and returns NULL,
>> sel_attach() correctly cleans up the child inode with iput() and returns
>> ERR_PTR(-ENOMEM). However, the parent directory's link count has already
>> been incremented and is never decremented on this error path.
>>
>> In the original code, the parent link count increment happened after
>> d_add() succeeded, ensuring it only occurred when the full operation
>> completed successfully.
> 
> All callers of sel_make_dir() proceed to remove the parent in case of
> failure.  All directories are created either at mount time or at
> policy reload afterwards.  A failure in the former will have
> sel_fill_super() return an error, with the entire filesystem instance
> being torn apart by the cleanup path in its caller (get_tree_single()).
> No directories survive that.  A failure in the latter (in something
> called from sel_make_policy_nodes()) will be taken care of by the
> call of simple_recursive_removal() in the end of sel_make_policy_nodes() -
> there we
> 	1.  create a temporary directory ("/.swapover").  We do *NOT*
> use sel_make_dir() for that - see sel_make_swapover_dir().  If that has
> failed, we return an error.
> 	2.  create and populate two subtrees in it ("booleans" and "classes").
> That's the step where we would create subdirectories and that's where
> sel_make_dir() failures might occur.
> 	3.  if the subtree creation had been successful, swap "/.swapover/booleans"
> with "/booleans" and "/.swapover/classes" with "/classes" respectively.
> 	4.  recursively remove "/.swapover", along with anything that might
> be in it.  In case of success that would be the old "/classes" and "/booleans"
> that got replaced, in case of failure - whatever we have partially created.
> 
> That's the same reason why we don't need to bother with failure cleanups in
> the functions that populate directories - if they fail halfway through, the
> entire (sub)tree is going to be wiped out in one pass.

I've rerun this review a bunch of times, and it properly reproduces a
variation of Al's explanation every time.  I'll try to firm up the vfs
rules but I can't find a specific way to fix this one.

-chris