All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mike Marshall <hubcap@omnibond.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Martin Brandenburg <martin@omnibond.com>,
	devel@lists.orangefs.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 1/2] Revert "orangefs: remember count when reading."
Date: Tue, 31 Mar 2020 09:55:09 -0400	[thread overview]
Message-ID: <CAOg9mST05zXRU1rmgTNdYPb93zHsO9N3TOMhuGGj8Q=6=_j=kw@mail.gmail.com> (raw)
In-Reply-To: <20200326170705.1552562-2-hch@lst.de>

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
>

  reply	other threads:[~2020-03-31 13:55 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-26 17:07 orangefs fixes Christoph Hellwig
2020-03-26 17:07 ` [PATCH 1/2] Revert "orangefs: remember count when reading." Christoph Hellwig
2020-03-31 13:55   ` Mike Marshall [this message]
2020-04-04 16:28   ` [PATCH] orangefs: complete Christoph's "remember count" reversion hubcap
2020-04-04 17:43     ` Matthew Wilcox
2020-04-04 20:57       ` Mike Marshall
2020-03-26 17:07 ` [PATCH 2/2] orangefs: don't mess with I_DIRTY_TIMES in orangefs_flush Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOg9mST05zXRU1rmgTNdYPb93zHsO9N3TOMhuGGj8Q=6=_j=kw@mail.gmail.com' \
    --to=hubcap@omnibond.com \
    --cc=devel@lists.orangefs.org \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=martin@omnibond.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.