There's a long long-standing bug...As long as I don't know when it dates
from.
I've written and attached a simple program to reproduce this bug, and it can
immediately trigger the bug in my box. It uses two threads, one keeps calling
read(), and the other calling readdir(), both on the same directory fd.
When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
feature disabled, I got this:
EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
...
If we configured errors=remount-ro, the filesystem will become read-only.
SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
{
...
loff_t pos = file_pos_read(file);
ret = vfs_read(file, buf, count, &pos);
file_pos_write(file, pos);
fput_light(file, fput_needed);
...
}
While readdir() is protected with i_mutex, f_pos can be changed without any locking
in various read()/write() syscalls, which leads to this bug.
What makes things worse is Andi removed i_mutex from generic_file_llseek, so you
can trigger the same bug by replacing read() with lseek() in the test program.
commit ef3d0fd27e90f67e35da516dafc1482c82939a60
Author: Andi Kleen <[email protected]>
Date: Thu Sep 15 16:06:48 2011 -0700
vfs: do (nearly) lockless generic_file_llseek
I've tested ext3 with dir_index enabled and btrfs, nothing bad happened, but there
should be some other vulnerabilities. For example, running the test program on /sys
for a few minutes triggered this warning:
[ 917.994600] ------------[ cut here ]------------
[ 917.994614] WARNING: at fs/sysfs/sysfs.h:195 sysfs_readdir+0x24c/0x260()
[ 917.994621] Hardware name: Tecal RH2285
...
[ 917.994725] Pid: 8754, comm: a.out Not tainted 3.8.0-rc2-tj-0.7-default+ #69
[ 917.994731] Call Trace:
[ 917.994736] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
[ 917.994743] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
[ 917.994752] [<ffffffff81041fff>] warn_slowpath_common+0x7f/0xc0
[ 917.994759] [<ffffffff8104205a>] warn_slowpath_null+0x1a/0x20
[ 917.994766] [<ffffffff81205c6c>] sysfs_readdir+0x24c/0x260
[ 917.994774] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
[ 917.994780] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
[ 917.994787] [<ffffffff8119cfc1>] vfs_readdir+0xb1/0xd0
[ 917.994794] [<ffffffff8119d07b>] sys_getdents64+0x9b/0x110
[ 917.994803] [<ffffffff814a45d9>] system_call_fastpath+0x16/0x1b
[ 917.994809] ---[ end trace 6efe15a65b89022a ]---
[ 917.994816] ida_remove called for id=13073 which is not allocated.
We can fix this bug in each filesystem, but can't we just make sure i_mutex is
acquired in lseek(), read(), write() and readdir() for directory file operations?
(the patch is for demonstration only)
diff --git a/fs/read_write.c b/fs/read_write.c
index bb34af3..41f76e5 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -218,14 +218,25 @@ EXPORT_SYMBOL(default_llseek);
loff_t vfs_llseek(struct file *file, loff_t offset, int whence)
{
+ struct inode *inode = file->f_path.dentry->d_inode;
loff_t (*fn)(struct file *, loff_t, int);
+ int ret;
fn = no_llseek;
if (file->f_mode & FMODE_LSEEK) {
if (file->f_op && file->f_op->llseek)
fn = file->f_op->llseek;
}
- return fn(file, offset, whence);
+
+ if (S_ISDIR(inode->i_mode)) {
+ mutex_lock(&inode->i_mutex);
+ ret = fn(file, offset, whence);
+ mutex_unlock(&inode->i_mutex);
+ } else {
+ ret = fn(file, offset, whence);
+ }
+
+ return ret;
}
EXPORT_SYMBOL(vfs_llseek);
@@ -442,12 +453,32 @@ EXPORT_SYMBOL(vfs_write);
static inline loff_t file_pos_read(struct file *file)
{
- return file->f_pos;
+ struct inode *inode = file->f_path.dentry->d_inode;
+ loff_t pos;
+
+ if (S_ISDIR(inode->i_mode)) {
+ mutex_lock(&inode->i_mutex);
+ pos = file->f_pos;
+ mutex_unlock(&inode->i_mutex);
+ } else {
+ pos = file->f_pos;
+ }
+
+ return pos;
}
static inline void file_pos_write(struct file *file, loff_t pos)
{
- file->f_pos = pos;
+ struct inode *inode = file->f_path.dentry->d_inode;
+
+ if (S_ISDIR(inode->i_mode)) {
+ mutex_lock(&inode->i_mutex);
+ file->f_pos = pos;
+ file->f_version = 0;
+ mutex_unlock(&inode->i_mutex);
+ } else {
+ file->f_pos = pos;
+ }
}
SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
On tue, 19 Feb 2013 09:22:40 +0800, Li Zefan wrote:
> There's a long long-standing bug...As long as I don't know when it dates
> from.
>
> I've written and attached a simple program to reproduce this bug, and it can
> immediately trigger the bug in my box. It uses two threads, one keeps calling
> read(), and the other calling readdir(), both on the same directory fd.
>
> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
> feature disabled, I got this:
>
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> ...
>
> If we configured errors=remount-ro, the filesystem will become read-only.
>
> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
> {
> ...
> loff_t pos = file_pos_read(file);
> ret = vfs_read(file, buf, count, &pos);
> file_pos_write(file, pos);
> fput_light(file, fput_needed);
> ...
> }
>
> While readdir() is protected with i_mutex, f_pos can be changed without any locking
> in various read()/write() syscalls, which leads to this bug.
>
> What makes things worse is Andi removed i_mutex from generic_file_llseek, so you
> can trigger the same bug by replacing read() with lseek() in the test program.
>
> commit ef3d0fd27e90f67e35da516dafc1482c82939a60
> Author: Andi Kleen <[email protected]>
> Date: Thu Sep 15 16:06:48 2011 -0700
>
> vfs: do (nearly) lockless generic_file_llseek
>
> I've tested ext3 with dir_index enabled and btrfs, nothing bad happened, but there
> should be some other vulnerabilities. For example, running the test program on /sys
> for a few minutes triggered this warning:
>
> [ 917.994600] ------------[ cut here ]------------
> [ 917.994614] WARNING: at fs/sysfs/sysfs.h:195 sysfs_readdir+0x24c/0x260()
> [ 917.994621] Hardware name: Tecal RH2285
> ...
> [ 917.994725] Pid: 8754, comm: a.out Not tainted 3.8.0-rc2-tj-0.7-default+ #69
> [ 917.994731] Call Trace:
> [ 917.994736] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
> [ 917.994743] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
> [ 917.994752] [<ffffffff81041fff>] warn_slowpath_common+0x7f/0xc0
> [ 917.994759] [<ffffffff8104205a>] warn_slowpath_null+0x1a/0x20
> [ 917.994766] [<ffffffff81205c6c>] sysfs_readdir+0x24c/0x260
> [ 917.994774] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
> [ 917.994780] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
> [ 917.994787] [<ffffffff8119cfc1>] vfs_readdir+0xb1/0xd0
> [ 917.994794] [<ffffffff8119d07b>] sys_getdents64+0x9b/0x110
> [ 917.994803] [<ffffffff814a45d9>] system_call_fastpath+0x16/0x1b
> [ 917.994809] ---[ end trace 6efe15a65b89022a ]---
> [ 917.994816] ida_remove called for id=13073 which is not allocated.
>
>
> We can fix this bug in each filesystem, but can't we just make sure i_mutex is
> acquired in lseek(), read(), write() and readdir() for directory file operations?
I think it is unnecessary to acquire i_mutex in lseek(), read() and write(), because
we can be aware of the change of f_pos, and then get and tune the value in readdir(),
just like ext3 with dir_index enabled.
Thanks
Miao
On Tue 19-02-13 09:22:40, Li Zefan wrote:
> There's a long long-standing bug...As long as I don't know when it dates
> from.
>
> I've written and attached a simple program to reproduce this bug, and it can
> immediately trigger the bug in my box. It uses two threads, one keeps calling
> read(), and the other calling readdir(), both on the same directory fd.
So the fact that read() or even write() to fd opened O_RDONLY has *any*
effect on f_pos looks really unexpected to me. I think we really should
have there:
if (ret >= 0)
file_pos_write(...);
That would solve problems with read() and write() on directories for
pretty much every filesystem since the first usually returns -EISDIR and
the second -EBADF.
> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
> feature disabled, I got this:
>
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> ...
>
> If we configured errors=remount-ro, the filesystem will become read-only.
>
> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
> {
> ...
> loff_t pos = file_pos_read(file);
> ret = vfs_read(file, buf, count, &pos);
> file_pos_write(file, pos);
> fput_light(file, fput_needed);
> ...
> }
>
> While readdir() is protected with i_mutex, f_pos can be changed without
> any locking in various read()/write() syscalls, which leads to this bug.
>
> What makes things worse is Andi removed i_mutex from generic_file_llseek,
> so you can trigger the same bug by replacing read() with lseek() in the
> test program.
Yes, and here I'd say it's a filesystem issue. If filesystem needs f_pos
changed only under i_mutex, it should use default_llseek() or get the mutex
itself. That's what the callback is for. We shouldn't unnecessarily impose
the i_mutex restriction on llseek on a directory for every filesystem.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2013/2/19 17:19, Jan Kara wrote:
> On Tue 19-02-13 09:22:40, Li Zefan wrote:
>> There's a long long-standing bug...As long as I don't know when it dates
>> from.
>>
>> I've written and attached a simple program to reproduce this bug, and it can
>> immediately trigger the bug in my box. It uses two threads, one keeps calling
>> read(), and the other calling readdir(), both on the same directory fd.
> So the fact that read() or even write() to fd opened O_RDONLY has *any*
> effect on f_pos looks really unexpected to me. I think we really should
> have there:
> if (ret >= 0)
> file_pos_write(...);
I thought about this. The problem is then we have to check every fop->write()
to see if any of them can return -errno with file->f_pos changed and fix them,
though it's do-able.
> That would solve problems with read() and write() on directories for
> pretty much every filesystem since the first usually returns -EISDIR and
> the second -EBADF.
Yeah, seems ceph is the only filesystem that allows read() on directories.
>
>> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
>> feature disabled, I got this:
>>
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
>> ...
>>
>> If we configured errors=remount-ro, the filesystem will become read-only.
>>
>> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
>> {
>> ...
>> loff_t pos = file_pos_read(file);
>> ret = vfs_read(file, buf, count, &pos);
>> file_pos_write(file, pos);
>> fput_light(file, fput_needed);
>> ...
>> }
>>
>> While readdir() is protected with i_mutex, f_pos can be changed without
>> any locking in various read()/write() syscalls, which leads to this bug.
>>
>> What makes things worse is Andi removed i_mutex from generic_file_llseek,
>> so you can trigger the same bug by replacing read() with lseek() in the
>> test program.
> Yes, and here I'd say it's a filesystem issue. If filesystem needs f_pos
> changed only under i_mutex, it should use default_llseek() or get the mutex
> itself. That's what the callback is for. We shouldn't unnecessarily impose
> the i_mutex restriction on llseek on a directory for every filesystem.
>
One of my concern is, concurrent lseek() and readdir() doesn't seem to be
well tested. I'll add a test case in xfstests.
On 2013/2/19 17:19, Jan Kara wrote:
> On Tue 19-02-13 09:22:40, Li Zefan wrote:
>> There's a long long-standing bug...As long as I don't know when it dates
>> from.
>>
>> I've written and attached a simple program to reproduce this bug, and it can
>> immediately trigger the bug in my box. It uses two threads, one keeps calling
>> read(), and the other calling readdir(), both on the same directory fd.
> So the fact that read() or even write() to fd opened O_RDONLY has *any*
> effect on f_pos looks really unexpected to me. I think we really should
> have there:
> if (ret >= 0)
> file_pos_write(...);
I thought about this. The problem is then we have to check every fop->write()
to see if any of them can return -errno with file->f_pos changed and fix them,
though it's do-able.
> That would solve problems with read() and write() on directories for
> pretty much every filesystem since the first usually returns -EISDIR and
> the second -EBADF.
Yeah, seems ceph is the only filesystem that allows read() on directories.
>
>> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
>> feature disabled, I got this:
>>
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
>> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
>> ...
>>
>> If we configured errors=remount-ro, the filesystem will become read-only.
>>
>> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
>> {
>> ...
>> loff_t pos = file_pos_read(file);
>> ret = vfs_read(file, buf, count, &pos);
>> file_pos_write(file, pos);
>> fput_light(file, fput_needed);
>> ...
>> }
>>
>> While readdir() is protected with i_mutex, f_pos can be changed without
>> any locking in various read()/write() syscalls, which leads to this bug.
>>
>> What makes things worse is Andi removed i_mutex from generic_file_llseek,
>> so you can trigger the same bug by replacing read() with lseek() in the
>> test program.
> Yes, and here I'd say it's a filesystem issue. If filesystem needs f_pos
> changed only under i_mutex, it should use default_llseek() or get the mutex
> itself. That's what the callback is for. We shouldn't unnecessarily impose
> the i_mutex restriction on llseek on a directory for every filesystem.
>
One of my concern is, concurrent lseek() and readdir() doesn't seem to be
well tested. I'll add a test case in xfstests.
On Tue, Feb 19, 2013 at 09:22:40AM +0800, Li Zefan wrote:
> There's a long long-standing bug...As long as I don't know when it dates
> from.
>
> I've written and attached a simple program to reproduce this bug, and it can
> immediately trigger the bug in my box. It uses two threads, one keeps calling
> read(), and the other calling readdir(), both on the same directory fd.
Hi Zefan,
Out of curiosity, why do you call read(2) on a directory fd? I only
open(2) a directory in order to execute a flush operation to make sure
that a file is really created.
Regards,
- Zheng
>
> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
> feature disabled, I got this:
>
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> ...
>
> If we configured errors=remount-ro, the filesystem will become read-only.
>
> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
> {
> ...
> loff_t pos = file_pos_read(file);
> ret = vfs_read(file, buf, count, &pos);
> file_pos_write(file, pos);
> fput_light(file, fput_needed);
> ...
> }
>
> While readdir() is protected with i_mutex, f_pos can be changed without any locking
> in various read()/write() syscalls, which leads to this bug.
>
> What makes things worse is Andi removed i_mutex from generic_file_llseek, so you
> can trigger the same bug by replacing read() with lseek() in the test program.
>
> commit ef3d0fd27e90f67e35da516dafc1482c82939a60
> Author: Andi Kleen <[email protected]>
> Date: Thu Sep 15 16:06:48 2011 -0700
>
> vfs: do (nearly) lockless generic_file_llseek
>
> I've tested ext3 with dir_index enabled and btrfs, nothing bad happened, but there
> should be some other vulnerabilities. For example, running the test program on /sys
> for a few minutes triggered this warning:
>
> [ 917.994600] ------------[ cut here ]------------
> [ 917.994614] WARNING: at fs/sysfs/sysfs.h:195 sysfs_readdir+0x24c/0x260()
> [ 917.994621] Hardware name: Tecal RH2285
> ...
> [ 917.994725] Pid: 8754, comm: a.out Not tainted 3.8.0-rc2-tj-0.7-default+ #69
> [ 917.994731] Call Trace:
> [ 917.994736] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
> [ 917.994743] [<ffffffff81205c6c>] ? sysfs_readdir+0x24c/0x260
> [ 917.994752] [<ffffffff81041fff>] warn_slowpath_common+0x7f/0xc0
> [ 917.994759] [<ffffffff8104205a>] warn_slowpath_null+0x1a/0x20
> [ 917.994766] [<ffffffff81205c6c>] sysfs_readdir+0x24c/0x260
> [ 917.994774] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
> [ 917.994780] [<ffffffff8119cbd0>] ? sys_ioctl+0x90/0x90
> [ 917.994787] [<ffffffff8119cfc1>] vfs_readdir+0xb1/0xd0
> [ 917.994794] [<ffffffff8119d07b>] sys_getdents64+0x9b/0x110
> [ 917.994803] [<ffffffff814a45d9>] system_call_fastpath+0x16/0x1b
> [ 917.994809] ---[ end trace 6efe15a65b89022a ]---
> [ 917.994816] ida_remove called for id=13073 which is not allocated.
>
>
> We can fix this bug in each filesystem, but can't we just make sure i_mutex is
> acquired in lseek(), read(), write() and readdir() for directory file operations?
>
> (the patch is for demonstration only)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index bb34af3..41f76e5 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -218,14 +218,25 @@ EXPORT_SYMBOL(default_llseek);
>
> loff_t vfs_llseek(struct file *file, loff_t offset, int whence)
> {
> + struct inode *inode = file->f_path.dentry->d_inode;
> loff_t (*fn)(struct file *, loff_t, int);
> + int ret;
>
> fn = no_llseek;
> if (file->f_mode & FMODE_LSEEK) {
> if (file->f_op && file->f_op->llseek)
> fn = file->f_op->llseek;
> }
> - return fn(file, offset, whence);
> +
> + if (S_ISDIR(inode->i_mode)) {
> + mutex_lock(&inode->i_mutex);
> + ret = fn(file, offset, whence);
> + mutex_unlock(&inode->i_mutex);
> + } else {
> + ret = fn(file, offset, whence);
> + }
> +
> + return ret;
> }
> EXPORT_SYMBOL(vfs_llseek);
>
> @@ -442,12 +453,32 @@ EXPORT_SYMBOL(vfs_write);
>
> static inline loff_t file_pos_read(struct file *file)
> {
> - return file->f_pos;
> + struct inode *inode = file->f_path.dentry->d_inode;
> + loff_t pos;
> +
> + if (S_ISDIR(inode->i_mode)) {
> + mutex_lock(&inode->i_mutex);
> + pos = file->f_pos;
> + mutex_unlock(&inode->i_mutex);
> + } else {
> + pos = file->f_pos;
> + }
> +
> + return pos;
> }
>
> static inline void file_pos_write(struct file *file, loff_t pos)
> {
> - file->f_pos = pos;
> + struct inode *inode = file->f_path.dentry->d_inode;
> +
> + if (S_ISDIR(inode->i_mode)) {
> + mutex_lock(&inode->i_mutex);
> + file->f_pos = pos;
> + file->f_version = 0;
> + mutex_unlock(&inode->i_mutex);
> + } else {
> + file->f_pos = pos;
> + }
> }
>
> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
>
>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <dirent.h>
> #include <fcntl.h>
> #include <sys/stat.h>
> #include <sys/types.h>
>
> int main(int argc, char *argv[])
> {
> int fd;
> int ret;
> DIR *dir;
> struct dirent *ptr;
>
> if (argc != 2)
> errx(1, "Please specify a directory");
>
> dir = opendir(argv[1]);
> if (!dir)
> err(1, "Failed to open directory %s", argv[1]);
>
> fd = dirfd(dir);
> if (fd < 0)
> err(1, "Failed to get dirfd");
>
> ret = fork();
> if (ret == 0) {
> char buf[100];
>
> while (1)
> read(fd, buf, 100);
> } else {
> int ret2;
>
> while (1) {
> ret2 = lseek(fd, 0, SEEK_SET);
> if (ret2 < -1)
> err(1, "seek failed");
>
> while (ptr = readdir(dir))
> ;
> }
> }
>
> closedir(dir);
> return 0;
> }
On 2013/2/19 20:33, Zheng Liu wrote:
> On Tue, Feb 19, 2013 at 09:22:40AM +0800, Li Zefan wrote:
>> There's a long long-standing bug...As long as I don't know when it dates
>> from.
>>
>> I've written and attached a simple program to reproduce this bug, and it can
>> immediately trigger the bug in my box. It uses two threads, one keeps calling
>> read(), and the other calling readdir(), both on the same directory fd.
>
> Hi Zefan,
>
> Out of curiosity, why do you call read(2) on a directory fd? I only
> open(2) a directory in order to execute a flush operation to make sure
> that a file is really created.
>
Because something wrong happened in userspace programs.
After a thread closed a socket, another thread is still reading data from
this socket, but the socket fd has been re-used for opening directory for
readdir()!
On Tue 19-02-13 19:47:30, Li Zefan wrote:
> On 2013/2/19 17:19, Jan Kara wrote:
> > On Tue 19-02-13 09:22:40, Li Zefan wrote:
> >> There's a long long-standing bug...As long as I don't know when it dates
> >> from.
> >>
> >> I've written and attached a simple program to reproduce this bug, and it can
> >> immediately trigger the bug in my box. It uses two threads, one keeps calling
> >> read(), and the other calling readdir(), both on the same directory fd.
> > So the fact that read() or even write() to fd opened O_RDONLY has *any*
> > effect on f_pos looks really unexpected to me. I think we really should
> > have there:
> > if (ret >= 0)
> > file_pos_write(...);
>
> I thought about this. The problem is then we have to check every fop->write()
> to see if any of them can return -errno with file->f_pos changed and fix them,
> though it's do-able.
But returning error and advancing f_pos would be a bug - specification
says write() returns the number of bytes written or -1 and f_pos should be
advanced by the number of bytes written.
> > That would solve problems with read() and write() on directories for
> > pretty much every filesystem since the first usually returns -EISDIR and
> > the second -EBADF.
>
> Yeah, seems ceph is the only filesystem that allows read() on directories.
>
> >> When I ran it on ext3 (can be replaced with ext2/ext4) which has _dir_index_
> >> feature disabled, I got this:
> >>
> >> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> >> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> >> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=993, inode=0, rec_len=0, name_len=0
> >> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory #34817: rec_len is smaller than minimal - offset=1009, inode=0, rec_len=0, name_len=0
> >> ...
> >>
> >> If we configured errors=remount-ro, the filesystem will become read-only.
> >>
> >> SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, size_t, count)
> >> {
> >> ...
> >> loff_t pos = file_pos_read(file);
> >> ret = vfs_read(file, buf, count, &pos);
> >> file_pos_write(file, pos);
> >> fput_light(file, fput_needed);
> >> ...
> >> }
> >>
> >> While readdir() is protected with i_mutex, f_pos can be changed without
> >> any locking in various read()/write() syscalls, which leads to this bug.
> >>
> >> What makes things worse is Andi removed i_mutex from generic_file_llseek,
> >> so you can trigger the same bug by replacing read() with lseek() in the
> >> test program.
> > Yes, and here I'd say it's a filesystem issue. If filesystem needs f_pos
> > changed only under i_mutex, it should use default_llseek() or get the mutex
> > itself. That's what the callback is for. We shouldn't unnecessarily impose
> > the i_mutex restriction on llseek on a directory for every filesystem.
>
> One of my concern is, concurrent lseek() and readdir() doesn't seem to be
> well tested. I'll add a test case in xfstests.
Yes, that might be a useful test to add.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2013/2/19 20:59, Jan Kara wrote:
> On Tue 19-02-13 19:47:30, Li Zefan wrote:
>> On 2013/2/19 17:19, Jan Kara wrote:
>>> On Tue 19-02-13 09:22:40, Li Zefan wrote:
>>>> There's a long long-standing bug...As long as I don't know when it dates
>>>> from.
>>>>
>>>> I've written and attached a simple program to reproduce this bug, and it can
>>>> immediately trigger the bug in my box. It uses two threads, one keeps calling
>>>> read(), and the other calling readdir(), both on the same directory fd.
>>> So the fact that read() or even write() to fd opened O_RDONLY has *any*
>>> effect on f_pos looks really unexpected to me. I think we really should
>>> have there:
>>> if (ret >= 0)
>>> file_pos_write(...);
>>
>> I thought about this. The problem is then we have to check every fop->write()
>> to see if any of them can return -errno with file->f_pos changed and fix them,
>> though it's do-able.
> But returning error and advancing f_pos would be a bug - specification
> says write() returns the number of bytes written or -1 and f_pos should be
> advanced by the number of bytes written.
>
Oh, I had an illusion that vfs saves f_pos and calls write() and restore f_pos
if write() fails.
On Tue, Feb 19, 2013 at 09:22:40AM +0800, Li Zefan wrote:
> There's a long long-standing bug...As long as I don't know when it dates
> from.
>
> I've written and attached a simple program to reproduce this bug, and it can
> immediately trigger the bug in my box. It uses two threads, one keeps calling
> read(), and the other calling readdir(), both on the same directory fd.
> While readdir() is protected with i_mutex, f_pos can be changed without any locking
> in various read()/write() syscalls, which leads to this bug.
_What_ read/write syscalls (on a directory, that is)?
> We can fix this bug in each filesystem, but can't we just make sure i_mutex is
> acquired in lseek(), read(), write() and readdir() for directory file operations?
>
> (the patch is for demonstration only)
No. This is a very massive overkill. If anything, we want to *reduce* the
amount of time we hold ->i_mutex in that area.
There are several bugs mixed here:
* disappearing exclusion between readdir and lseek for directories.
Bad, since offset validation suddenly needs to be redone every time we look
at ->f_pos in ->readdir() instances *and* since ->readdir() itself updates
position, often by file->f_pos += something.
* write(2) doing "get a copy of f_pos, try and fail ->write(),
put that copy back". With no locking whatsoever. What we get is a f_pos
value reverting to what it used to be at some random earlier point. Makes
life really nasty for everything that updates ->f_pos, obviously.
* read(2) doing the same, *and* some directories apparently having
->read() now.
->readdir() part of that would be the simplest one - we need to stop messing
with ->f_pos and just pass an address of its copy, like we do for ->read()
et.al. Preserving the method prototype is not worth it and this particular
method has needed an overhaul of calling conventions for many reasons.
The issue with write(2) and friends is potentially nastier. I'm looking
at the ->f_pos users right now, and while the majority are ->readdir()
and ->llseek() instances, there's some stuff beyond that. Some of that is
done with struct file opened kernel-side and not accessible to userland;
those are safe (and often really ugly - see drivers/media/pci/cx25821/
hits for f_pos). Some are simply wrong - e.g. dev_mem_read()
(in drivers/net/wireless/ti/wlcore/debugfs.c) ignores *ppos value and uses
file->f_pos instead; wrong behaviour for ->read() instance. I'm about
20% through the list; so far everything seems to be possible to deal with
(especially if we add a couple of helpers for common lseek variants and
use existing generic_file_llseek_size()), so it might turn out to be
not a serious issue, but it's a potential minefield. Hell knows...
As for ->readdir(), I'd like to resurrect an old proposal to change the ABI
of that sucker. Quoting the thread from 4 years ago:
====
As for the locking... I toyed with one idea for a while: instead of passing
a callback and one of many completely different structs, how about a common
*beginning* of a struct, with callback stored in it along with several
common fields? Namely,
* count of filldir calls already made
* pointer to file in question
* "are we done" flag
And instead of calling filldir(buf, ...) ->readdir() would call one of several
helpers:
* readdir_emit(buf, ...) - obvious
* readdir_relax(buf) - called in a spot convenient for dropping
and retaking lock; returns whether we need to do revalidation.
* readdir_eof(buf) - end of directory
* maybe readdir_dot() and readdir_dotdot() - those are certainly
common enough
That's the obvious flagday stuff, but since we need to give serious beating
to most of the instances anyway... Might as well consider something in
that direction.
====
Back then it didn't go anywhere, but if we really go for change of calling
conventions (passing a pointer to copy of ->f_pos), it would make a lot of
sense, IMO. Note that ->i_mutex contention could be seriously relieved that
way - e.g. ext* would just call readdir_relax() at the block boundaries,
since those locations are always valid there, etc.
Comments?
>> We can fix this bug in each filesystem, but can't we just make sure i_mutex is
>> acquired in lseek(), read(), write() and readdir() for directory file operations?
>>
>> (the patch is for demonstration only)
>
> No. This is a very massive overkill. If anything, we want to *reduce* the
> amount of time we hold ->i_mutex in that area.
>
> There are several bugs mixed here:
> * disappearing exclusion between readdir and lseek for directories.
> Bad, since offset validation suddenly needs to be redone every time we look
> at ->f_pos in ->readdir() instances *and* since ->readdir() itself updates
> position, often by file->f_pos += something.
> * write(2) doing "get a copy of f_pos, try and fail ->write(),
> put that copy back". With no locking whatsoever. What we get is a f_pos
> value reverting to what it used to be at some random earlier point. Makes
> life really nasty for everything that updates ->f_pos, obviously.
> * read(2) doing the same, *and* some directories apparently having
> ->read() now.
>
> ->readdir() part of that would be the simplest one - we need to stop messing
> with ->f_pos and just pass an address of its copy, like we do for ->read()
> et.al. Preserving the method prototype is not worth it and this particular
> method has needed an overhaul of calling conventions for many reasons.
>
> The issue with write(2) and friends is potentially nastier. I'm looking
> at the ->f_pos users right now, and while the majority are ->readdir()
> and ->llseek() instances, there's some stuff beyond that. Some of that is
> done with struct file opened kernel-side and not accessible to userland;
> those are safe (and often really ugly - see drivers/media/pci/cx25821/
> hits for f_pos). Some are simply wrong - e.g. dev_mem_read()
> (in drivers/net/wireless/ti/wlcore/debugfs.c) ignores *ppos value and uses
> file->f_pos instead; wrong behaviour for ->read() instance. I'm about
> 20% through the list; so far everything seems to be possible to deal with
> (especially if we add a couple of helpers for common lseek variants and
> use existing generic_file_llseek_size()), so it might turn out to be
> not a serious issue, but it's a potential minefield. Hell knows...
>
> As for ->readdir(), I'd like to resurrect an old proposal to change the ABI
> of that sucker. Quoting the thread from 4 years ago:
> ====
> As for the locking... I toyed with one idea for a while: instead of passing
> a callback and one of many completely different structs, how about a common
> *beginning* of a struct, with callback stored in it along with several
> common fields? Namely,
> * count of filldir calls already made
> * pointer to file in question
> * "are we done" flag
> And instead of calling filldir(buf, ...) ->readdir() would call one of several
> helpers:
> * readdir_emit(buf, ...) - obvious
> * readdir_relax(buf) - called in a spot convenient for dropping
> and retaking lock; returns whether we need to do revalidation.
> * readdir_eof(buf) - end of directory
> * maybe readdir_dot() and readdir_dotdot() - those are certainly
> common enough
> That's the obvious flagday stuff, but since we need to give serious beating
> to most of the instances anyway... Might as well consider something in
> that direction.
> ====
>
> Back then it didn't go anywhere, but if we really go for change of calling
> conventions (passing a pointer to copy of ->f_pos), it would make a lot of
> sense, IMO. Note that ->i_mutex contention could be seriously relieved that
> way - e.g. ext* would just call readdir_relax() at the block boundaries,
> since those locations are always valid there, etc.
>
So there will be no lock to protect f_pos in read/write/llseek in your proposal.
Do we need to care about reading/writing fpos in 32 bit machine is not atomic?
> As for ->readdir(), I'd like to resurrect an old proposal to change the ABI
> of that sucker. Quoting the thread from 4 years ago:
I'd love to see the readdir() interface cleaned up, yes please.
> Comments?
Hmm. Do we want to think about letting callers copy the name to
userspace in fragments?
I was looking at current readdir paths and saw that btrfs has an
annoying wrinkle that its directory entries might be in large blocks
that span disjoint pages. So it copies them to a kmalloced()ed buffer
to pass to filldir().
Maybe it wouldn't be worth having to track incomplete entries in the
core, but it'd be nice to get rid of this copy in btrfs.
(btrfs could also be more clever and only do this dance when the name
really does span pages.. it's very rare. :/).
- z