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.
--
tejun
