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 <[email protected]>
>
> Acked-by: Tejun Heo <[email protected]>
>
> > + 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.
