On Mon, Sep 4, 2023 at 6:03 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Mon, 4 Sept 2023 at 16:47, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> # v5.6
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxx/
> >
> > 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.
