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? > 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? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=5 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
