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

[Petr Mladek]

On Wed 2026-03-25 15:22:29, John Ogness wrote:
> 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.

Good point!

> > 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;

Ah, I have missed that this earlier check won't catch it because
it is '<' and not '<='.

> > 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.

IMHO, there is no function which would expect that a data_block
must have a textlen > 0. It just should not exit. Because zero size
data blocks are handled special way.

So, just to be sure that the new code works as expected.
Note that the moved check:

	/* Sanity check. Data-less blocks were handled earlier. */
	if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size) || !*data_size))
		return NULL;

warns only when data_check_size() fails. It just quietly returns NULL
when *data_size is zero.

If we really want to warn. Then it would make more sense to change "<"
to "<=" in the previous check before the subtraction.

I do not have a strong opinion. But I tend to add the warning because
the zero size is not expected at this stage. Well, we could do
it in a followup patch and keep this on as is.

Best Regards,
Petr