summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorEric Dumazet <edumazet@google.com>2015-06-29 17:10:30 +0200
committerAl Viro <viro@zeniv.linux.org.uk>2015-07-01 02:31:08 -0400
commit5ba97d2832f87943c43bb69cb1ef86dbc59df5bc (patch)
tree97666a4d6fe7e925f3fd676f52bd339855a229a5
parent8a81252b774b53e628a8a0fe18e2b8fc236d92cc (diff)
downloadop-kernel-dev-5ba97d2832f87943c43bb69cb1ef86dbc59df5bc.zip
op-kernel-dev-5ba97d2832f87943c43bb69cb1ef86dbc59df5bc.tar.gz
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>
-rw-r--r--fs/file.c10
1 files changed, 8 insertions, 2 deletions
diff --git a/fs/file.c b/fs/file.c
index 3d2eb4c..6c672ad 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -664,11 +664,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();
OpenPOWER on IntegriCloud