cifs: fix page reference leak with readv/writev
diff mbox series

Message ID 20190410193747.25302-1-jglisse@redhat.com
State New
Headers show
Series
  • cifs: fix page reference leak with readv/writev
Related show

Commit Message

Jerome Glisse April 10, 2019, 7:37 p.m. UTC
From: Jérôme Glisse <jglisse@redhat.com>

CIFS can leak pages reference gotten through GUP (get_user_pages*()
through iov_iter_get_pages()). This happen if cifs_send_async_read()
or cifs_write_from_iter() calls fail from within __cifs_readv() and
__cifs_writev() respectively. This patch move page unreference to
cifs_aio_ctx_release() which will happens on all code paths this is
all simpler to follow for correctness.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Steve French <sfrench@samba.org>
Cc: linux-cifs@vger.kernel.org
Cc: samba-technical@lists.samba.org
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Stable <stable@vger.kernel.org>
---
 fs/cifs/file.c | 15 +--------------
 fs/cifs/misc.c | 23 ++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 15 deletions(-)

Comments

Steve French April 11, 2019, 2:47 a.m. UTC | #1
How was this discovered? Does it address a reported user problem?

On Wed, Apr 10, 2019 at 2:38 PM <jglisse@redhat.com> wrote:
>
> From: Jérôme Glisse <jglisse@redhat.com>
>
> CIFS can leak pages reference gotten through GUP (get_user_pages*()
> through iov_iter_get_pages()). This happen if cifs_send_async_read()
> or cifs_write_from_iter() calls fail from within __cifs_readv() and
> __cifs_writev() respectively. This patch move page unreference to
> cifs_aio_ctx_release() which will happens on all code paths this is
> all simpler to follow for correctness.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Steve French <sfrench@samba.org>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Stable <stable@vger.kernel.org>
> ---
>  fs/cifs/file.c | 15 +--------------
>  fs/cifs/misc.c | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 89006e044973..a756a4d3f70f 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2858,7 +2858,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
>         struct cifs_tcon *tcon;
>         struct cifs_sb_info *cifs_sb;
>         struct dentry *dentry = ctx->cfile->dentry;
> -       unsigned int i;
>         int rc;
>
>         tcon = tlink_tcon(ctx->cfile->tlink);
> @@ -2922,10 +2921,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
>                 kref_put(&wdata->refcount, cifs_uncached_writedata_release);
>         }
>
> -       if (!ctx->direct_io)
> -               for (i = 0; i < ctx->npages; i++)
> -                       put_page(ctx->bv[i].bv_page);
> -
>         cifs_stats_bytes_written(tcon, ctx->total_len);
>         set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);
>
> @@ -3563,7 +3558,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
>         struct iov_iter *to = &ctx->iter;
>         struct cifs_sb_info *cifs_sb;
>         struct cifs_tcon *tcon;
> -       unsigned int i;
>         int rc;
>
>         tcon = tlink_tcon(ctx->cfile->tlink);
> @@ -3647,15 +3641,8 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
>                 kref_put(&rdata->refcount, cifs_uncached_readdata_release);
>         }
>
> -       if (!ctx->direct_io) {
> -               for (i = 0; i < ctx->npages; i++) {
> -                       if (ctx->should_dirty)
> -                               set_page_dirty(ctx->bv[i].bv_page);
> -                       put_page(ctx->bv[i].bv_page);
> -               }
> -
> +       if (!ctx->direct_io)
>                 ctx->total_len = ctx->len - iov_iter_count(to);
> -       }
>
>         /* mask nodata case */
>         if (rc == -ENODATA)
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index bee203055b30..9bc0d17a9d77 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -768,6 +768,11 @@ cifs_aio_ctx_alloc(void)
>  {
>         struct cifs_aio_ctx *ctx;
>
> +       /*
> +        * Must use kzalloc to initialize ctx->bv to NULL and ctx->direct_io
> +        * to false so that we know when we have to unreference pages within
> +        * cifs_aio_ctx_release()
> +        */
>         ctx = kzalloc(sizeof(struct cifs_aio_ctx), GFP_KERNEL);
>         if (!ctx)
>                 return NULL;
> @@ -786,7 +791,23 @@ cifs_aio_ctx_release(struct kref *refcount)
>                                         struct cifs_aio_ctx, refcount);
>
>         cifsFileInfo_put(ctx->cfile);
> -       kvfree(ctx->bv);
> +
> +       /*
> +        * ctx->bv is only set if setup_aio_ctx_iter() was call successfuly
> +        * which means that iov_iter_get_pages() was a success and thus that
> +        * we have taken reference on pages.
> +        */
> +       if (ctx->bv) {
> +               unsigned i;
> +
> +               for (i = 0; i < ctx->npages; i++) {
> +                       if (ctx->should_dirty)
> +                               set_page_dirty(ctx->bv[i].bv_page);
> +                       put_page(ctx->bv[i].bv_page);
> +               }
> +               kvfree(ctx->bv);
> +       }
> +
>         kfree(ctx);
>  }
>
> --
> 2.20.1
>
Jerome Glisse April 18, 2019, 9:12 p.m. UTC | #2
On Wed, Apr 10, 2019 at 09:47:01PM -0500, Steve French wrote:
> How was this discovered? Does it address a reported user problem?

