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.

Reply via email to