On Mon, Jul 28, 2025 at 2:45 PM Antonio Quartulli <antonio@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 26/07/2025 20:27, Amir Goldstein wrote:
> > On Fri, Jul 25, 2025 at 10:33 AM Antonio Quartulli
> > <antonio@xxxxxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 21/07/2025 22:38, Antonio Quartulli wrote:
> >>> In case of ovl_lookup_temp() failure, we currently print `err`
> >>> which is actually not initialized at all.
> >>>
> >>> Instead, properly print PTR_ERR(whiteout) which is where the
> >>> actual error really is.
> >>>
> >>> Address-Coverity-ID: 1647983 ("Uninitialized variables (UNINIT)")
> >>> Fixes: 8afa0a7367138 ("ovl: narrow locking in ovl_whiteout()")
> >>> Signed-off-by: Antonio Quartulli <antonio@xxxxxxxxxxxxx>
> >>> ---
> >>> fs/overlayfs/dir.c | 5 +++--
> >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> >>> index 30619777f0f6..70b8687dc45e 100644
> >>> --- a/fs/overlayfs/dir.c
> >>> +++ b/fs/overlayfs/dir.c
> >>> @@ -117,8 +117,9 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> >>> if (!IS_ERR(whiteout))
> >>> return whiteout;
> >>> if (PTR_ERR(whiteout) != -EMLINK) {
> >>> - pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
> >>> - ofs->whiteout->d_inode->i_nlink, err);
> >>> + pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%lu)\n",
> >>
> >> while re-reading this patch, I realized that the format string for
> >> PTR_ERR(..) was supposed to be %ld, not %lu...
> >>
> >> Sorry about that :(
> >
> > No worries, but its not %ld either. the error is an int.
>
> PTR_ERR() returns long - this is what the patch is printing.
Doesn't matter what PTR_ERR returns, this is conceptually an int
error code.
>
> >
> >>
> >> Neil should I send yet another patch or maybe this can be sneaked into
> >> another change you are about to send?
> >
> > Please test this fix suggested by Neil and send a patch to Christian.
> >
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -116,10 +116,10 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> > inode_unlock(wdir);
> > if (!IS_ERR(whiteout))
> > return whiteout;
> > - if (PTR_ERR(whiteout) != -EMLINK) {
> > - pr_warn("Failed to link whiteout - disabling
> > whiteout inode sharing(nlink=%u, err=%lu)\n",
> > - ofs->whiteout->d_inode->i_nlink,
> > - PTR_ERR(whiteout));
> > + err = PTR_ERR(whiteout);
> > + if (err != -EMLINK) {
> > + pr_warn("Failed to link whiteout - disabling
> > whiteout inode sharing(nlink=%u, err=%i)\n",
> > + ofs->whiteout->d_inode->i_nlink, err);
> > ofs->no_shared_whiteout = true;
> > }
> > }
>
> Actually I think Neil was suggesting to make `err` local to the two
> blocks where it is currently used.
>
> This way the compiler would have caught its usage out of scope in the
> first place.
>
> It should be as listed below (including the format string fix).
> If you guys are fine with it, I'll send it as PATCH.
>
Sorry I do not like this suggestion.
I prefer not to reduce the scope of err var
and not have to define it twice.
Thanks,
Amir.
> Thanks!
>
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -80,7 +80,6 @@ struct dentry *ovl_lookup_temp(struct ovl_fs *ofs,
> struct dentry *workdir)
>
> static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> {
> - int err;
> struct dentry *whiteout;
> struct dentry *workdir = ofs->workdir;
> struct inode *wdir = workdir->d_inode;
> @@ -91,7 +90,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> inode_lock_nested(wdir, I_MUTEX_PARENT);
> whiteout = ovl_lookup_temp(ofs, workdir);
> if (!IS_ERR(whiteout)) {
> - err = ovl_do_whiteout(ofs, wdir, whiteout);
> + int err = ovl_do_whiteout(ofs, wdir, whiteout);
> if (err) {
> dput(whiteout);
> whiteout = ERR_PTR(err);
> @@ -107,7 +106,8 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> inode_lock_nested(wdir, I_MUTEX_PARENT);
> whiteout = ovl_lookup_temp(ofs, workdir);
> if (!IS_ERR(whiteout)) {
> - err = ovl_do_link(ofs, ofs->whiteout, wdir,
> whiteout);
> + int err = ovl_do_link(ofs, ofs->whiteout, wdir,
> + whiteout);
> if (err) {
> dput(whiteout);
> whiteout = ERR_PTR(err);
> @@ -117,7 +117,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
> if (!IS_ERR(whiteout))
> return whiteout;
> if (PTR_ERR(whiteout) != -EMLINK) {
> - pr_warn("Failed to link whiteout - disabling
> whiteout inode sharing(nlink=%u, err=%lu)\n",
> + pr_warn("Failed to link whiteout - disabling
> whiteout inode sharing(nlink=%u, err=%ld)\n",
> ofs->whiteout->d_inode->i_nlink,
> PTR_ERR(whiteout));
> ofs->no_shared_whiteout = true;
>
>
>
> --
> Antonio Quartulli
>
> CEO and Co-Founder
> Mandelbit Srl
> https://www.mandelbit.com
>
