Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 29 Jul 2025 17:10:18 +0300
From:      Konstantin Belousov <kostikbel@gmail.com>
To:        Mark Johnston <markj@freebsd.org>, mckusick@freebsd.org
Cc:        src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org
Subject:   Re: git: 610319c766e9 - main - ufs_vnops.c: newparent is not bool
Message-ID:  <aIjWSjBxuC5RnVwy@kib.kiev.ua>
In-Reply-To: <aIjS4rG-fBgQYOzA@nuc>
References:  <202507282057.56SKvL6x019403@gitrepo.freebsd.org> <aIi-DIIrxOpZKtBD@nuc> <aIjGOssFkCTP2xHI@kib.kiev.ua> <aIjJp7uE_KbHB7jF@nuc> <aIjQegd9HFhTXRfY@kib.kiev.ua> <aIjS4rG-fBgQYOzA@nuc>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jul 29, 2025 at 09:55:46AM -0400, Mark Johnston wrote:
> On Tue, Jul 29, 2025 at 04:45:30PM +0300, Konstantin Belousov wrote:
> > 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.
> 
> This looks right to me.  I quickly tested it with the reproducers from
> syzbot and didn't see any problems.
> 
> I'd also rename non-bool isrmdir to something like "newparent", just to
> make it a bit clearer that it's not a boolean value.

commit 6dfbc1e9bde38761f0b2267732bf690640cece0b
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.
    Use newparent instead of isrmdir for the argument name in this case.
    
    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..67cd6fb4b738 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 newparent)
 {
 
 	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 newparent)	/* indicates if doing RMDIR */
 {
 	int offset;
 	struct diradd *dap = NULL;
@@ -9558,10 +9558,10 @@ softdep_setup_directory_change(
 	/*
 	 * Allocate a new dirrem and ACQUIRE_LOCK.
 	 */
-	dirrem = newdirrem(bp, dp, ip, isrmdir, &prevdirrem);
+	dirrem = newdirrem(bp, dp, ip, newparent != 0, &prevdirrem);
 	pagedep = dirrem->dm_pagedep;
 	/*
-	 * The possible values for isrmdir:
+	 * The possible values for newparent:
 	 *	0 - non-directory file rename
 	 *	1 - directory rename within same directory
 	 *   inum - directory rename to new directory of given inode number
@@ -9572,7 +9572,7 @@ softdep_setup_directory_change(
 	 * the DIRCHG flag to tell handle_workitem_remove to skip the 
 	 * followup dirrem.
 	 */
-	if (isrmdir > 1)
+	if (newparent > 1)
 		dirrem->dm_state |= DIRCHG;
 
 	/*
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..fd0539c40c0d 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 newparent)
 {
 	struct buf *bp;
 	struct direct *ep;
@@ -1267,7 +1267,8 @@ ufs_dirrewrite(struct inode *dp, struct inode *oip, ino_t newinum, int newtype,
 	if (!OFSFMT(vdp))
 		ep->d_type = newtype;
 	if (DOINGSOFTDEP(vdp)) {
-		softdep_setup_directory_change(bp, dp, oip, newinum, isrmdir);
+		softdep_setup_directory_change(bp, dp, oip, newinum,
+		    newparent);
 		bdwrite(bp);
 	} else {
 		if (DOINGASYNC(vdp)) {
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++;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?aIjWSjBxuC5RnVwy>