Re: git: 610319c766e9 - main - ufs_vnops.c: newparent is not bool

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.