[PATCH] ovl: fix incorrect fdput() on aio completion

[PATCH] ovl: fix incorrect fdput() on aio completion

[Amir Goldstein]

ovl_{read,write}_iter() always call fdput(real) to put one or zero
refcounts of the real file, but for aio, whether it was submitted or not,
ovl_aio_put() also calls fdput(), which is not balanced.  This is only a
problem in the less common case when FDPUT_FPUT flag is set.

To fix the problem use get_file() to take file refcount and use fput()
instead of fdput() in ovl_aio_put().

Fixes: 2406a307ac7d ("ovl: implement async IO routines")
Cc: <stable@vger.kernel.org> # v5.6
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

This is the refcount leak fix that I found during work on backing_fs [1]
that deserves to be fast tracked into stable.

If it's ok with you, I will prepare a PR after rc1 including this
fix and the symlink fileattr fix.

Thanks,
Amir.

[1] https://lore.kernel.org/r/CAOQ4uxgzYevVCaGBjjckOr1vv0gKvVPYiOAL6E_KQY-YQx_7hg@mail.gmail.com/

 fs/overlayfs/file.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 3b4cc633d763..c743820e5c61 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -19,7 +19,6 @@ struct ovl_aio_req {
 	struct kiocb iocb;
 	refcount_t ref;
 	struct kiocb *orig_iocb;
-	struct fd fd;
 };
 
 static struct kmem_cache *ovl_aio_request_cachep;
@@ -280,7 +279,7 @@ static rwf_t ovl_iocb_to_rwf(int ifl)
 static inline void ovl_aio_put(struct ovl_aio_req *aio_req)
 {
 	if (refcount_dec_and_test(&aio_req->ref)) {
-		fdput(aio_req->fd);
+		fput(aio_req->iocb.ki_filp);
 		kmem_cache_free(ovl_aio_request_cachep, aio_req);
 	}
 }
@@ -342,7 +341,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		if (!aio_req)
 			goto out;
 
-		aio_req->fd = real;
+		get_file(real.file);
 		real.flags = 0;
 		aio_req->orig_iocb = iocb;
 		kiocb_clone(&aio_req->iocb, iocb, real.file);
@@ -409,7 +408,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 		if (!aio_req)
 			goto out;
 
-		aio_req->fd = real;
+		get_file(real.file);
 		real.flags = 0;
 		aio_req->orig_iocb = iocb;
 		kiocb_clone(&aio_req->iocb, iocb, real.file);
-- 
2.34.1

Re: [PATCH] ovl: fix incorrect fdput() on aio completion

[Miklos Szeredi]

On Mon, 4 Sept 2023 at 16:47, Amir Goldstein <amir73il@gmail.com> wrote:
>
> ovl_{read,write}_iter() always call fdput(real) to put one or zero
> refcounts of the real file, but for aio, whether it was submitted or not,
> ovl_aio_put() also calls fdput(), which is not balanced.  This is only a
> problem in the less common case when FDPUT_FPUT flag is set.
>
> To fix the problem use get_file() to take file refcount and use fput()
> instead of fdput() in ovl_aio_put().
>
> Fixes: 2406a307ac7d ("ovl: implement async IO routines")
> Cc: <stable@vger.kernel.org> # v5.6
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> This is the refcount leak fix that I found during work on backing_fs [1]
> that deserves to be fast tracked into stable.
>
> If it's ok with you, I will prepare a PR after rc1 including this
> fix and the symlink fileattr fix.

Looks good.

Thanks,
Miklos