I have spotted it while tracking down how page reference are taken
for bio and how they are release. In the current code, once the page
are GUPed they are never release if there is any failure, on failure
cifs_aio_ctx_release() will be call but it will just free the bio_vec
not release the page reference.

Page reference are only drop if everything is successful.

So this patch move the page reference droping to cifs_aio_ctx_release()
which is call from all code path including failure AFAICT and thus page
reference will be drop if failure does happen.

Cheers,
Jérôme

> 
> On Wed, Apr 10, 2019 at 2:38 PM <jglisse@redhat.com> wrote:
> >
> > From: Jérôme Glisse <jglisse@redhat.com>
> >
> > CIFS can leak pages reference gotten through GUP (get_user_pages*()
> > through iov_iter_get_pages()). This happen if cifs_send_async_read()
> > or cifs_write_from_iter() calls fail from within __cifs_readv() and
> > __cifs_writev() respectively. This patch move page unreference to
> > cifs_aio_ctx_release() which will happens on all code paths this is
> > all simpler to follow for correctness.
> >
> > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > Cc: Steve French <sfrench@samba.org>
> > Cc: linux-cifs@vger.kernel.org
> > Cc: samba-technical@lists.samba.org
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Stable <stable@vger.kernel.org>
> > ---
> >  fs/cifs/file.c | 15 +--------------
> >  fs/cifs/misc.c | 23 ++++++++++++++++++++++-
> >  2 files changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 89006e044973..a756a4d3f70f 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -2858,7 +2858,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> >         struct cifs_tcon *tcon;
> >         struct cifs_sb_info *cifs_sb;
> >         struct dentry *dentry = ctx->cfile->dentry;
> > -       unsigned int i;
> >         int rc;
> >
> >         tcon = tlink_tcon(ctx->cfile->tlink);
> > @@ -2922,10 +2921,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> >                 kref_put(&wdata->refcount, cifs_uncached_writedata_release);
> >         }
> >
> > -       if (!ctx->direct_io)
> > -               for (i = 0; i < ctx->npages; i++)
> > -                       put_page(ctx->bv[i].bv_page);
> > -
> >         cifs_stats_bytes_written(tcon, ctx->total_len);
> >         set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);
> >
> > @@ -3563,7 +3558,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> >         struct iov_iter *to = &ctx->iter;
> >         struct cifs_sb_info *cifs_sb;
> >         struct cifs_tcon *tcon;
> > -       unsigned int i;
> >         int rc;
> >
> >         tcon = tlink_tcon(ctx->cfile->tlink);
> > @@ -3647,15 +3641,8 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> >                 kref_put(&rdata->refcount, cifs_uncached_readdata_release);
> >         }
> >
> > -       if (!ctx->direct_io) {
> > -               for (i = 0; i < ctx->npages; i++) {
> > -                       if (ctx->should_dirty)
> > -                               set_page_dirty(ctx->bv[i].bv_page);
> > -                       put_page(ctx->bv[i].bv_page);
> > -               }
> > -
> > +       if (!ctx->direct_io)
> >                 ctx->total_len = ctx->len - iov_iter_count(to);
> > -       }
> >
> >         /* mask nodata case */
> >         if (rc == -ENODATA)
> > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > index bee203055b30..9bc0d17a9d77 100644
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -768,6 +768,11 @@ cifs_aio_ctx_alloc(void)
> >  {
> >         struct cifs_aio_ctx *ctx;
> >
> > +       /*
> > +        * Must use kzalloc to initialize ctx->bv to NULL and ctx->direct_io
> > +        * to false so that we know when we have to unreference pages within
> > +        * cifs_aio_ctx_release()
> > +        */
> >         ctx = kzalloc(sizeof(struct cifs_aio_ctx), GFP_KERNEL);
> >         if (!ctx)
> >                 return NULL;
> > @@ -786,7 +791,23 @@ cifs_aio_ctx_release(struct kref *refcount)
> >                                         struct cifs_aio_ctx, refcount);
> >
> >         cifsFileInfo_put(ctx->cfile);
> > -       kvfree(ctx->bv);
> > +
> > +       /*
> > +        * ctx->bv is only set if setup_aio_ctx_iter() was call successfuly
> > +        * which means that iov_iter_get_pages() was a success and thus that
> > +        * we have taken reference on pages.
> > +        */
> > +       if (ctx->bv) {
> > +               unsigned i;
> > +
> > +               for (i = 0; i < ctx->npages; i++) {
> > +                       if (ctx->should_dirty)
> > +                               set_page_dirty(ctx->bv[i].bv_page);
> > +                       put_page(ctx->bv[i].bv_page);
> > +               }
> > +               kvfree(ctx->bv);
> > +       }
> > +
> >         kfree(ctx);
> >  }
> >
> > --
> > 2.20.1
> >
> 
> 
> -- 
> Thanks,
> 
> Steve
Pavel Shilovsky April 18, 2019, 9:45 p.m. UTC | #3
чт, 18 апр. 2019 г. в 14:12, Jerome Glisse <jglisse@redhat.com>:
>
> On Wed, Apr 10, 2019 at 09:47:01PM -0500, Steve French wrote:
> > How was this discovered? Does it address a reported user problem?
>
> I have spotted it while tracking down how page reference are taken
> for bio and how they are release. In the current code, once the page
> are GUPed they are never release if there is any failure, on failure
> cifs_aio_ctx_release() will be call but it will just free the bio_vec
> not release the page reference.
>
> Page reference are only drop if everything is successful.
>
> So this patch move the page reference droping to cifs_aio_ctx_release()
> which is call from all code path including failure AFAICT and thus page
> reference will be drop if failure does happen.
>
> Cheers,
> Jérôme
>
> >
> > On Wed, Apr 10, 2019 at 2:38 PM <jglisse@redhat.com> wrote:
> > >
> > > From: Jérôme Glisse <jglisse@redhat.com>
> > >
> > > CIFS can leak pages reference gotten through GUP (get_user_pages*()
> > > through iov_iter_get_pages()). This happen if cifs_send_async_read()
> > > or cifs_write_from_iter() calls fail from within __cifs_readv() and
> > > __cifs_writev() respectively. This patch move page unreference to
> > > cifs_aio_ctx_release() which will happens on all code paths this is
> > > all simpler to follow for correctness.
> > >
> > > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > > Cc: Steve French <sfrench@samba.org>
> > > Cc: linux-cifs@vger.kernel.org
> > > Cc: samba-technical@lists.samba.org
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > Cc: Stable <stable@vger.kernel.org>
> > > ---
> > >  fs/cifs/file.c | 15 +--------------
> > >  fs/cifs/misc.c | 23 ++++++++++++++++++++++-
> > >  2 files changed, 23 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 89006e044973..a756a4d3f70f 100644
> > > --- a/fs/cifs/file.c
> > > +++ b/fs/cifs/file.c
> > > @@ -2858,7 +2858,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> > >         struct cifs_tcon *tcon;
> > >         struct cifs_sb_info *cifs_sb;
> > >         struct dentry *dentry = ctx->cfile->dentry;
> > > -       unsigned int i;
> > >         int rc;
> > >
> > >         tcon = tlink_tcon(ctx->cfile->tlink);
> > > @@ -2922,10 +2921,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> > >                 kref_put(&wdata->refcount, cifs_uncached_writedata_release);
> > >         }
> > >
> > > -       if (!ctx->direct_io)
> > > -               for (i = 0; i < ctx->npages; i++)
> > > -                       put_page(ctx->bv[i].bv_page);
> > > -
> > >         cifs_stats_bytes_written(tcon, ctx->total_len);
> > >         set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);
> > >
> > > @@ -3563,7 +3558,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> > >         struct iov_iter *to = &ctx->iter;
> > >         struct cifs_sb_info *cifs_sb;
> > >         struct cifs_tcon *tcon;
> > > -       unsigned int i;
> > >         int rc;
> > >
> > >         tcon = tlink_tcon(ctx->cfile->tlink);
> > > @@ -3647,15 +3641,8 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> > >                 kref_put(&rdata->refcount, cifs_uncached_readdata_release);
> > >         }
> > >
> > > -       if (!ctx->direct_io) {
> > > -               for (i = 0; i < ctx->npages; i++) {
> > > -                       if (ctx->should_dirty)
> > > -                               set_page_dirty(ctx->bv[i].bv_page);
> > > -                       put_page(ctx->bv[i].bv_page);
> > > -               }
> > > -
> > > +       if (!ctx->direct_io)
> > >                 ctx->total_len = ctx->len - iov_iter_count(to);
> > > -       }
> > >
> > >         /* mask nodata case */
> > >         if (rc == -ENODATA)
> > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > > index bee203055b30..9bc0d17a9d77 100644
> > > --- a/fs/cifs/misc.c
> > > +++ b/fs/cifs/misc.c
> > > @@ -768,6 +768,11 @@ cifs_aio_ctx_alloc(void)
> > >  {
> > >         struct cifs_aio_ctx *ctx;
> > >
> > > +       /*
> > > +        * Must use kzalloc to initialize ctx->bv to NULL and ctx->direct_io
> > > +        * to false so that we know when we have to unreference pages within
> > > +        * cifs_aio_ctx_release()
> > > +        */
> > >         ctx = kzalloc(sizeof(struct cifs_aio_ctx), GFP_KERNEL);
> > >         if (!ctx)
> > >                 return NULL;
> > > @@ -786,7 +791,23 @@ cifs_aio_ctx_release(struct kref *refcount)
> > >                                         struct cifs_aio_ctx, refcount);
> > >
> > >         cifsFileInfo_put(ctx->cfile);
> > > -       kvfree(ctx->bv);
> > > +
> > > +       /*
> > > +        * ctx->bv is only set if setup_aio_ctx_iter() was call successfuly
> > > +        * which means that iov_iter_get_pages() was a success and thus that
> > > +        * we have taken reference on pages.
> > > +        */
> > > +       if (ctx->bv) {
> > > +               unsigned i;
> > > +
> > > +               for (i = 0; i < ctx->npages; i++) {
> > > +                       if (ctx->should_dirty)
> > > +                               set_page_dirty(ctx->bv[i].bv_page);
> > > +                       put_page(ctx->bv[i].bv_page);
> > > +               }
> > > +               kvfree(ctx->bv);
> > > +       }
> > > +
> > >         kfree(ctx);
> > >  }
> > >
> > > --
> > > 2.20.1
> > >

