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

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

[youngjun]

When "ovl_is_inuse" true case, trap inode reference not put.

Signed-off-by: youngjun <her0gyugyu@gmail.com>
---
 fs/overlayfs/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 91476bc422f9..8837fc1ec3be 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1499,8 +1499,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 
 		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

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

[Amir Goldstein]

On Mon, Jun 15, 2020 at 6:57 PM youngjun <her0gyugyu@gmail.com> wrote:
>
> When "ovl_is_inuse" true case, trap inode reference not put.
>
> Signed-off-by: youngjun <her0gyugyu@gmail.com>

Fixes: 0be0bfd2de9d ("ovl: fix regression caused by overlapping layers
detection")
Cc: <stable@vger.kernel.org> # v4.19+
Reviewed-by: Amir Goldstein <amir73il@gmail.com>


> ---
>  fs/overlayfs/super.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 91476bc422f9..8837fc1ec3be 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1499,8 +1499,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
>
>                 if (ovl_is_inuse(stack[i].dentry)) {
>                         err = ovl_report_in_use(ofs, "lowerdir");
> -                       if (err)
> +                       if (err) {
> +                               iput(trap);
>                                 goto out;
> +                       }
>                 }
>

Urgh! I moved this ovl_is_inuse() after ovl_setup_trap() for a reason, but I did
not explain why. While we are fixing the bug, it would be nice to add a comment
above ovl_setup_trap():

/*
 * 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.
 */

Thanks,
Amir.

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

[youngjun]

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.

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.
+ */
 static int ovl_setup_trap(struct super_block *sb, struct dentry *dir,
 			  struct inode **ptrap, const char *name)
 {
@@ -1499,8 +1505,10 @@ static int ovl_get_layers(struct super_block *sb, struct ovl_fs *ofs,
 
 		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

Thank you for comment Amir. I modified patch as you said.

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

[Amir Goldstein]

Hi youngjun!

Thank you for your patch.
You asked for guidance about posting patch revisions so let me repeat
my comment in a more clear way (see below).


On Tue, Jun 16, 2020 at 7:46 AM youngjun <her0gyugyu@gmail.com> wrote:
>

When posting a revision of a patch already posted, the practice
is to use the subject prefix [PATCH v2].
This will be auto generated for you with -v option for git format-patch.

Also, it is not valuable to CC LKML on patches with such a narrow
scope. The only relevant CC for this patch is the overlayfs list,
overlayfs maintainer and developers that reviewed v1 (me in that case).

> 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.
>

Please add these lines to the bottom of commit message:
(They help the stable tree maintainers know that patch
 should be picked up and to which stable tree)

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.
> + */

Sorry for not being clear about this comment.
I meant it should come before the call to ovl_setup_trap() in
ovl_get_layers(). It is not true in general that we always call ovl_setup_trap()
before ovl_is_inuse(). It is only true and relevant for checking lower layers.

If anything I wrote is not clear, do not hesitate to ask for more clarification.

Thanks,
Amir.

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

[youngjun]

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.
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'?
2) The comment "Please add these lines to the bottom of commit message..."
   Is it needed manually when I revised patch?

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

[Amir Goldstein]

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.

[PATCH 4/4] ovl: inode reference leak in ovl_is_inuse true case.

[youngjun]

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..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

Again, Great thanks Amir. I revise my patch through your kind guidance.

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

[youngjun]

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..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

Sorry. I Wrongly sent subect version. I changed it to v4.
Thank you Amir.

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

[Miklos Szeredi]

On Tue, Jun 16, 2020 at 10:30 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>

Thanks, applied.

Miklos