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@xxxxxx> 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@xxxxxx>
> ---
> 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
>
