Re: [PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Greg Kroah-Hartman]

On Tue, Jul 28, 2026 at 07:18:38AM +0200, Christoph Hellwig wrote:
> On Tue, Jul 28, 2026 at 07:15:28AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 27, 2026 at 04:51:40PM -0600, Keith Busch wrote:
> > > On Mon, Jul 27, 2026 at 09:19:27PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Jul 27, 2026 at 08:24:40AM -0600, Keith Busch wrote:
> > > > > On Sat, Jul 25, 2026 at 03:51:11PM +0200, Hari Mishal wrote:
> > > > > > write_stream is validated against bdev_max_write_streams() in both
> > > > > > generic block direct I/O (block/fops.c) and F2FS before a bio
> > > > > > carrying it is ever built, so write_stream > nr_plids shouldn't be
> > > > > > reachable through any current legitimate path. The remaining users
> > > > > > of bio->bi_write_stream elsewhere in the block layer only copy an
> > > > > > already-validated value between bios (bio.c, blk-crypto-fallback.c)
> > > > > > or compare it for merge eligibility (blk-merge.c); none of them
> > > > > > introduce a new, unvalidated value.
> > > > > > 
> > > > > > Using WARN_ON_ONCE as the backstop for that assumption isn't worth
> > > > > > it given how many deployed systems run with panic-on-warn enabled;
> > > > > > the existing graceful return BLK_STS_INVAL already handles it on
> > > > > > its own.
> > > > > 
> > > > > That's not a very good reason to remove a WARN_ON. You've left the check
> > > > > in for a condition that should never happen, so when it does happen,
> > > > > it'll be impossible to debug without the WARN.
> > > > > 
> > > > > And the WARN also annotates the branch as unlikely, which is desirable
> > > > > for this case.
> > > > 
> > > > But, if it ever does happen, a WARN_ON will reboot the box, given that
> > > > billions of Linux systems have panic-on-warn enabled.  
> > > 
> > > So WARN_ON is the new BUG_ON now?
> > 
> > It has been that way since syzbot started sending us reports (i.e. for
> > many many years...)
> > 
> > > If the condition happens we need to
> > > know how we got here and make it obvious something is wrong. So I guess
> > > we'd have to replace every one of these:
> > > 
> > > 	if (WARN_ON_ONCE(condition)) ...
> > > 
> > > With an open-coded version like:
> > > 
> > > 	if (unlikely(condition)) {
> > > 		do_once(dump_stack());
> > > 		...
> > > 	}
> > > 
> > > ?
> > 
> > Yes, if userspace can trigger this.  Because again, this will cause a
> > box to reboot.
> > 
> > If we didn't have panic-on-warn, a ton of CVEs would just disappear
> > tomorrow.  But that's not the world we live in :(
> > 
> > > That doesn't seem right, so if that is the suggestion, then I think we
> > > need a new macro to provide the result that the WARN_ON usage expected.
> > 
> > You can provide a tracedump if you really need/want it, no need to call
> > WARN_ON(), the macro is there for you to use.
> > 
> > > > So if this can ever happen, 
> > > 
> > > But it can't ever happen. This patch's commit message reasoned that as
> > > justification to remove the warn, but we need to know how we got here
> > > when it does happen because it means somebody broke contract.
> > 
> > Fair enough, but note that if userspace can trigger this, it should be
> > fixed up.  See the other WARN_ON patch fix for nvme that I sent yesterday
> > for an example of userspace being able to trigger this type of issue:
> > 	https://lore.kernel.org/r/20260727-nvme-tcp-v1-1-61c0e36763eb@xxxxxxxxxxxxxxxxxxx
> > 
> > > > just properly handle it and recover and don't loose user
> > > > data.
> > > 
> > > We can't save the data from this specific condition: the data from the
> > > request is unwritable and lost. We've also learned that EINVAL errors
> > > are not handled for many DM stacking drivers in very bad ways, so again,
> > > we need to know how we got here when something breaks the API contract
> > > otherwise it'll be a difficult problem to debug without that visibility.
> > 
> > Ok, if you want to keep this here, that's fine, because you know you
> > can't recover properly and crashing the system is the only acceptable
> > thing to do.  But in that case, why not make it a BUG_ON()?
> 
> Because we don't want a BUG_ON.  If you set panic on warn in anything
> but a debug setup you get what you pay for, and I'm really tired of
> all these totally stupid attempts to make WARN_ON the new BUG_ON.
> It is not, and that's for a reason.

I'm tired of it too, but again, if this can be hit by something a user
does, it ends up being a DoS on the machine :(

thanks,

greg k-h