Re: [PATCH 3/3] kernfs: fix up the unlocked attribute reads on the creation paths

[Tejun Heo <tj@kernel.org>]

On Wed, Sep 02, 2026 at 09:02:53PM -0700, Shakeel Butt wrote:
> Two creation paths read a live node's attributes without holding
> kernfs_iattr_rwsem, which kernfs_iop_setattr() takes for writing.  They
> do not want the same fix.
> 
> kernfs_create_link() copies the target's ia_uid and then its ia_gid
> into the new link.  A chown of the target between the two reads leaves
> the link with the old uid and the new gid, an owner the target never
> had, when copying the target's owner is the whole point.  Read both
> fields under the rwsem.
> 
> kernfs_new_node() reads the parent's ia_gid for S_ISGID inheritance,
> and that one does not care which value it gets: the node does not exist
> yet, so nothing orders a racing chown against the creation either way.
> Taking the rwsem there would only serialize creation under a set-gid
> parent against a chown of that parent, to pick between two answers that
> are both right.  Mark the field read data_race() instead.
> 
> The pointer that leads to it is a different matter: __kernfs_iattrs()
> publishes kernfs_node::iattr with try_cmpxchg(), so there is no
> unmarked write for that read to pair with, and both sides read it with
> READ_ONCE() like the rest of fs/kernfs does.
> 
> The Fixes tag is for the symlink half.  kernfs_create_link() has read
> the pair without a lock since it started copying the target's owner at
> all; only the name of the lock its writer takes has changed since.  The
> data_race() is a marking rather than a fix.
> 
> Fixes: 488dee96bb62 ("kernfs: allow creating kernfs objects with arbitrary uid/gid")
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Tejun Heo <tj@kernel.org>

> +		if (attrs) {
> +			/*
> +			 * Unlocked on purpose: the gid is inherited onto a
> +			 * node that does not exist yet, so nothing orders a
> +			 * racing chown against this creation, and either
> +			 * value is correct.  The pointer above needs no such
> +			 * marking, __kernfs_iattrs() publishes it with
> +			 * try_cmpxchg().
> +			 */
> +			gid = data_race(attrs->ia_gid);

Nit: READ_ONCE() is likely the better fit as it's an intentional lockless
read whose value is used.

Thanks.

-- 
tejun