On 11/10/26 18:37, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
- [Low] ovl: ovl_lower_positive() returns false when ovl_override_creds() fails
- [Low] ovl: ovl_nlink_end() skips ovl_cleanup_index() on override failure
--
commit 8148285e45d307c93a9e362536fb412f0d9a0ee9
Author: Vladimir Riabchun <[email protected]>
ve/overlayfs: Override VE together with creds
This patch changes ovl_override_creds() to switch the VE together with the
creds. It returns an error for cross-CT access, and it converts all
callers to the new ovl_creds_desc / ovl_revert_creds() interface. The goal
is to fix a NULL pointer dereference seen in ovl_get_index_name_fh() when a
host task works on an overlayfs created inside a CT.
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
[ ... ]
@@ -1426,7 +1436,10 @@ bool ovl_lower_positive(struct dentry *dentry)
if (!ovl_dentry_upper(dentry))
return true;
- old_cred = ovl_override_creds(dentry->d_sb);
+ /* Ugh, hard - we can't pass an error from here. */
+ if (ovl_override_creds(dentry->d_sb, &old_cred))
+ return false;
+
[Severity: Low]
If ovl_override_creds() fails here (for example -EINVAL on a cross-CT
access), ovl_lower_positive() returns false, which means "no lower entry
exists". Is that the safe answer?
The callers use this value to decide whether a whiteout is needed:
ovl_do_remove()
bool lower_positive = ovl_lower_positive(dentry);
...
if (!lower_positive)
err = ovl_remove_upper(dentry, is_dir, &list);
else
err = ovl_remove_and_whiteout(dentry, &list);
ovl_rename()
if (ovl_lower_positive(old)) {
... flags |= RENAME_WHITEOUT; ...
A false result on error selects the plain remove, or skips
RENAME_WHITEOUT. A lower file could then show up again after the unlink.
The operation probably fails later, because ovl_copy_up_flags() and the
ovl_override_creds() call in ovl_do_remove() fail the same way for the
same VE pair. That makes correctness depend on call ordering.
The lookup-error path in this same function already assumes "something is
there" and sets positive = true. Would returning true here be more
consistent and not depend on that ordering?
The problem is that failing ovl_override_creds means something is very
wrong, and we don't have a nice solutions. The users of
ovl_lower_positive are doing cred override later anyway, so they will
smell the problem.
It really shouldn't matter, I think.
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
[ ... ]
@@ -1216,11 +1243,15 @@ void ovl_nlink_end(struct dentry *dentry)
ovl_drop_write(dentry);
if (ovl_test_flag(OVL_INDEX, inode) && inode->i_nlink == 0) {
- const struct cred *old_cred;
+ struct ovl_creds_desc old_cred;
+ int err;
- old_cred = ovl_override_creds(dentry->d_sb);
- ovl_cleanup_index(dentry);
- revert_creds(old_cred);
+ err = ovl_override_creds(dentry->d_sb, &old_cred);
+ if (err)
+ pr_err("%s: skipping ovl_cleanup_index.\n", __func__);
+ else
+ ovl_cleanup_index(dentry);
+ ovl_revert_creds(&old_cred);
}
[Severity: Low]
ovl_nlink_end() is void and runs after the upper unlink or rename has
already succeeded. If ovl_override_creds() fails here, the orphan index
entry is left behind. The only trace is the pr_err(), and the caller is
never told.
Is this reachable? ovl_nlink_start() calls ovl_override_creds() earlier in
the same operation and would normally have failed first. If it is not
reachable in practice, would a WARN_ON_ONCE or a comment saying so be
clearer than a silent skip?
True, shouldn't be reachable. I'd keep this error check and warning,
this is not really important.
--
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
