Re: Possible data race on file->f_mode between __fget() and generic_fadvise()

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Nov 18, 2019 at 09:02:31PM -0500, Meng Xu wrote:
>                     __fget
>                       [READ] if (file->f_mode & mask)
> 
> [Thread 2: SYS_fadvise64]
> __do_sys_fadvise64
>   ksys_fadvise64_64
>     vfs_fadvise
>       generic_fadvise
>         [WRITE] file->f_mode &= ~FMODE_RANDOM;
> 
> However, in this particular case, there is no issues caused as the
> mask passed in __fget() is always 0 and hence, does not matter what
> the [WRITE] statement is doing.

Or FMODE_PATH.  Other readers in the same area look at FMODE_ATOMIC_POS
as well.  And neither FMODE_PATH nor FMODE_ATOMIC_POS is ever modified
after the time struct file instance is set up - certainly not after the
time it's present in descriptor table.

Barriers to look at:
        rcu_assign_pointer(fdt->fd[fd], file);
on the insertion side (__fd_install()),
                return rcu_dereference_raw(fdt->fd[fd]);
on the fetch side (fcheck_files()).  All stores to *file done
prior to putting it into descriptor table should be visible
to everyone who fetches file from there.

If your struct file reaches the reader via some other mechanism, the barriers
are up to that other mechanism.  These are for descriptor tables.  In case of
transmission by SCM_RIGHTS datagrams, for example, we have a RELEASE/ACQUIRE
pair provided by queue lock of the AF_UNIX socket involved, etc.

Generally, if another thread could find a struct file instance, but fail to
observe the stores done by do_dentry_open(), we would be very deep in trouble;
that would include the stores done by ->open(), setting of ->f_op, etc.
So anything that makes struct file visible in shared data structures ought
to take care with barriers, with (minimal) cooperation of those who fetch it
from those.  Pretty much any kind of locks will do it automatically (all
stores prior to unlock are visible to everone doing lock later); lockless
mechanisms need to take some care.

In any case, these bits of ->f_mode fall into the same category as ->f_path,
->f_op, etc. - set prior to return from the function that sets the struct
file instance up and never changed afterwards.  All flags that can be
changed later are protected by ->f_lock; actually, right now it's
FMODE_RANDOM alone, but that can change.