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

[Shakeel Butt <shakeel.butt@linux.dev>]

On Thu, Sep 03, 2026 at 10:26:07AM -1000, Tejun Heo wrote:
> 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 TJ for the review. I will fix this in next version.