>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/r/CAOQ4uxgzYevVCaGBjjckOr1vv0gKvVPYiOAL6E_KQY-YQx_7hg@mail.gmail.com/
>
>  fs/overlayfs/file.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 3b4cc633d763..c743820e5c61 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -19,7 +19,6 @@ struct ovl_aio_req {
>         struct kiocb iocb;
>         refcount_t ref;
>         struct kiocb *orig_iocb;
> -       struct fd fd;
>  };
>
>  static struct kmem_cache *ovl_aio_request_cachep;
> @@ -280,7 +279,7 @@ static rwf_t ovl_iocb_to_rwf(int ifl)
>  static inline void ovl_aio_put(struct ovl_aio_req *aio_req)
>  {
>         if (refcount_dec_and_test(&aio_req->ref)) {
> -               fdput(aio_req->fd);
> +               fput(aio_req->iocb.ki_filp);
>                 kmem_cache_free(ovl_aio_request_cachep, aio_req);
>         }
>  }
> @@ -342,7 +341,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>                 if (!aio_req)
>                         goto out;
>
> -               aio_req->fd = real;
> +               get_file(real.file);
>                 real.flags = 0;
>                 aio_req->orig_iocb = iocb;
>                 kiocb_clone(&aio_req->iocb, iocb, real.file);

It might be clearer to do the get_file() here:

+                 kiocb_clone(&aio_req->iocb, iocb, get_file(real.file));

Looks good otherwise.

Thanks,
Miklos

Re: [PATCH] ovl: fix incorrect fdput() on aio completion

[Amir Goldstein]

On Mon, Sep 4, 2023 at 6:03 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 4 Sept 2023 at 16:47, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > ovl_{read,write}_iter() always call fdput(real) to put one or zero
> > refcounts of the real file, but for aio, whether it was submitted or not,
> > ovl_aio_put() also calls fdput(), which is not balanced.  This is only a
> > problem in the less common case when FDPUT_FPUT flag is set.
> >
> > To fix the problem use get_file() to take file refcount and use fput()
> > instead of fdput() in ovl_aio_put().
> >
> > Fixes: 2406a307ac7d ("ovl: implement async IO routines")
> > Cc: <stable@vger.kernel.org> # v5.6
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos,
> >
> > This is the refcount leak fix that I found during work on backing_fs [1]
> > that deserves to be fast tracked into stable.
> >
> > If it's ok with you, I will prepare a PR after rc1 including this
> > fix and the symlink fileattr fix.
>
> Looks good.
>
> Thanks,
> Miklos
>
>
> >
> > Thanks,
> > Amir.
> >
> > [1] https://lore.kernel.org/r/CAOQ4uxgzYevVCaGBjjckOr1vv0gKvVPYiOAL6E_KQY-YQx_7hg@mail.gmail.com/
> >
> >  fs/overlayfs/file.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index 3b4cc633d763..c743820e5c61 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -19,7 +19,6 @@ struct ovl_aio_req {
> >         struct kiocb iocb;
> >         refcount_t ref;
> >         struct kiocb *orig_iocb;
> > -       struct fd fd;
> >  };
> >
> >  static struct kmem_cache *ovl_aio_request_cachep;
> > @@ -280,7 +279,7 @@ static rwf_t ovl_iocb_to_rwf(int ifl)
> >  static inline void ovl_aio_put(struct ovl_aio_req *aio_req)
> >  {
> >         if (refcount_dec_and_test(&aio_req->ref)) {
> > -               fdput(aio_req->fd);
> > +               fput(aio_req->iocb.ki_filp);
> >                 kmem_cache_free(ovl_aio_request_cachep, aio_req);
> >         }
> >  }
> > @@ -342,7 +341,7 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >                 if (!aio_req)
> >                         goto out;
> >
> > -               aio_req->fd = real;
> > +               get_file(real.file);
> >                 real.flags = 0;
> >                 aio_req->orig_iocb = iocb;
> >                 kiocb_clone(&aio_req->iocb, iocb, real.file);
>
> It might be clearer to do the get_file() here:
>
> +                 kiocb_clone(&aio_req->iocb, iocb, get_file(real.file));
>

Right. I will fix and stage.

> Looks good otherwise.
>

I will take it as Reviewed-by ;-)
and will do the same for symlink fileattr fix.

Thanks,
Amir.