From nobody Tue Jul 29 13:38:21 2025 X-Original-To: dev-commits-src-all@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 4brxJ421mJz630ly; Tue, 29 Jul 2025 13:38:28 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qt1-x832.google.com (mail-qt1-x832.google.com [IPv6:2607:f8b0:4864:20::832]) (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 4brxJ35Fcpz3D1j; Tue, 29 Jul 2025 13:38:27 +0000 (UTC) (envelope-from markjdb@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-qt1-x832.google.com with SMTP id d75a77b69052e-4abc006bcadso63087981cf.0; Tue, 29 Jul 2025 06:38:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1753796305; x=1754401105; 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=DkSMTbVJtZ28aAxDo/6AKFMRgZH6cBGph/6DqauDKGQ=; b=aVGnlzYvoh/ORlgHc3BVjSN+O4Ah/GDwCxdhAsjDJ9U8L+TFTNXnYz1RkNYzpALUAW VTtwXKXYBjt+O2aO2H9ynnZfWyGYkhBhVKXRra2wjGRI8GogqNG94nBS8dYJ92ovynEh UMhbI/FQpMHLZ7bx/8ppKwQg4PQRSJw+u2lyaviYyEXZGpUX448j5+5zyaiKhXPFVFBz oDKGvWc8yFDlWQCTAhO95WNhgTkp2NDawt1mBRKJwcut40tt4egCmhgkiDApoHaUkkE2 LQbuQWUZ8gfcJkGqa+ib1vifbRUsevbLXjh+aSbSG2jMt5/gKnIeC+nvxuB4d/BHshuN /2aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1753796305; x=1754401105; 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=DkSMTbVJtZ28aAxDo/6AKFMRgZH6cBGph/6DqauDKGQ=; b=ZTfH3vZkM5aSh4yKQC8w5X1qaykA+YljSi0ZjUJC5759/YYzfEu5sAw4PVdVVCoGJ2 dVOETpdFRFgNSTveamlF8CP3pAWF8qTWctU71rEFriqEOEXyRhKxHR9xWRSrYXVPT35Z oLQ3SUMJxp4eC+w9wDRdWIaZbzJyO7FqQL4QTNpFi4zQF9GCiPzo5c1C5hVYB33SETAe OJxWjtr3KeV33z3xesTCHdbA/bd8mjsIz79H4yRYhtSbXgDMoi9gu5tSbS4qEofNdejN uXr+X/yjFLpBJ3i8W2k58LwFiwvryzOib5ASp4xiR6eCZ4Bs5L+J0ZIyLRSXpy0oP1/d 12lw== X-Forwarded-Encrypted: i=1; AJvYcCXTSg9S4eKN+X7H0xaqsoJmofjkNrzlIuqUhSrBQre4JBjvy3lcgGMoa0Om5/Gs/nv0VkFtrzh0ATBH6mBmV89cbiQqpYA=@freebsd.org, AJvYcCXzweEiLOg1YbTpy77XODVYvoYveTyTlcfV1YAYI/5KOSpqC+AP/ckcHBpW/y05z7CPXwve3668iLjzC0+LedGZxAIA@freebsd.org X-Gm-Message-State: AOJu0Yyf6FJGfhrHXj77n2AjBSwsT748Hqw2gh16kH4GKVpmaeMZrQdp iraKJ0APoUt072FBQkZX1TbvSpUE55z5JvYmvLj7SA6kJxoZjmPuYKPR X-Gm-Gg: ASbGnctKJFrU4xbrq5jtj0AI5c2WRboNMeIeYi6bQ9ZKqaaK4NsAx2Vb7YURYhK7AAQ FN9snb+UzNyF1Xu9qn5iMbTKahkgQxJTspHUGBn0W7eARYstbwOSde3LKKTBDBM+0whsHRWK+zv zGeBPbdVMdyflUx74QqKkTb1xfbghlVvMJATFcxlR3pR3Y5lb20OKw9mPqy+w/48UUqhRgrkcAL CyLiEKUlIP4jsfYtNjauD0ME3aiwdQ+PMQwdHIhpswDVxoBXG0fZWM3BCvfhu9fLNjUnw6e5oZE 3DPL0gstwcc4A0fyqltfAkidP6Rw0DCS4dChGV4ZduTa8TglA+fJi53ccL1fxCNTZua1cn2fbTA cmAFV7QEaZtj9KI1w+kE2JjEc+icLQx+eY83tWyx3noNhYU8= X-Google-Smtp-Source: AGHT+IEBWamxDM4kf/wY4lU6ynM3tizcRvdxzDdJaxAzhndSeP7IUaALXPhYgLnf6Xga1Jz32f87eA== X-Received: by 2002:ad4:5ba6:0:b0:707:5d35:2a54 with SMTP id 6a1803df08f44-7075d352bb4mr19476296d6.43.1753796304662; Tue, 29 Jul 2025 06:38:24 -0700 (PDT) Received: from nuc (192-0-220-237.cpe.teksavvy.com. [192.0.220.237]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-70729aafa4csm44481746d6.42.2025.07.29.06.38.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Jul 2025 06:38:23 -0700 (PDT) Date: Tue, 29 Jul 2025 09:38:21 -0400 From: Mark Johnston To: Konstantin Belousov Cc: 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 all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 4brxJ35Fcpz3D1j 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 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 > > > > > 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, 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) {