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

On Tue, Jul 29, 2025 at 09:38:21AM -0400, Mark Johnston wrote:
> On Tue, Jul 29, 2025 at 04:22:56PM +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, then revert does not fix anything.
> 
> Indeed.  In the meantime, syzbot produced a reproducer:
> https://syzkaller.appspot.com/text?tag=ReproC&x=12fe9782580000
> 
> With the diff below, I no longer see any panics:
> 
> diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
> index 050b21c2be0b..b7453db9013c 100644
> --- a/sys/ufs/ufs/ufs_vnops.c
> +++ b/sys/ufs/ufs/ufs_vnops.c
> @@ -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) {

Go ahead if you want, my patch fixes more bugs there, I believe, and is
still needed.