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

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

[Meng Xu]

Hi VFS developers,

There might exists cases where file->f_mode may race in __fget() and
generic_fadvise().

[Setup]
mkdir(dir_foo, 0777) = 0;
open(dir_foo, 0x10000, 0777) = 3;
dup2(3, 199) = 199;

[Thread 1]: mkdirat(199, dir_foo, 0576) = 0;
[Thread 2]: fadvise64(3, 140517292505364, 155, 0x2) = 0;

[Thread 1: SYS_mkdirat]
__do_sys_mkdirat
  do_mkdirat
    user_path_create
      filename_create
        filename_parentat
          path_parentat
            path_init
              fdget_raw
                __fdget_raw
                  __fget_light
                    __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.

But just in case there may be other cases where the mask is not 0, it
may leads to weird situations and I am posting this issue here for
more visibility. Feel free to comment on its validity.

Best Regards,
Meng

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

[Al Viro]

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.