fs/file.c: __fget() and dup2() atomicity rules

__fget() does lockless fetch of pointer from the descriptor
table, attempts to grab a reference and treats "it was already
zero" as "it's already gone from the table, we just hadn't
seen the store, let's fail".  Unfortunately, that breaks the
atomicity of dup2() - __fget() might see the old pointer,
notice that it's been already dropped and treat that as
"it's closed".  What we should be getting is either the
old file or new one, depending whether we come before or after
dup2().

Dmitry had following test failing sometimes :

int fd;
void *Thread(void *x) {
  char buf;
  int n = read(fd, &buf, 1);
  if (n != 1)
    exit(printf("read failed: n=%d errno=%d\n", n, errno));
  return 0;
}

int main()
{
  fd = open("/dev/urandom", O_RDONLY);
  int fd2 = open("/dev/urandom", O_RDONLY);
  if (fd == -1 || fd2 == -1)
    exit(printf("open failed\n"));
  pthread_t th;
  pthread_create(&th, 0, Thread, 0);
  if (dup2(fd2, fd) == -1)
    exit(printf("dup2 failed\n"));
  pthread_join(th, 0);
  if (close(fd) == -1)
    exit(printf("close failed\n"));
  if (close(fd2) == -1)
    exit(printf("close failed\n"));
  printf("DONE\n");
  return 0;
}

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Orabug: 25408921
From 25408921:
Signed-off-by: todd.vierling@oracle.com
Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>

diff --git a/fs/file.c b/fs/file.c
index 93c5f89c248b07b6fba50fd31f4bd36ba9520fef..492bd74c4433889d5fdfa3bade7cc298e72e33c9 100644 (file)
--- a/fs/file.c
+++ b/fs/file.c
@@ -635,11 +635,17 @@ static struct file *__fget(unsigned int fd, fmode_t mask)
        struct file *file;
 
        rcu_read_lock();
+loop:
        file = fcheck_files(files, fd);
        if (file) {
-               /* File object ref couldn't be taken */
-               if ((file->f_mode & mask) || !get_file_rcu(file))
+               /* File object ref couldn't be taken.
+                * dup2() atomicity guarantee is the reason
+                * we loop to catch the new file (or NULL pointer)
+                */
+               if (file->f_mode & mask)
                        file = NULL;
+               else if (!get_file_rcu(file))
+                       goto loop;
        }
        rcu_read_unlock();