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>