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

[Petr Mladek <pmladek@suse.com>]

On Thu 2026-03-26 11:46:48, John Ogness wrote:
> On 2026-03-26, Petr Mladek <pmladek@suse.com> wrote:
> > 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.
> 
> ??? The above code warns for !*data_size as well.

Grr, I wrongly interpreted the brackets.

> > If we really want to warn. Then it would make more sense to change "<"
> > to "<=" in the previous check before the subtraction.
> 
> Yes, actually I would prefer that. Let me send a v2 where I relocate and
> reduce the data_check_size()-WARN and extend the data_size-WARN. So it
> is something like this:
> 
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 56c8e3d031f49..aa4b39e94cfa2 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1302,23 +1302,26 @@ 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)))) {
>  		return NULL;
>  	}
>  
> -	/* A valid data block will always have at least an ID. */
> -	if (WARN_ON_ONCE(*data_size < sizeof(db->id)))
> +	/*
> +	 * A regular data block will always have an ID and at least
> +	 * 1 byte of data. Data-less blocks were handled earlier.
> +	 */
> +	if (WARN_ON_ONCE(*data_size <= sizeof(db->id)))
>  		return NULL;
>  
>  	/* Subtract block ID space from size to reflect data size. */
>  	*data_size -= sizeof(db->id);
>  
> +	/* Sanity check the max size of the regular data block. */
> +	if (WARN_ON_ONCE(!data_check_size(data_ring, *data_size)))
> +		return NULL;
> +
>  	return &db->data[0];
>  }

I like this. Thanks for patience with me.

Best Regards,
Petr