From nobody Tue Jul 29 14:27:24 2025 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4bryNf6Kk6z633TR; Tue, 29 Jul 2025 14:27:30 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qv1-xf30.google.com (mail-qv1-xf30.google.com [IPv6:2607:f8b0:4864:20::f30]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4bryNf1bBLz3LFd; Tue, 29 Jul 2025 14:27:30 +0000 (UTC) (envelope-from markjdb@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-qv1-xf30.google.com with SMTP id 6a1803df08f44-7074710a809so18262176d6.1; Tue, 29 Jul 2025 07:27:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1753799248; x=1754404048; darn=freebsd.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:sender:from:to:cc:subject:date:message-id :reply-to; bh=zYRVKscjHKwsXWKH9q5/YDir/JbKTrZ9K45yeAvGrpM=; b=RVH0XUE6EDik3DmbctyhZxxPA1vl01LCDsBep8+E0CEpZG75JHywX9OS+/nyWEAzqI q5prTgiGsJkFU/4Sj/WErKCnpZqEqu0eBMnUKcyal2SasD5LlWepbSV7yONP7axVIS5U k5idHKk/k+8LQrX8z0/s1nFecY8w6rieL0NDmjsVpHxauM5WFr1RJw5w1vBWObOpiI3E jc16Sdj56V0398nvuVArjSoUWKc65tNs2nXrEgEhiBJPSeO474gpeUm3z+XTu0MBbH3o +71SHQxs4epHIkWq7EzVAPkSeH5XHfIyIz3lCYMD/B2YTqQGWaLo6N6dI6mzZ8LqwQNM /b0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1753799248; x=1754404048; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:sender:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zYRVKscjHKwsXWKH9q5/YDir/JbKTrZ9K45yeAvGrpM=; b=hB4uvWSnKt4keyPrISv4V86x/73jdsRaQ1SRFFTS8dWwZybDaTd/Sb+PQDvLPHaHJF DmK7DZN0ITvTcUHvZGxWysv/VSDdHvrDB4cVm/GOgJ6UJ1Lw9TRLDP9UvkipihTwsdKX xYIhUYgdDLWbMCERaydFUzQytUdt43P6Mdk5b7MigUCPhFjNPpfy593QY4/XpyWaJaly xw6S2akXlAIgo8J5TxSdyzJHIS3WcExzIE5j7CZfXNGO3W1SJmuqTF0PsFMIwCDzL8Lk TGSMeU1baw9Jy18Tt22PEUofPIqQh7MjJyGFP0ypo2rXudF1hg/guqbyctfKJ4cfsYlD rdRA== X-Forwarded-Encrypted: i=1; AJvYcCVbhtwQ96GGTRsMD5woYDI72rZfto/ZeZW0oCrPVqOQyjbq1qM6VwSFbKweH6rLhp6ceurWW421yZPHqJ11aOI=@freebsd.org, AJvYcCWl1gxSJpzpSGDV5ZQAm2sdY3UHJgIR3OIJfxXlYto6fmjJt06sAbsFEIQcof+ArRhMSoQYyqzYkcmC7Yl7L3CurmLZDi8=@freebsd.org, AJvYcCXB3E3ZkFiM/POSoCEAlGTxhU8aPw36oTPnwbPsAkSF2te+cHNKLN3g0BZWYBh8sdjAOlrDHIVtg2p8X9Xpqk/AaGaB@freebsd.org X-Gm-Message-State: AOJu0YyLB9RVfHzswlLhAC3VA9sN+JAFpXxCtp/6SmfynDRJ6dYP9KZj gxowFcbyOo5LL1V+f9gaKoyDh0L4gpvxlzdRjakfGQWlYbZJMmJL5gLN X-Gm-Gg: ASbGncvboltPGq+mNamhlUfukTVsX+avSwEBxE6FtVGKR8wGx2FbfP/C3P8P//ezNqI 6Qu3pKlqi5kkWMbNk0dc3IBXPrsC7Oa77krJFGrblJCVEbVTd/4xzn5+SAV7ht/NCJol/Bcd/V6 THMCrltxuLowUyK0P8kcmkZOh5/rwsCHWQM4dzasZplmjZFPXG9VMzuyq9E9PDeJtJ9Yh2ZJmjK Dxlau4zeh42hOUeypQCPETHCUkKcf+QS63U2Q5FvynMl4dZJK7p3gTDGtL5YpYaBfX8bXgSVnT8 uImDMv6dyMFj34ZYXT6C49KKBOh1brJo1Mnx6+oYS/rpOUw72SRqj2IjHY+eaQrriuDOQ46/IRr a/JydJOYSVePNgVfE+moOsNjNJH9+Gj2P7NOU X-Google-Smtp-Source: AGHT+IEddwsuk4qeXDI5ugE0oGEW7OhgeBwoCmTL1nPtM+lWsCG2ZwITQli1FoE9D2Y1g0FrpvM0Gg== X-Received: by 2002:a05:6214:d88:b0:707:6087:72e5 with SMTP id 6a1803df08f44-70760877ba0mr19196486d6.35.1753799247460; Tue, 29 Jul 2025 07:27:27 -0700 (PDT) Received: from nuc (192-0-220-237.cpe.teksavvy.com. [192.0.220.237]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-7074c5f1b5asm22018336d6.24.2025.07.29.07.27.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Jul 2025 07:27:26 -0700 (PDT) Date: Tue, 29 Jul 2025 10:27:24 -0400 From: Mark Johnston To: Konstantin Belousov Cc: mckusick@freebsd.org, 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: References: <202507282057.56SKvL6x019403@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 4bryNf1bBLz3LFd X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] 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 > > > > > > > AuthorDate: 2025-07-27 13:50:57 +0000 > > > > > > > Commit: Konstantin Belousov > > > > > > > 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 > 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.