Good catch, thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky
Steve French April 21, 2019, 3:48 a.m. UTC | #4
updated patch in cifs-2.6.git for-next

On Thu, Apr 18, 2019 at 4:45 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> чт, 18 апр. 2019 г. в 14:12, Jerome Glisse <jglisse@redhat.com>:
> >
> > On Wed, Apr 10, 2019 at 09:47:01PM -0500, Steve French wrote:
> > > How was this discovered? Does it address a reported user problem?
> >
> > I have spotted it while tracking down how page reference are taken
> > for bio and how they are release. In the current code, once the page
> > are GUPed they are never release if there is any failure, on failure
> > cifs_aio_ctx_release() will be call but it will just free the bio_vec
> > not release the page reference.
> >
> > Page reference are only drop if everything is successful.
> >
> > So this patch move the page reference droping to cifs_aio_ctx_release()
> > which is call from all code path including failure AFAICT and thus page
> > reference will be drop if failure does happen.
> >
> > Cheers,
> > Jérôme
> >
> > >
> > > On Wed, Apr 10, 2019 at 2:38 PM <jglisse@redhat.com> wrote:
> > > >
> > > > From: Jérôme Glisse <jglisse@redhat.com>
> > > >
> > > > CIFS can leak pages reference gotten through GUP (get_user_pages*()
> > > > through iov_iter_get_pages()). This happen if cifs_send_async_read()
> > > > or cifs_write_from_iter() calls fail from within __cifs_readv() and
> > > > __cifs_writev() respectively. This patch move page unreference to
> > > > cifs_aio_ctx_release() which will happens on all code paths this is
> > > > all simpler to follow for correctness.
> > > >
> > > > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > > > Cc: Steve French <sfrench@samba.org>
> > > > Cc: linux-cifs@vger.kernel.org
> > > > Cc: samba-technical@lists.samba.org
> > > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > > Cc: linux-fsdevel@vger.kernel.org
> > > > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Cc: Stable <stable@vger.kernel.org>
> > > > ---
> > > >  fs/cifs/file.c | 15 +--------------
> > > >  fs/cifs/misc.c | 23 ++++++++++++++++++++++-
> > > >  2 files changed, 23 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > index 89006e044973..a756a4d3f70f 100644
> > > > --- a/fs/cifs/file.c
> > > > +++ b/fs/cifs/file.c
> > > > @@ -2858,7 +2858,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> > > >         struct cifs_tcon *tcon;
> > > >         struct cifs_sb_info *cifs_sb;
> > > >         struct dentry *dentry = ctx->cfile->dentry;
> > > > -       unsigned int i;
> > > >         int rc;
> > > >
> > > >         tcon = tlink_tcon(ctx->cfile->tlink);
> > > > @@ -2922,10 +2921,6 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
> > > >                 kref_put(&wdata->refcount, cifs_uncached_writedata_release);
> > > >         }
> > > >
> > > > -       if (!ctx->direct_io)
> > > > -               for (i = 0; i < ctx->npages; i++)
> > > > -                       put_page(ctx->bv[i].bv_page);
> > > > -
> > > >         cifs_stats_bytes_written(tcon, ctx->total_len);
> > > >         set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);
> > > >
> > > > @@ -3563,7 +3558,6 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> > > >         struct iov_iter *to = &ctx->iter;
> > > >         struct cifs_sb_info *cifs_sb;
> > > >         struct cifs_tcon *tcon;
> > > > -       unsigned int i;
> > > >         int rc;
> > > >
> > > >         tcon = tlink_tcon(ctx->cfile->tlink);
> > > > @@ -3647,15 +3641,8 @@ collect_uncached_read_data(struct cifs_aio_ctx *ctx)
> > > >                 kref_put(&rdata->refcount, cifs_uncached_readdata_release);
> > > >         }
> > > >
> > > > -       if (!ctx->direct_io) {
> > > > -               for (i = 0; i < ctx->npages; i++) {
> > > > -                       if (ctx->should_dirty)
> > > > -                               set_page_dirty(ctx->bv[i].bv_page);
> > > > -                       put_page(ctx->bv[i].bv_page);
> > > > -               }
> > > > -
> > > > +       if (!ctx->direct_io)
> > > >                 ctx->total_len = ctx->len - iov_iter_count(to);
> > > > -       }
> > > >
> > > >         /* mask nodata case */
> > > >         if (rc == -ENODATA)
> > > > diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> > > > index bee203055b30..9bc0d17a9d77 100644
> > > > --- a/fs/cifs/misc.c
> > > > +++ b/fs/cifs/misc.c
> > > > @@ -768,6 +768,11 @@ cifs_aio_ctx_alloc(void)
> > > >  {
> > > >         struct cifs_aio_ctx *ctx;
> > > >
> > > > +       /*
> > > > +        * Must use kzalloc to initialize ctx->bv to NULL and ctx->direct_io
> > > > +        * to false so that we know when we have to unreference pages within
> > > > +        * cifs_aio_ctx_release()
> > > > +        */
> > > >         ctx = kzalloc(sizeof(struct cifs_aio_ctx), GFP_KERNEL);
> > > >         if (!ctx)
> > > >                 return NULL;
> > > > @@ -786,7 +791,23 @@ cifs_aio_ctx_release(struct kref *refcount)
> > > >                                         struct cifs_aio_ctx, refcount);
> > > >
> > > >         cifsFileInfo_put(ctx->cfile);
> > > > -       kvfree(ctx->bv);
> > > > +
> > > > +       /*
> > > > +        * ctx->bv is only set if setup_aio_ctx_iter() was call successfuly
> > > > +        * which means that iov_iter_get_pages() was a success and thus that
> > > > +        * we have taken reference on pages.
> > > > +        */
> > > > +       if (ctx->bv) {
> > > > +               unsigned i;
> > > > +
> > > > +               for (i = 0; i < ctx->npages; i++) {
> > > > +                       if (ctx->should_dirty)
> > > > +                               set_page_dirty(ctx->bv[i].bv_page);
> > > > +                       put_page(ctx->bv[i].bv_page);
> > > > +               }
> > > > +               kvfree(ctx->bv);
> > > > +       }
> > > > +
> > > >         kfree(ctx);
> > > >  }
> > > >
> > > > --
> > > > 2.20.1
> > > >
>
> Good catch, thanks!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky

