From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 859182FD1DC; Tue, 2 Sep 2025 18:07:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.97.179.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756836462; cv=none; b=Cg2mylnID6+76Cu4+LoMscsF2BccQknFqTARcrg20A6rTAxZQcayHcH0QaxbCe4MdVhXJK2x1mnjJznQNiVstsQtQfPBH83xyxXcBM80nxIi2VoLEwY6GThpYx0wUZzBvllyc0VfquwcnlHxqYqrcmbilBxBZI9F39HMeEDAWow= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756836462; c=relaxed/simple; bh=hDQkjgpchSfHdmIKlzZy1e2r5Zb4UPQsU9QDvkepNzs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=K0Wph0QxT6ovTOQ5VvsyC3T+dCzmGCSvnLYtLed99psQF6XUKiELlrZjxyGz3iVyvimM8roUY0dR8sA4m2gI8r/ylefrBITCkZXm7X5Sojj728/RULTH+QeAuiNoERnwhgnPvLDkwvHiROvj/Qi//7JAsYZu9M1MLvWw1D6Wkzs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=igalia.com; spf=pass smtp.mailfrom=igalia.com; dkim=pass (2048-bit key) header.d=igalia.com header.i=@igalia.com header.b=JWb6TM65; arc=none smtp.client-ip=213.97.179.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=igalia.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=igalia.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=igalia.com header.i=@igalia.com header.b="JWb6TM65" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:MIME-Version:Message-ID: Date:References:In-Reply-To:Subject:Cc:To:From:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=oNdlysJjdtKeCzz+IbJEvciXiA4nco2aQvV39BALyhM=; b=JWb6TM65a/UPvsSXA+pyfx2u2O b5dm0iZbFXQ9nSHsSTIN8KRaRSiQzrTkWRsj7/OVxsYaiYS9PAuZID2HLca9F+mclJ0soZmf4eSmZ 72XHDh5NZrvPUKFuIWLEc49vAYP9FJEF+R6o2kOIWri3uoSGobPc41dxUKdULSNaUsc2ilGSOQlmX QncHO+omRc+ch8P3Sa0yWddSWubSqrOLqD6uVNdxVrtV2EjOHZN40D4mxEm1KKTr4qxoTgFH05QD5 MnFP//3WIOGlvMrTSTQev8Zesu7H6LmPiR05BkVAq8meLDzEP3+BzDIx6+Z+TCLYYSNTPGtqHg9Sy Sxl1BaTA==; Received: from bl17-145-117.dsl.telepac.pt ([188.82.145.117] helo=localhost) by fanzine2.igalia.com with utf8esmtpsa (Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim) id 1utVPq-005p1w-BU; Tue, 02 Sep 2025 20:07:34 +0200 From: Luis Henriques To: Joanne Koong Cc: Miklos Szeredi , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] fuse: remove WARN_ON_ONCE() from fuse_iomap_writeback_{range,submit}() In-Reply-To: (Joanne Koong's message of "Tue, 2 Sep 2025 10:35:28 -0700") References: <20250902152234.35173-1-luis@igalia.com> Date: Tue, 02 Sep 2025 19:07:33 +0100 Message-ID: <87bjnssp7e.fsf@wotan.olymp> Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi Joanne, On Tue, Sep 02 2025, Joanne Koong wrote: > On Tue, Sep 2, 2025 at 8:22=E2=80=AFAM Luis Henriques w= rote: >> >> The usage of WARN_ON_ONCE doesn't seem to be necessary in these function= s. >> All fuse_iomap_writeback_submit() call sites already ensure that wpc->wb= _ctx >> contains a valid fuse_fill_wb_data. > > Hi Luis, > > Maybe I'm misunderstanding the purpose of WARN()s and when they should > be added, but I thought its main purpose is to guarantee that the > assumptions you're relying on are correct, even if that can be > logically deduced in the code. That's how I see it being used in other > parts of the fuse and non-fuse codebase. For instance, to take one > example, in the main fuse dev.c code, there's a WARN_ON in > fuse_request_queue_background() that the request has the FR_BACKGROUND > bit set. All call sites already ensure that the FR_BACKGROUND bit is > set when they send it as a background request. I don't feel strongly > about whether we decide to remove the WARN or not, but it would be > useful to know as a guiding principle when WARNs should be added vs > when they should not. I'm obviously not an authority on the subject, but those two WARN_ON caught my attention because if they were ever triggered, the kernel would crash anyway and the WARNs would be useless. For example, in fuse_iomap_writeback_range() you have: struct fuse_fill_wb_data *data =3D wpc->wb_ctx; struct fuse_writepage_args *wpa =3D data->wpa; [...] WARN_ON_ONCE(!data); In this case, if 'data' was NULL, you would see a BUG while initialising 'wpa' and the WARN wouldn't help. I'm not 100% sure these WARN_ON_ONCE() should be dropped. But if there is a small chance of that assertion to ever be true, then there's a need to fix the code and make it safer. I.e. the 'wpa' initialisation should be done after the WARN_ON_ONCE() and that WARN_ON_ONCE() should be changed to something like: if (WARN_ON_ONCE(!data)) return -EIO; /* or other errno */ Does it make sense? As I said, I can send another patch to keep those WARNs and fix these error paths. But again, after going through the call sites I believe it's safe to assume that WARN_ON_ONCE() will never trigger. Cheers, --=20 Lu=C3=ADs > Thanks, > Joanne > >> >> Function fuse_iomap_writeback_range() also seems to always be called wit= h a >> valid value. But even if this wasn't the case, there would be a crash >> before this WARN_ON_ONCE() because ->wpa is being accessed before it. >> > > I agree, for the fuse_iomap_writeback_range() case, it would be more > useful if "wpa =3D data->wpa" was moved below that warn. > >> Signed-off-by: Luis Henriques >> --- >> As I'm saying above, I _think_ there's no need for these WARN_ON_ONCE(). >> However, if I'm wrong and they are required, I believe there's a need for >> a different patch (I can send one) to actually prevent a kernel crash. >> >> fs/fuse/file.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c >> index 5525a4520b0f..fac52f9fb333 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -2142,8 +2142,6 @@ static ssize_t fuse_iomap_writeback_range(struct i= omap_writepage_ctx *wpc, >> struct fuse_conn *fc =3D get_fuse_conn(inode); >> loff_t offset =3D offset_in_folio(folio, pos); >> >> - WARN_ON_ONCE(!data); >> - >> if (!data->ff) { >> data->ff =3D fuse_write_file_get(fi); >> if (!data->ff) >> @@ -2182,8 +2180,6 @@ static int fuse_iomap_writeback_submit(struct ioma= p_writepage_ctx *wpc, >> { >> struct fuse_fill_wb_data *data =3D wpc->wb_ctx; >> >> - WARN_ON_ONCE(!data); >> - >> if (data->wpa) { >> WARN_ON(!data->wpa->ia.ap.num_folios); >> fuse_writepages_send(wpc->inode, data);