Re: [PATCH v3] ovl: inode reference leak in ovl_is_inuse true case.

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Jun 16, 2020 at 9:58 AM youngjun <her0gyugyu@gmail.com> wrote:
>
> When "ovl_is_inuse" true case, trap inode reference not put.
> plus adding the comment explaining sequence of
> ovl_is_inuse after ovl_setup_trap.
>
> Fixes: 0be0bfd2de9d ("ovl: fix regression caused by overlapping layers..")
> Cc: <stable@vger.kernel.org> # v4.19+
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: youngjun <her0gyugyu@gmail.com>
> > ---
> >  fs/overlayfs/super.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 91476bc422f9..0396793dadb8 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -1029,6 +1029,12 @@ static const struct xattr_handler *ovl_xattr_handlers[] = {
> >         NULL
> >  };
> >
> > +/*
> > + * Check if lower root conflicts with this overlay layers before checking
> > + * if it is in-use as upperdir/workdir of "another" mount, because we do
> > + * not bother to check in ovl_is_inuse() if the upperdir/workdir is in fact
> > + * in-use by our upperdir/workdir.
> > + */
>
> Signed-off-by: youngjun <her0gyugyu@gmail.com>
> ---
>  fs/overlayfs/super.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 91476bc422f9..3097142b1e23 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1493,14 +1493,22 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
>                 if (err < 0)
>                         goto out;
>
> +               /*
> +                * Check if lower root conflicts with this overlay layers before checking
> +                * if it is in-use as upperdir/workdir of "another" mount, because we do
> +                * not bother to check in ovl_is_inuse() if the upperdir/workdir is in fact
> +                * in-use by our upperdir/workdir.
> +                */
>                 err = ovl_setup_trap(sb, stack[i].dentry, &trap, "lowerdir");
>                 if (err)
>                         goto out;
>
>                 if (ovl_is_inuse(stack[i].dentry)) {
>                         err = ovl_report_in_use(ofs, "lowerdir");
> -                       if (err)
> +                       if (err) {
> +                               iput(trap);
>                                 goto out;
> +                       }
>                 }
>
>                 mnt = clone_private_mount(&stack[i]);
> --
> 2.17.1
>
> Great thanks Amir. guidance to me really helpful.
> As you comment, I modified my patch.
>
> 1) I revised three patch so version is 3.

Thanks good, but you should not reply to v2, you should post
a clean new patch, with revised commit message and change.

> 2) I misunderstood your comment(adding annotation) firstly.
> But, I clearly know at now and modified it.
> the annotation before "ovl_setup_trap" is clearly understood as I see.
>
> And I have some questions.
>
> 1) As I understand, The CC you said valid is 'linux-unionfs'?

Yes.
And as I understand there is no need actually CC the patch to
stable <stable@vger.kernel.org>, the text in commit message
is enough for their tools to pickup the patch  whenever it
gets merged.

> 2) The comment "Please add these lines to the bottom of commit message..."
>    Is it needed manually when I revised patch?

Yes, these lines are supposed to be there in the final merged commit.
Maintainers can add them, but it's nice to make life easier for busy
maintainers and add those line when you re-post the patch,
so the maintainer doesn't need to look it back in emails.

Thanks,
Amir.