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