From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6C2E0373BF8 for ; Tue, 24 Mar 2026 16:45:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774370755; cv=none; b=RqSBfKwpW3nbs39xE6e7CMi/C77vpdrGZ+qe8av5mpvJyvIyTESZ7fi8urrKgmYFRkArwC9iQ8cd0OXaTLDYp+hcpXAwwdgJRajEJQ2Ot1n7QUF0Oes0ws9fjwrKBg2y3z4LUDDaDKgYy71x832MxR8nCnG3RSdhwN2aMTzNyX4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774370755; c=relaxed/simple; bh=lgtsycOKSqg8vAItSGt1LQogJtfyuRTEClm52Mk2MAs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PzS3qxRxEpa+l3nyo6jkO8RWecpanUtkWyjCiiWdSNA2b51Ek/dL0l6+9wByhzreFWHCDAcyMiB4aA6hJnfDfvuKF2F7NYo42MRx58jjJx9odbkRuRu5obm8W2oJEPn8dGaoeWPdLjHPgwBZrvPyfoHktSapDaElYTES+QHm4QY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=JJfKEHCy; arc=none smtp.client-ip=209.85.128.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="JJfKEHCy" Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-4852e9ca034so39234555e9.2 for ; Tue, 24 Mar 2026 09:45:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1774370752; x=1774975552; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=z3PWTBo53WIZpw60kGM5OuNGkA28mXpc0cWB1XdX5pU=; b=JJfKEHCyv9FaTdy9Q7OUsfc49WcCgl8gWu3rYeQocyzSksxqS6rOzMa3F8L7CUFCDE JR/C4rQwxhiJ3rF5QyeiD5QOMm2ysKN7LD4XF/CX2e6oDmY2nKDbBQi/P3TZg5blKvjW 8BBydad8UFY8mzOV2zcDrUnoBeXZWlB1yxligvjnl0t+irJaGl/qIFjq9t3fku6V+Doe gyJdZSAJCTfjiR2hpLX3c7rGjK6uAo4xSOhlBgs5JSMWmXhDVOw4ubI37i10cznQ16sL dmvkWnGW4riKl0pSafvw+apl/LojGb4j51ySZpwuEPYSeyhJqt1MysHQfGa3j6ooTDka oYWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774370752; x=1774975552; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=z3PWTBo53WIZpw60kGM5OuNGkA28mXpc0cWB1XdX5pU=; b=h6Eyh/W8ABuEkNNwhMQMvEuIcZY7xlTaGCkb2N8dJVV26AbUGEBUemVHy3Di19YRvP 7F9jHRArZZouO4pHwGHgLSbUMrHupn89idLeaO4SHoiGAD+aQpZyB6lDEkfnWCJp/A1j jfOf0+Lr+NBY/KBlc6LHzq1dJXkms4ukFPrp4LQRzoqGaJ1ysqdNLsLwj52jqqUfVeOq K/XWxr/lHoEO5q/UrpZL4lZOxPHsF3YcKKsXsnsY+AHV8aYn5ksvsObJqbdKIgUVgo+l vMk4n1SXdgcJbAh9yRgZuixaUlKsgkTe9AqB6bgqjY05L7K0hY81dzLSYJqFJ8hN+QdR PcQg== X-Forwarded-Encrypted: i=1; AJvYcCVNcsyE1va70RuNPjM0nVPXevhPwlW/jg7xgZGR5n8iShCV9WJWHy0LA28wwvb6Dmio2qiKJytL8UBHUfU=@vger.kernel.org X-Gm-Message-State: AOJu0Yz/Ha+Mn/7+M8cGkeQmAYvuD9toeeARgEdhYWyalNk78/huhgKn 2i/NQrDXkq/PE9uFMcyxgDmWmZAYOV6BsPTKJxkYPz5Qj6IFE/j1rYQclGlAlz8VKC4= X-Gm-Gg: ATEYQzwaF/fga/fOrtKgppjRUYTN0gCeqqkLcm1VdmtFB5OFAmoEoJykVAKMVep0Lht K0FQ/I72bjURooxVVRKzw53QkTrrZBOOmVI+n2/oZt3tWDqe1uBafPjKZOeJA0ikBplq7NmbvWC MZuQqrApRMvv/6b6cdezUYOZXJTL8zuOOms0r7qzqYgVmmXQv4Qb1Ztm4QoZnqhL6tdnpe7lHOA ixalLr8CrrBtCuHnqz0inf9pzl0vj5Za4dNyAjyhB/PXmFyviAxB4aecCF89JlHkkdxeMdTR4ng +xEERd8870TRQYrtqWIHL68jf665qW0bxaXHEibuOcUzj+zNNSAWGcuq05A4wf+rWskT1PpZQGf S79EHuRdQDLWG7640Wjl/vtnOeBC4gao2YMi01q28MGd72EG63VV4g0kVhgggXaWOLu/IS6Kxyq W52Sn8rEyHtnU6xbsGghXJUHbU+Q== X-Received: by 2002:a05:600c:c16e:b0:485:3dfc:569 with SMTP id 5b1f17b1804b1-48716042b0emr5933695e9.16.1774370751673; Tue, 24 Mar 2026 09:45:51 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-487116abe8esm65048645e9.4.2026.03.24.09.45.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Mar 2026 09:45:51 -0700 (PDT) Date: Tue, 24 Mar 2026 17:45:49 +0100 From: Petr Mladek To: David Laight , Rasmus Villemoes Cc: Andy Shevchenko , "Masami Hiramatsu (Google)" , Steven Rostedt , Sergey Senozhatsky , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/2] lib/vsprintf: Fix to check field_width and precision Message-ID: References: <177410406326.38798.16853803119128725972.stgit@devnote2> <177410407207.38798.2345647618225402693.stgit@devnote2> <20260323135905.5272127b@pumpkin> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260323135905.5272127b@pumpkin> On Mon 2026-03-23 13:59:05, David Laight wrote: > On Mon, 23 Mar 2026 15:27:31 +0200 > Andy Shevchenko wrote: > > > On Sat, Mar 21, 2026 at 11:41:12PM +0900, Masami Hiramatsu (Google) wrote: > > > > > Check the field_width and presition correctly. Previously it depends > > > on the bitfield conversion from int to check out-of-range error. > > > However, commit 938df695e98d ("vsprintf: associate the format state > > > with the format pointer") changed those fields to int. > > > We need to check the out-of-range correctly without bitfield > > > conversion. > > > > ... > > > > > static void > > > set_field_width(struct printf_spec *spec, int width) > > > { > > > - spec->field_width = width; > > > - if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) { > > > - spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX); > > > + if (WARN_ONCE(width > FIELD_WIDTH_MAX || width < -FIELD_WIDTH_MAX, > > > + "field width %d too large", width)) { > > > + width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX); > > > } > > > + spec->field_width = width; > > > } > > > > > > static void > > > set_precision(struct printf_spec *spec, int prec) > > > { > > > - spec->precision = prec; > > > - if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) { > > > - spec->precision = clamp(prec, 0, PRECISION_MAX); > > > + if (WARN_ONCE(prec > PRECISION_MAX || prec < 0, > > > + "precision %d too large", prec)) { > > > + prec = clamp(prec, 0, PRECISION_MAX); > > > } > > > + spec->precision = prec; > > > } > > > > Looking at this, perhaps > > > > #define clamp_WARN_*(...) > > ... It would make sense. But I do not want to force Masami to do so. > When I looked at this I did wonder whether the compiler would manage to > only do the comparisons once. I believe that compilers would optimize this. > Even if it doesn't the separate WARN is more readable. > > Or maybe: > spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX); > WARN_ON(spec->field_width != width, "field width %d too large", width); But this is fine as well. > I'd be more worried about the bloat and system panic for all the systems > with panic_on_warn set (the default for many distos). > (And, like panic_on_oops, it doesn't give time for the error to get into > the system logs.) The WARN_ONCE() has been added by the commit 4d72ba014b4b09 ("lib/vsprintf.c: warn about too large precisions and field widths"). The motivation was that it was used also by some %p? modifiers, e.g. for the bitfield size, where a clamped value might cause more confusion. I do not want to open a bike shedding whether it is important enough or not. I agree that it likely is not a reason to panic but it was there 10 years so I could live with it. That said, I always thought about introducing a macro which would print a message+backtrace but it would not panic, e.g. SOFT_WARN() or INFO() or MSG_AND_BT(level, msg, ...). But it seems to be out of scope of this patchset. > I've also just fixed nolibc's handling of %*.*s (which is in 'next' since > I only wrote it recently), the above is actually broken. > Negative 'precision' (all values) are fine, they just request the default. Great catch! We should clamp the precision to (0, PRECISION_MAX). But we should warn only when it is outside of (-PRECISION_MAX, PRECISION_MAX). > So the patch needs a big fat NACK... What is an acceptable solution then, please? Frankly, I would like to stay on earth. This started as a simple fix of a regression added a year ago. For me, any solution which restores the year old behavior is good enough. We might need to find another volunteer to implement a better solution, e.g. the new non-panicing MSG_AND_BT() macro. Alternatively, we could remove the WARN_ONCE() completely. It looks acceptable for me. But Rasmus would need to agree as well. Best Regards, Petr