| Message ID | 20200326170705.1552562-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "orangefs: remember count when reading." | expand |
Hi Christoph... Thanks for the patches... the revision of c2549f8c (which I dreamed up :-) ) might hose up some other stuff with our recent page cache revisions, though I have no doubt about your concerns... I'll post back after I've run some tests... -Mike On Thu, Mar 26, 2020 at 1:07 PM Christoph Hellwig <hch@lst.de> wrote: > > ->read_iter calls can race with each other and one or more ->flush calls. > Remove the the scheme to store the read count in the file private data > as is is completely racy and can cause use after free or double free > conditions. > > This reverts commit c2549f8c7a28c00facaf911f700c4811cfd6f52b. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/orangefs/file.c | 26 +------------------------- > fs/orangefs/orangefs-kernel.h | 4 ---- > 2 files changed, 1 insertion(+), 29 deletions(-) > > diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c > index c740159d9ad1..173e6ea57a47 100644 > --- a/fs/orangefs/file.c > +++ b/fs/orangefs/file.c > @@ -346,23 +346,8 @@ static ssize_t orangefs_file_read_iter(struct kiocb *iocb, > struct iov_iter *iter) > { > int ret; > - struct orangefs_read_options *ro; > - > orangefs_stats.reads++; > > - /* > - * Remember how they set "count" in read(2) or pread(2) or whatever - > - * users can use count as a knob to control orangefs io size and later > - * we can try to help them fill as many pages as possible in readpage. > - */ > - if (!iocb->ki_filp->private_data) { > - iocb->ki_filp->private_data = kmalloc(sizeof *ro, GFP_KERNEL); > - if (!iocb->ki_filp->private_data) > - return(ENOMEM); > - ro = iocb->ki_filp->private_data; > - ro->blksiz = iter->count; > - } > - > down_read(&file_inode(iocb->ki_filp)->i_rwsem); > ret = orangefs_revalidate_mapping(file_inode(iocb->ki_filp)); > if (ret) > @@ -650,12 +635,6 @@ static int orangefs_lock(struct file *filp, int cmd, struct file_lock *fl) > return rc; > } > > -static int orangefs_file_open(struct inode * inode, struct file *file) > -{ > - file->private_data = NULL; > - return generic_file_open(inode, file); > -} > - > static int orangefs_flush(struct file *file, fl_owner_t id) > { > /* > @@ -669,9 +648,6 @@ static int orangefs_flush(struct file *file, fl_owner_t id) > struct inode *inode = file->f_mapping->host; > int r; > > - kfree(file->private_data); > - file->private_data = NULL; > - > if (inode->i_state & I_DIRTY_TIME) { > spin_lock(&inode->i_lock); > inode->i_state &= ~I_DIRTY_TIME; > @@ -694,7 +670,7 @@ const struct file_operations orangefs_file_operations = { > .lock = orangefs_lock, > .unlocked_ioctl = orangefs_ioctl, > .mmap = orangefs_file_mmap, > - .open = orangefs_file_open, > + .open = generic_file_open, > .flush = orangefs_flush, > .release = orangefs_file_release, > .fsync = orangefs_fsync, > diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h > index ed67f39fa7ce..e12aeb9623d6 100644 > --- a/fs/orangefs/orangefs-kernel.h > +++ b/fs/orangefs/orangefs-kernel.h > @@ -239,10 +239,6 @@ struct orangefs_write_range { > kgid_t gid; > }; > > -struct orangefs_read_options { > - ssize_t blksiz; > -}; > - > extern struct orangefs_stats orangefs_stats; > > /* > -- > 2.25.1 >
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c index c740159d9ad1..173e6ea57a47 100644 --- a/fs/orangefs/file.c +++ b/fs/orangefs/file.c @@ -346,23 +346,8 @@ static ssize_t orangefs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter) { int ret; - struct orangefs_read_options *ro; - orangefs_stats.reads++; - /* - * Remember how they set "count" in read(2) or pread(2) or whatever - - * users can use count as a knob to control orangefs io size and later - * we can try to help them fill as many pages as possible in readpage. - */ - if (!iocb->ki_filp->private_data) { - iocb->ki_filp->private_data = kmalloc(sizeof *ro, GFP_KERNEL); - if (!iocb->ki_filp->private_data) - return(ENOMEM); - ro = iocb->ki_filp->private_data; - ro->blksiz = iter->count; - } - down_read(&file_inode(iocb->ki_filp)->i_rwsem); ret = orangefs_revalidate_mapping(file_inode(iocb->ki_filp)); if (ret) @@ -650,12 +635,6 @@ static int orangefs_lock(struct file *filp, int cmd, struct file_lock *fl) return rc; } -static int orangefs_file_open(struct inode * inode, struct file *file) -{ - file->private_data = NULL; - return generic_file_open(inode, file); -} - static int orangefs_flush(struct file *file, fl_owner_t id) { /* @@ -669,9 +648,6 @@ static int orangefs_flush(struct file *file, fl_owner_t id) struct inode *inode = file->f_mapping->host; int r; - kfree(file->private_data); - file->private_data = NULL; - if (inode->i_state & I_DIRTY_TIME) { spin_lock(&inode->i_lock); inode->i_state &= ~I_DIRTY_TIME; @@ -694,7 +670,7 @@ const struct file_operations orangefs_file_operations = { .lock = orangefs_lock, .unlocked_ioctl = orangefs_ioctl, .mmap = orangefs_file_mmap, - .open = orangefs_file_open, + .open = generic_file_open, .flush = orangefs_flush, .release = orangefs_file_release, .fsync = orangefs_fsync, diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h index ed67f39fa7ce..e12aeb9623d6 100644 --- a/fs/orangefs/orangefs-kernel.h +++ b/fs/orangefs/orangefs-kernel.h @@ -239,10 +239,6 @@ struct orangefs_write_range { kgid_t gid; }; -struct orangefs_read_options { - ssize_t blksiz; -}; - extern struct orangefs_stats orangefs_stats; /*
->read_iter calls can race with each other and one or more ->flush calls. Remove the the scheme to store the read count in the file private data as is is completely racy and can cause use after free or double free conditions. This reverts commit c2549f8c7a28c00facaf911f700c4811cfd6f52b. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/orangefs/file.c | 26 +------------------------- fs/orangefs/orangefs-kernel.h | 4 ---- 2 files changed, 1 insertion(+), 29 deletions(-)