Patch
diff mbox series

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 89006e044973..a756a4d3f70f 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2858,7 +2858,6 @@  static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
 	struct cifs_tcon *tcon;
 	struct cifs_sb_info *cifs_sb;
 	struct dentry *dentry = ctx->cfile->dentry;
-	unsigned int i;
 	int rc;
 
 	tcon = tlink_tcon(ctx->cfile->tlink);
@@ -2922,10 +2921,6 @@  static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
 		kref_put(&wdata->refcount, cifs_uncached_writedata_release);
 	}
 
-	if (!ctx->direct_io)
-		for (i = 0; i < ctx->npages; i++)
-			put_page(ctx->bv[i].bv_page);
-
 	cifs_stats_bytes_written(tcon, ctx->total_len);
 	set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(dentry->d_inode)->flags);
 
@@ -3563,7 +3558,6 @@  collect_uncached_read_data(struct cifs_aio_ctx *ctx)
 	struct iov_iter *to = &ctx->iter;
 	struct cifs_sb_info *cifs_sb;
 	struct cifs_tcon *tcon;
-	unsigned int i;
 	int rc;
 
 	tcon = tlink_tcon(ctx->cfile->tlink);
@@ -3647,15 +3641,8 @@  collect_uncached_read_data(struct cifs_aio_ctx *ctx)
 		kref_put(&rdata->refcount, cifs_uncached_readdata_release);
 	}
 
