On Tue, Jul 29, 2025 at 05:10:18PM +0300, Konstantin Belousov wrote: > 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 <k...@freebsd.org> > > > > > > > AuthorDate: 2025-07-27 13:50:57 +0000 > > > > > > > Commit: Konstantin Belousov <k...@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 <k...@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:
Looks ok to me.
