Re: [PATCH] ovl: inode reference leak in ovl_is_inuse true case.
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 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: # v4.19+ Reviewed-by: Amir Goldstein > Signed-off-by: youngjun > --- > 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.
Re: [PATCH] ovl: inode reference leak in "ovl_is_inuse" true case.
On Mon, Jun 15, 2020 at 6:57 PM youngjun wrote: > > When "ovl_is_inuse" true case, trap inode reference not put. > > Signed-off-by: youngjun Fixes: 0be0bfd2de9d ("ovl: fix regression caused by overlapping layers detection") Cc: # v4.19+ Reviewed-by: Amir Goldstein > --- > 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.