-	if (!ctx->direct_io) {
-		for (i = 0; i < ctx->npages; i++) {
-			if (ctx->should_dirty)
-				set_page_dirty(ctx->bv[i].bv_page);
-			put_page(ctx->bv[i].bv_page);
-		}
-
+	if (!ctx->direct_io)
 		ctx->total_len = ctx->len - iov_iter_count(to);
-	}
 
 	/* mask nodata case */
 	if (rc == -ENODATA)
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index bee203055b30..9bc0d17a9d77 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -768,6 +768,11 @@  cifs_aio_ctx_alloc(void)
 {
 	struct cifs_aio_ctx *ctx;
 
+	/*
+	 * Must use kzalloc to initialize ctx->bv to NULL and ctx->direct_io
+	 * to false so that we know when we have to unreference pages within
+	 * cifs_aio_ctx_release()
+	 */
 	ctx = kzalloc(sizeof(struct cifs_aio_ctx), GFP_KERNEL);
 	if (!ctx)
 		return NULL;
@@ -786,7 +791,23 @@  cifs_aio_ctx_release(struct kref *refcount)
 					struct cifs_aio_ctx, refcount);
 
 	cifsFileInfo_put(ctx->cfile);
-	kvfree(ctx->bv);
+
+	/*
+	 * ctx->bv is only set if setup_aio_ctx_iter() was call successfuly
+	 * which means that iov_iter_get_pages() was a success and thus that
+	 * we have taken reference on pages.
+	 */
+	if (ctx->bv) {
+		unsigned i;
+
+		for (i = 0; i < ctx->npages; i++) {
+			if (ctx->should_dirty)
+				set_page_dirty(ctx->bv[i].bv_page);
+			put_page(ctx->bv[i].bv_page);
+		}
+		kvfree(ctx->bv);
+	}
+
 	kfree(ctx);
 }