($INBOX_DIR/description missing)
 help / color / Atom feed
* [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case.
@ 2020-06-15 15:56 youngjun
  2020-06-15 16:45 ` Amir Goldstein
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: youngjun @ 2020-06-15 15:56 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-unionfs, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case.
  2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
@ 2020-06-15 16:45 ` Amir Goldstein
  2020-06-16  4:46 ` [PATCH] ovl: inode reference leak in ovl_is_inuse " youngjun
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2020-06-15 16:45 UTC (permalink / raw)
  To: youngjun; +Cc: Miklos Szeredi, overlayfs, linux-kernel

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
  2020-06-15 16:45 ` Amir Goldstein
@ 2020-06-16  4:46 ` youngjun
  2020-06-16  5:33   ` Amir Goldstein
  2020-06-16  6:57 ` [PATCH v3] " youngjun
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: youngjun @ 2020-06-16  4:46 UTC (permalink / raw)
  To: amir73il; +Cc: Miklos Szeredi, linux-unionfs, linux-kernel, 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-16  4:46 ` [PATCH] ovl: inode reference leak in ovl_is_inuse " youngjun
@ 2020-06-16  5:33   ` Amir Goldstein
  0 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2020-06-16  5:33 UTC (permalink / raw)
  To: youngjun; +Cc: Miklos Szeredi, overlayfs, linux-kernel

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
  2020-06-15 16:45 ` Amir Goldstein
  2020-06-16  4:46 ` [PATCH] ovl: inode reference leak in ovl_is_inuse " youngjun
@ 2020-06-16  6:57 ` youngjun
  2020-06-16  7:11   ` Amir Goldstein
  2020-06-16  8:30 ` [PATCH 4/4] " youngjun
  2020-06-16  8:33 ` [PATCH v4] " youngjun
  4 siblings, 1 reply; 9+ messages in thread
From: youngjun @ 2020-06-16  6:57 UTC (permalink / raw)
  To: amir73il; +Cc: linux-unionfs, youngjun, stable

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-16  6:57 ` [PATCH v3] " youngjun
@ 2020-06-16  7:11   ` Amir Goldstein
  0 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2020-06-16  7:11 UTC (permalink / raw)
  To: youngjun; +Cc: overlayfs, stable

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
                   ` (2 preceding siblings ...)
  2020-06-16  6:57 ` [PATCH v3] " youngjun
@ 2020-06-16  8:30 ` youngjun
  2020-07-14 14:40   ` Miklos Szeredi
  2020-06-16  8:33 ` [PATCH v4] " youngjun
  4 siblings, 1 reply; 9+ messages in thread
From: youngjun @ 2020-06-16  8:30 UTC (permalink / raw)
  To: amir73il; +Cc: linux-unionfs, youngjun, stable

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
                   ` (3 preceding siblings ...)
  2020-06-16  8:30 ` [PATCH 4/4] " youngjun
@ 2020-06-16  8:33 ` youngjun
  4 siblings, 0 replies; 9+ messages in thread
From: youngjun @ 2020-06-16  8:33 UTC (permalink / raw)
  To: amir73il; +Cc: linux-unionfs, youngjun, stable

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] ovl: inode reference leak in ovl_is_inuse true case.
  2020-06-16  8:30 ` [PATCH 4/4] " youngjun
@ 2020-07-14 14:40   ` Miklos Szeredi
  0 siblings, 0 replies; 9+ messages in thread
From: Miklos Szeredi @ 2020-07-14 14:40 UTC (permalink / raw)
  To: youngjun; +Cc: Amir Goldstein, overlayfs, stable

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 15:56 [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case youngjun
2020-06-15 16:45 ` Amir Goldstein
2020-06-16  4:46 ` [PATCH] ovl: inode reference leak in ovl_is_inuse " youngjun
2020-06-16  5:33   ` Amir Goldstein
2020-06-16  6:57 ` [PATCH v3] " youngjun
2020-06-16  7:11   ` Amir Goldstein
2020-06-16  8:30 ` [PATCH 4/4] " youngjun
2020-07-14 14:40   ` Miklos Szeredi
2020-06-16  8:33 ` [PATCH v4] " youngjun

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-unionfs/0 linux-unionfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-unionfs linux-unionfs/ https://lore.kernel.org/linux-unionfs \
		linux-unionfs@vger.kernel.org
	public-inbox-index linux-unionfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-unionfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git