Re: [PATCH printk] printk_ringbuffer: Fix get_data() size sanity check

[John Ogness]

On 2026-03-25, Petr Mladek <pmladek@xxxxxxxx> wrote:
>> --- a/kernel/printk/printk_ringbuffer.c
>> +++ b/kernel/printk/printk_ringbuffer.c
>> @@ -1302,10 +1302,6 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>  		return NULL;
>>  	}
>>  
>> -	/* Sanity check. Data-less blocks were handled earlier. */
>> -	if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
>> -		return NULL;
>> -
>>  	/* A valid data block will always be aligned to the ID size. */
>>  	if (WARN_ON_ONCE(blk_lpos->begin != ALIGN(blk_lpos->begin, sizeof(db->id))) ||
>>  	    WARN_ON_ONCE(blk_lpos->next != ALIGN(blk_lpos->next, sizeof(db->id)))) {
>> @@ -1319,6 +1315,10 @@ static const char *get_data(struct prb_data_ring *data_ring,
>>  	/* Subtract block ID space from size to reflect data size. */
>>  	*data_size -= sizeof(db->id);
>>  
>> +	/* Sanity check. Data-less blocks were handled earlier. */
>> +	if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
>
> The check of "!*data_size" is wrong after sizeof(db->id) subtractions.

It is not wrong. It is checking something else.

> The question is whether we still need it. As the comment says, the
> data-less block were handled earlier. And it seems that data_alloc()
> explicitly creates data-less blocks when the given size is 0.
> And prb_reserve_in_last() only allows to increase the size.
>
> IMHO, we could remove it. Wrong values will be handled by the above check:
>
> 	/* A valid data block will always have at least an ID. */
> 	if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> 		return NULL;
>
> A zero size datablock which is not handled using the explicit
> data-less block is not expected after all.

Well, the point of the WARN is to catch when things are broken. If for
some reason a data block of textlen 0 exists, it is a bug. I think we
should still catch it. I do not know if there are places in the code
that assume if it is a data block, it must have a textlen > 0. Probably
not, but still, it should not exist.

John