Re: [PATCH] namei: Use READ_ONCE() for d_flags in __follow_mount_rcu()

[Jan Kara]

On Wed 15-07-26 20:50:57, Ze Tan wrote:
> __follow_mount_rcu() reads dentry->d_flags without holding d_lock.
> 
> Use READ_ONCE() for these reads to make the lockless access clear.
> 
> Signed-off-by: Ze Tan <tanze@xxxxxxxxxx>

Hum, so we are somewhat sloppy with the access to d_flags. Strictly
speaking we should be using WRITE_ONCE when updating them (and we do in
some places but not in others) and READ_ONCE when accessing them. OTOH the
only practical problem is KCSAN complaining, the problems that the compiler
could somehow tear stores to the d_flags are IMHO theoretical.

Having two accesses without READ_ONCE more or less doesn't really matter in
my opinion. Cleaning up all the d_flags accesses would make some sense but
I'm not sure it's worth the churn for mostly theoretical issues (I believe
Al or Christian was against it if my memory serves well).

								Honza

> ---
>  fs/namei.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 19ce43c9a6e6..cc8e8d961d7e 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1687,7 +1687,7 @@ EXPORT_SYMBOL(follow_down);
>  static bool __follow_mount_rcu(struct nameidata *nd, struct path *path)
>  {
>  	struct dentry *dentry = path->dentry;
> -	unsigned int flags = dentry->d_flags;
> +	unsigned int flags = READ_ONCE(dentry->d_flags);
>  
>  	if (unlikely(nd->flags & LOOKUP_NO_XDEV))
>  		return false;
> @@ -1701,7 +1701,7 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path)
>  			int res = dentry->d_op->d_manage(path, true);
>  			if (res)
>  				return res == -EISDIR;
> -			flags = dentry->d_flags;
> +			flags = READ_ONCE(dentry->d_flags);
>  		}
>  
>  		if (flags & DCACHE_MOUNTED) {
> @@ -1711,7 +1711,7 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path)
>  				dentry = path->dentry = mounted->mnt.mnt_root;
>  				nd->state |= ND_JUMPED;
>  				nd->next_seq = read_seqcount_begin(&dentry->d_seq);
> -				flags = dentry->d_flags;
> +				flags = READ_ONCE(dentry->d_flags);
>  				// makes sure that non-RCU pathwalk could reach
>  				// this state.
>  				if (read_seqretry(&mount_lock, nd->m_seq))
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR