From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) (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 E3D543C6A52 for ; Tue, 3 Mar 2026 16:52:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772556760; cv=none; b=skTxWThT9utAVYc/Hn/EjMsAX2fmYuWMjN781DXce7rZ5wqVZ05iZ5GYXrewGkKtOYi5F/j4IQYoG/lLwPoZTWSncDYryTcDmfu8IsbVex229NzpCOF51bSOUKudrQc0kb2LuSq7bQOe16M/f4aBJ0fyp/LoUZH/i2QbLTtyr8M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772556760; c=relaxed/simple; bh=4AvCxOPmI+icFHj9IgwVDfVS+4oPMGS89/Mqf4Lvt0Q=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Alz7zm6/3GM1/LO4VGfGFmxkOAGUcg0QlduHoRgjTQmq5yXFW7009Cjn4eidE8jHFNCod3gF843BOEpqqXezVlT7fSGN2XGdTe+mHXAiWNPTiFtQughEnaSFV5Evnimcl6d9ZUVL309P6WvFI5vYCJETSsK2if1UlK7ynWyHLu0= 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=aVmk3Tdh; arc=none smtp.client-ip=209.85.221.52 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="aVmk3Tdh" Received: by mail-wr1-f52.google.com with SMTP id ffacd0b85a97d-439b611274bso1651667f8f.3 for ; Tue, 03 Mar 2026 08:52:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1772556757; x=1773161557; 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=cVUeKMeWMxVtjvDglO/OFxpZb26EwFdj8HkHrwnW5eI=; b=aVmk3TdhSmy5E5ZyXB6NwqpPUZEKqzLadNN6IWrJOhEPHvhvjATk6sB+dcb8J6g0Bz vs4xHPRIRyzeqWFL8/P4AFIsF/3XxzB+p28pnG0OjoYw3P/Q5wQIoyoQwUgEM8XbIbD4 HAswULe6dCCeLpYKlbG6JGBjU3eRScSP5Gmw8RLRQeOfgCEsx6sGqWC3NhzkVOlIBDun TOCzdhlNmbgWKBwrWKbhXFX+SjZ03iJFYZBwfCnMxTX9yvj4AEXS+aIXOZX9LHm5SleD NKVG0K2Q0J81qCusGUklXROlkyptJJw9mUOW446NiSOZWRr2444GqUl6/ONoJ6ynO9TH m6yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772556757; x=1773161557; 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=cVUeKMeWMxVtjvDglO/OFxpZb26EwFdj8HkHrwnW5eI=; b=kBOS9RrmoI3IlDdKWRT7cSuo2yFaVSNMpRgA4jcGxEoLb3AlHozm7rqLWGxxie50Zb 4KkGxSZO83ldgkYkBFtFjHUWMf7Vku2Di59cRLwPzgTLWVgSVTs4o6VRliKsrQmJlUN3 iUGsN4NkqrGfJxJkSeS05ImaGnbH4L9Y9Z7IYWkGXr8+D8Nbagc08yommUU/FC1rv7tI x6FAnT5/lWLNvLDdUUmxe8XSuYroTkghk+RPRtHxQimGAtS45q3FmoJ4lsZnzRJZLPJU BUuJN1NbD8KMXFqfjy/05av4nviI0yr9sl4evvlLcpkE7c61ulfdA4rNpGb2pEza36TM PX3A== X-Forwarded-Encrypted: i=1; AJvYcCUhRldQYXJShscG+xS71eEvfzlkvciczd6fbpIbgszk7IpRxeZI4ZE17hzAaHyib1iJ8sjA5S7JWZ1BeNY=@vger.kernel.org X-Gm-Message-State: AOJu0Yx7hSKmM0GOhVjtYwDaiaAyXjkiNMXkfE9+IliSEiUNZ8+BFUlM zu4Wqzw97N/deRYZzRoawTkVmS7++jSb9gKgKvt3u6A+pdAskZci8Ip+G62hLmzrxq4hoSstMg1 VrTS2 X-Gm-Gg: ATEYQzxg5RaXP5w5idgKyOz0OADcrcHn66+jLuEAlU5Vowk2CzCC+4a87PvWNBBhV52 fYYdq0ZCvq2aEb2Y0XXTOUhtNUR59AcC0zn8992lhTtlkZAcoJyEFfhinohHQrfVDNwslRtoxve 31IaEJcTuzO32Jh7tYK2bhqNEcBDlL835bNpEBfCR/t0eZqejufnywN9mkEhPNYJIP29xuN+/S/ lUmgsa87XHivq6YUP1+PAI1mIQ0imuS+A1ZliGClievj+hCYVPKLzxzoykGsLaKwtuZgfdwwwRb A5yMBBswtFlKUsV15jKjXBLAL9K36Mv/wL9BF4/5n8mfx9ZpTzPSwtRPCGvGBTuhF75pYhQ7Dv4 azbKaAehG+ky3ddZUs2DPhaOzyK0tTcFB88R5GTZqUlfkgf5GbmGe1z5ZScHYTXjiJNzubmMVOu 5aPV9i7FzsfGtE/8x1nHmY8Q4W8A== X-Received: by 2002:a05:600c:1d0e:b0:483:acd9:bd18 with SMTP id 5b1f17b1804b1-483c9bc55ecmr293070395e9.1.1772556757085; Tue, 03 Mar 2026 08:52:37 -0800 (PST) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-483bd68826asm791391765e9.0.2026.03.03.08.52.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Mar 2026 08:52:36 -0800 (PST) Date: Tue, 3 Mar 2026 17:52:34 +0100 From: Petr Mladek To: John Ogness Cc: "feng.zhou" , linux-kernel@vger.kernel.org, senozhatsky@chromium.org, rostedt@goodmis.org Subject: Re: [PATCH] printk: Fix _DESCS_COUNT type for 64-bit systems Message-ID: References: <20260202094140.9518-1-realsummitzhou@gmail.com> <87o6m7fk4c.fsf@jogness.linutronix.de> 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: <87o6m7fk4c.fsf@jogness.linutronix.de> On Mon 2026-02-02 12:47:23, John Ogness wrote: > Hi Zhou, > > Thanks for reporting and fixing this! > > On 2026-02-02, "feng.zhou" wrote: > > The _DESCS_COUNT macro currently uses 1U (32-bit unsigned) instead of > > 1UL (unsigned long), which breaks the intended overflow testing design > > on 64-bit systems. > > > > Problem Analysis: > > ---------------- > > The printk_ringbuffer uses a deliberate design choice to initialize > > descriptor IDs near the maximum 62-bit value to trigger overflow early > > in the system's lifetime. This is documented in printk_ringbuffer.h: > > > > "initial values are chosen that map to the correct initial array > > indexes, but will result in overflows soon." > > > > The DESC0_ID macro calculates: > > DESC0_ID(ct_bits) = DESC_ID(-(_DESCS_COUNT(ct_bits) + 1)) > > > > On 64-bit systems with typical configuration (descbits=16): > > - Current buggy behavior: DESC0_ID = 0xfffeffff > > - Expected behavior: DESC0_ID = 0x3ffffffffffeffff > > > > The buggy version only uses 32 bits, which means: > > 1. The initial ID is nowhere near 2^62 > > 2. It would take ~140 trillion wraps to trigger 62-bit overflow > > 3. The overflow handling code is never tested in practice > > > > Root Cause: > > ---------- > > The issue is in this line: > > #define _DESCS_COUNT(ct_bits) (1U << (ct_bits)) > > > > When _DESCS_COUNT(16) is calculated: > > 1U << 16 = 0x10000 (32-bit value) > > -(0x10000 + 1) = -0x10001 = 0xFFFEFFFF (32-bit two's complement) > > > > On 64-bit systems, this 32-bit value doesn't get extended to create > > the intended 62-bit ID near the maximum value. > > I was surprised to see that 1U is used here. I did some historical > digging and it has existed since the very first private version of this > ringbuffer implementation. It was a private email to Petr: Yeah. > Date: 12 Oct 2019 > Subject: ringbuffer v1 > Message-ID: <87lftqwvhp.fsf@linutronix.de> > > That was the very first appearance of a descriptor initializer: > > +#define _DESCS_COUNT(ct_bits) (1U << (ct_bits)) > +#define DESCS_COUNT(dr) _DESCS_COUNT((dr)->count_bits) > +#define DESC0_ID(ct_bits) DESC_ID(-_DESCS_COUNT(ct_bits)) > > > Impact: > > ------ > > While index calculations still work correctly in the short term, this > > bug has several implications: > > > > 1. Violates the design intention documented in the code > > 2. Overflow handling code paths remain untested > > 3. ABA detection code doesn't get exercised under overflow conditions > > The ABA detection is only relevant for 32-bit systems. The patch is only > fixing an issue on 64-bit systems. > > > 4. In extreme long-term running scenarios (though unlikely), could > > potentially cause issues when ID actually reaches 2^62 > > It is worth investigating if this is an issue. Although it is quite > unlikely. A machine would need to generate 100k printk messages per > second for 490k years! Yes, I think that it is not much realistic. > > Verification: > > ------------ > > Tested on ARM64 system with CONFIG_LOG_BUF_SHIFT=20 (descbits=15): > > - Before fix: DESC0_ID(16) = 0xfffeffff > > - After fix: DESC0_ID(16) = 0x3fffffffffff7fff > > > > The fix aligns _DESCS_COUNT with _DATA_SIZE, which already correctly > > uses 1UL: > > #define _DATA_SIZE(sz_bits) (1UL << (sz_bits)) > > > > Signed-off-by: feng.zhou > > --- > > kernel/printk/printk_ringbuffer.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h > > index 4ef81349d9fb..4f4949700676 100644 > > --- a/kernel/printk/printk_ringbuffer.h > > +++ b/kernel/printk/printk_ringbuffer.h > > @@ -122,7 +122,7 @@ enum desc_state { > > }; > > > > #define _DATA_SIZE(sz_bits) (1UL << (sz_bits)) > > -#define _DESCS_COUNT(ct_bits) (1U << (ct_bits)) > > +#define _DESCS_COUNT(ct_bits) (1UL << (ct_bits)) > > #define DESC_SV_BITS BITS_PER_LONG > > #define DESC_FLAGS_SHIFT (DESC_SV_BITS - 2) > > #define DESC_FLAGS_MASK (3UL << DESC_FLAGS_SHIFT) > > The change looks correct. But I would like to take some more time to > understand why I never used UL in the first place. Good question. Anyway, my opinion about this patch evolved over time: 1. My first feeling was that this patch was not worth the effort and risk. As explained above, we would start testing corner-cases which could never be reached in reality. 2. Then I started looking at the code and realized that _DESCS_COUNT() was used also to initialize: #define DESCS_COUNT(desc_ring) _DESCS_COUNT((desc_ring)->count_bits) #define DESCS_COUNT_MASK(desc_ring) (DESCS_COUNT(desc_ring) - 1) and it would be great to be sure that _MASK covers the whole 64-bit unsigned long => patch was worth it. Also the @id is primary used to create index to @desc_ring using DESC_INDEX(). And I am sure that rotation of the @desc_ring is tested heavily => we should be on the safe side. Also I did some testing with the patch and did not find any problem. 3. Finally, I decided to double check how @id is handled in the code. And it is stored with state bits in @state_var => the highest 2 bits are cleared by DESC_ID() macro => ringing bells And indeed desc_reserve() shrinks the highest two bits when computing the next @id: id = DESC_ID(head_id + 1); and the masked @id is stored: } while (!atomic_long_try_cmpxchg(&desc_ring->head_id, &head_id, id)); /* LMM(desc_reserve:D) */ Now, get_desc_state() does a check: static enum desc_state get_desc_state(unsigned long id, unsigned long state_val) { if (id != DESC_ID(state_val)) return desc_miss; [...] } I belive that this check might fail when DESC_ID(state_val) is compared against non-masked @id But we seems to be on the safe side, because DESC0 is defined using DESC_ID() => the highest bits are masked: #define DESC0_ID(ct_bits) DESC_ID(-(_DESCS_COUNT(ct_bits) + 1)) => we seem to be on the safe side. Conclusion: The patch looks correct and acceptable to me: Reviewed-by: Petr Mladek Tested-by: Petr Mladek I am going to wait a bit whether John agrees with my opinion or whether he remembers and finds any blocker. > Also note (unrelated) that we are using 1U for the buffer of the data > ring. Not really an issue, but we should probably switch that to UL as > well. I might make sense to change this as well. But I would do it in a separate patch. We should be on the safe size even now because the log buffer size is limited by 2GB, see #define LOG_BUF_LEN_MAX ((u32)1 << 31) Best Regards, Petr