Re: git: 610319c766e9 - main - ufs_vnops.c: newparent is not bool
Date: Tue, 29 Jul 2025 13:45:30 UTC
On Tue, Jul 29, 2025 at 09:16:23AM -0400, Mark Johnston wrote:
> On Tue, Jul 29, 2025 at 04:01:46PM +0300, Konstantin Belousov wrote:
> > On Tue, Jul 29, 2025 at 08:26:52AM -0400, Mark Johnston wrote:
> > > On Mon, Jul 28, 2025 at 08:57:21PM +0000, Konstantin Belousov wrote:
> > > > The branch main has been updated by kib:
> > > >
> > > > URL: https://cgit.FreeBSD.org/src/commit/?id=610319c766e941de96e52f2d28fea9f8cfc51aeb
> > > >
> > > > commit 610319c766e941de96e52f2d28fea9f8cfc51aeb
> > > > Author: Konstantin Belousov <kib@FreeBSD.org>
> > > > AuthorDate: 2025-07-27 13:50:57 +0000
> > > > Commit: Konstantin Belousov <kib@FreeBSD.org>
> > > > CommitDate: 2025-07-28 20:57:14 +0000
> > > >
> > > > ufs_vnops.c: newparent is not bool
> > > >
> > > > Use proper comparision operators when we need to see if newparent was
> > > > set to not-zero value.
> > > >
> > > > Reviewed by: mckusick, olce
> > > > Sponsored by: The FreeBSD Foundation
> > > > MFC after: 1 week
> > > > Differential revision: https://reviews.freebsd.org/D51573
> > > > ---
> > > > sys/ufs/ufs/ufs_vnops.c | 15 +++++++--------
> > > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
> > > > index 406b8f943077..2757fb066981 100644
> > > > --- a/sys/ufs/ufs/ufs_vnops.c
> > > > +++ b/sys/ufs/ufs/ufs_vnops.c
> > > > @@ -1476,7 +1476,7 @@ relock:
> > > > * the user must have write permission in the source so
> > > > * as to be able to change "..".
> > > > */
> > > > - if (doingdirectory && newparent) {
> > > > + if (doingdirectory && newparent != 0) {
> > > > error = VOP_ACCESS(fvp, VWRITE, tcnp->cn_cred, curthread);
> > > > if (error)
> > > > goto unlockout;
> > > > @@ -1539,7 +1539,7 @@ relock:
> > > > if (tip == NULL) {
> > > > if (ITODEV(tdp) != ITODEV(fip))
> > > > panic("ufs_rename: EXDEV");
> > > > - if (doingdirectory && newparent) {
> > > > + if (doingdirectory && newparent != 0) {
> > > > /*
> > > > * Account for ".." in new directory.
> > > > * When source and destination have the same
> > > > @@ -1632,7 +1632,7 @@ relock:
> > > > goto bad;
> > > > }
> > > > if (doingdirectory) {
> > > > - if (!newparent) {
> > > > + if (newparent == 0) {
> > > > tdp->i_effnlink--;
> > > > if (DOINGSOFTDEP(tdvp))
> > > > softdep_change_linkcnt(tdp);
> > > > @@ -1642,11 +1642,10 @@ relock:
> > > > softdep_change_linkcnt(tip);
> > > > }
> > > > error = ufs_dirrewrite(tdp, tip, fip->i_number,
> > > > - IFTODT(fip->i_mode),
> > > > - (doingdirectory && newparent) ? newparent : doingdirectory);
> > > > + IFTODT(fip->i_mode), doingdirectory);
> > >
> > > Is this part of the change correct?
> > >
> > > syzbot is reporting some panics after this change and commit
> > > c069ca085bd185eda4a90dc4bc2b76cceb74579d:
> > > https://syzkaller.appspot.com/bug?extid=18722c8e4008048efb51
> > > https://syzkaller.appspot.com/bug?extid=6a4ea1e13f4e07369785
> > > https://syzkaller.appspot.com/bug?extid=5cb82352555d5d505640
> > > https://syzkaller.appspot.com/bug?extid=602fb6ee1a39abfd3b5c
> > > https://syzkaller.appspot.com/bug?extid=fb35cce6a6f5075a6692
> > > https://syzkaller.appspot.com/bug?extid=6fb8cb919cc686d1a1d0
> > > https://syzkaller.appspot.com/bug?extid=98c39c45a437812f7683
> > > https://syzkaller.appspot.com/bug?extid=02cb048d48b51bcd9c41
> >
> > I committed the revert, with the Fixes tag, for now. But consider:
> >
> > Two cases:
> > 1. doingdirectory == true:
> > the expression can be rewritten as newparent ? newparent : true.
> > Then this is the same as true.
> >
> > 2. doingdirectory == false:
> > then the expression is false.
>
> I think the problem is that the isrmdir parameter to ufs_dirrewrite() is
> not a bool as the name implies. See softdep_setup_directory_change(),
> which checks isrmdir > 1.
Right. While phab is down, this should fix it, and another error with
the signess of the inum passed in isrmdir (the > 1 check would fail in
fact).
While the phab is broken, I am posting the fix there.
commit 897f2b45277def12bac6d87dd8a139a04970b8d7
Author: Konstantin Belousov <kib@FreeBSD.org>
Date: Tue Jul 29 16:35:17 2025 +0300
ufs: change isrmdir type to bool or u_int as appropriate
Use bool for isrmdir argument to
ufs_dirremove()/softdep_setup_remove()/newdirrem(), where it is used as
bool.
Use u_int for isrmdir argument to
ufs_dirrewrite()/softdep_setup_directory_change()
where it is 0/1/ino. Without the change to unsigned, the
if (isrmdir > 1)
test is broken on volumes with many inodes.
Noted by: markj
Fixes: 610319c766e941de96e52f2d28fea9f8cfc51aeb
Fixes: 98eb6f0eaa50d8bd9a6794f0a9da2eddeae5bcd8
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential revision:
diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c
index 3f4aec02ba49..9393938d60c5 100644
--- a/sys/ufs/ffs/ffs_softdep.c
+++ b/sys/ufs/ffs/ffs_softdep.c
@@ -274,7 +274,7 @@ void
softdep_setup_remove(struct buf *bp,
struct inode *dp,
struct inode *ip,
- int isrmdir)
+ bool isrmdir)
{
panic("softdep_setup_remove called");
@@ -285,7 +285,7 @@ softdep_setup_directory_change(struct buf *bp,
struct inode *dp,
struct inode *ip,
ino_t newinum,
- int isrmdir)
+ u_int isrmdir)
{
panic("softdep_setup_directory_change called");
@@ -765,7 +765,7 @@ static void initiate_write_inodeblock_ufs2(struct inodedep *, struct buf *);
static void handle_workitem_freefile(struct freefile *);
static int handle_workitem_remove(struct dirrem *, int);
static struct dirrem *newdirrem(struct buf *, struct inode *,
- struct inode *, int, struct dirrem **);
+ struct inode *, bool, struct dirrem **);
static struct indirdep *indirdep_lookup(struct mount *, struct inode *,
struct buf *);
static void cancel_indirdep(struct indirdep *, struct buf *,
@@ -9169,7 +9169,7 @@ softdep_setup_remove(
struct buf *bp, /* buffer containing directory block */
struct inode *dp, /* inode for the directory being modified */
struct inode *ip, /* inode for directory entry being removed */
- int isrmdir) /* indicates if doing RMDIR */
+ bool isrmdir) /* indicates if doing RMDIR */
{
struct dirrem *dirrem, *prevdirrem;
struct inodedep *inodedep;
@@ -9361,7 +9361,7 @@ newdirrem(
struct buf *bp, /* buffer containing directory block */
struct inode *dp, /* inode for the directory being modified */
struct inode *ip, /* inode for directory entry being removed */
- int isrmdir, /* indicates if doing RMDIR */
+ bool isrmdir, /* indicates if doing RMDIR */
struct dirrem **prevdirremp) /* previously referenced inode, if any */
{
int offset;
@@ -9490,7 +9490,7 @@ newdirrem(
dirrem->dm_state |= COMPLETE;
cancel_diradd(dap, dirrem, jremref, dotremref, dotdotremref);
#ifdef INVARIANTS
- if (isrmdir == 0) {
+ if (!isrmdir) {
struct worklist *wk;
LIST_FOREACH(wk, &dirrem->dm_jwork, wk_list)
@@ -9525,7 +9525,7 @@ softdep_setup_directory_change(
struct inode *dp, /* inode for the directory being modified */
struct inode *ip, /* inode for directory entry being removed */
ino_t newinum, /* new inode number for changed entry */
- int isrmdir) /* indicates if doing RMDIR */
+ u_int isrmdir) /* indicates if doing RMDIR */
{
int offset;
struct diradd *dap = NULL;
@@ -9558,7 +9558,7 @@ softdep_setup_directory_change(
/*
* Allocate a new dirrem and ACQUIRE_LOCK.
*/
- dirrem = newdirrem(bp, dp, ip, isrmdir, &prevdirrem);
+ dirrem = newdirrem(bp, dp, ip, isrmdir != 0, &prevdirrem);
pagedep = dirrem->dm_pagedep;
/*
* The possible values for isrmdir:
diff --git a/sys/ufs/ufs/ufs_extern.h b/sys/ufs/ufs/ufs_extern.h
index ccd9046a5fa8..111fb1cb40b3 100644
--- a/sys/ufs/ufs/ufs_extern.h
+++ b/sys/ufs/ufs/ufs_extern.h
@@ -66,8 +66,8 @@ void ufs_makedirentry(struct inode *, struct componentname *,
struct direct *);
int ufs_direnter(struct vnode *, struct vnode *, struct direct *,
struct componentname *, struct buf *);
-int ufs_dirremove(struct vnode *, struct inode *, int, int);
-int ufs_dirrewrite(struct inode *, struct inode *, ino_t, int, int);
+int ufs_dirremove(struct vnode *, struct inode *, int, bool);
+int ufs_dirrewrite(struct inode *, struct inode *, ino_t, int, u_int);
int ufs_lookup_ino(struct vnode *, struct vnode **, struct componentname *,
ino_t *);
int ufs_getlbns(struct vnode *, ufs2_daddr_t, struct indir *, int *);
@@ -93,9 +93,9 @@ int softdep_setup_directory_add(struct buf *, struct inode *, off_t,
ino_t, struct buf *, int);
void softdep_change_directoryentry_offset(struct buf *, struct inode *,
caddr_t, caddr_t, caddr_t, int);
-void softdep_setup_remove(struct buf *,struct inode *, struct inode *, int);
+void softdep_setup_remove(struct buf *,struct inode *, struct inode *, bool);
void softdep_setup_directory_change(struct buf *, struct inode *,
- struct inode *, ino_t, int);
+ struct inode *, ino_t, u_int);
void softdep_change_linkcnt(struct inode *);
int softdep_slowdown(struct vnode *);
void softdep_setup_create(struct inode *, struct inode *);
diff --git a/sys/ufs/ufs/ufs_lookup.c b/sys/ufs/ufs/ufs_lookup.c
index 3f9c95e934fc..4877ff0c4db5 100644
--- a/sys/ufs/ufs/ufs_lookup.c
+++ b/sys/ufs/ufs/ufs_lookup.c
@@ -1101,7 +1101,7 @@ ufs_direnter(struct vnode *dvp, struct vnode *tvp, struct direct *dirp,
* to the size of the previous entry.
*/
int
-ufs_dirremove(struct vnode *dvp, struct inode *ip, int flags, int isrmdir)
+ufs_dirremove(struct vnode *dvp, struct inode *ip, int flags, bool isrmdir)
{
struct inode *dp;
struct direct *ep, *rep;
@@ -1224,7 +1224,7 @@ ufs_dirremove(struct vnode *dvp, struct inode *ip, int flags, int isrmdir)
*/
int
ufs_dirrewrite(struct inode *dp, struct inode *oip, ino_t newinum, int newtype,
- int isrmdir)
+ u_int isrmdir)
{
struct buf *bp;
struct direct *ep;
diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
index 050b21c2be0b..ee2188baf28d 100644
--- a/sys/ufs/ufs/ufs_vnops.c
+++ b/sys/ufs/ufs/ufs_vnops.c
@@ -1051,7 +1051,7 @@ ufs_remove(
#ifdef UFS_GJOURNAL
ufs_gjournal_orphan(vp);
#endif
- error = ufs_dirremove(dvp, ip, ap->a_cnp->cn_flags, 0);
+ error = ufs_dirremove(dvp, ip, ap->a_cnp->cn_flags, false);
if (ip->i_nlink <= 0)
vp->v_vflag |= VV_NOSYNC;
if (IS_SNAPSHOT(ip)) {
@@ -1209,7 +1209,7 @@ ufs_whiteout(
#endif
cnp->cn_flags &= ~DOWHITEOUT;
- error = ufs_dirremove(dvp, NULL, cnp->cn_flags, 0);
+ error = ufs_dirremove(dvp, NULL, cnp->cn_flags, false);
break;
default:
panic("ufs_whiteout: unknown op");
@@ -1643,7 +1643,7 @@ ufs_rename(
}
error = ufs_dirrewrite(tdp, tip, fip->i_number,
IFTODT(fip->i_mode), (doingdirectory && newparent != 0) ?
- newparent != 0: doingdirectory);
+ newparent : doingdirectory);
if (error) {
if (doingdirectory) {
if (newparent == 0) {
@@ -1728,7 +1728,7 @@ ufs_rename(
"rename: missing .. entry");
cache_purge(fdvp);
}
- error = ufs_dirremove(fdvp, fip, fcnp->cn_flags, 0);
+ error = ufs_dirremove(fdvp, fip, fcnp->cn_flags, false);
/*
* The kern_renameat() looks up the fvp using the DELETE flag, which
* causes the removal of the name cache entry for fvp.
@@ -2309,7 +2309,7 @@ ufs_rmdir(
ip->i_effnlink--;
if (DOINGSOFTDEP(vp))
softdep_setup_rmdir(dp, ip);
- error = ufs_dirremove(dvp, ip, cnp->cn_flags, 1);
+ error = ufs_dirremove(dvp, ip, cnp->cn_flags, true);
if (error) {
dp->i_effnlink++;
ip->i_effnlink++;