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

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.