Re: [PATCH] printk: Fix _DESCS_COUNT type for 64-bit systems

[Petr Mladek <pmladek@suse.com>]

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" <realsummitzhou@gmail.com> 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 <realsummitzhou@gmail.com>
> > ---
> >  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 <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

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