Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   John Ogness <john.ogness@linutronix.de>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 7/7] printk: Handle dropped message smarter
In-Reply-To: <Y5CMD8iZND3rgugG@alley>
References: <20221123231400.614679-1-john.ogness@linutronix.de>
 <20221123231400.614679-8-john.ogness@linutronix.de>
 <Y5CMD8iZND3rgugG@alley>
Date:   Wed, 07 Dec 2022 18:04:56 +0106
Message-ID: <87h6y71773.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 2022-12-07, Petr Mladek <pmladek@suse.com> wrote:
>> +	/* Print it into ext_text, which is unused. */
>> +	len = snprintf(ext_text, DROPPED_TEXT_MAX,
>> +		       "** %lu printk messages dropped **\n", dropped);
>> +
>
> I would feel better if we check here that the text fits into the rest
> of the buffer.
>
> 	if (WARN_ON_ONCE(len + cmsg->outbuf_len < sizeof(cbufs->ext_text)))
> 		return;
>
> I know that it is kind-of guaranteed by the above compilation check
> of the *_MAX values. But there might be a bug and cmsg->outbuf_len
> might contains a garbage.

OK.

>> +	/*
>> +	 * Append the record text to the dropped message so that it
>> +	 * goes out with one write.
>> +	 */
>> +	memcpy(ext_text + len, &cbufs->text[0], cmsg->outbuf_len);
>> +
>> +	/* Update the output buffer descriptor. */
>> +	cmsg->outbuf = ext_text;
>> +	cmsg->outbuf_len += len;
>
> I still think that it would be better to rename the buffers in
> struct console_message and avoid the switches of the purpose
> of the two buffers.
>
> We could print the message about dropped text into a local buffer
> on stack. IMHO, 64 bytes are acceptable. And we could insert it
> into the outbuf by shuffling the existing text. Something like:
>
> static void msg_print_dropped(struct console_message *cmsg,
> 			      unsinged long dropped)
> {
> 	char dropped_msg[DROPPED_TEXT_MAX];
> 	int dropped_len;
>
> 	if (!con->dropped)
> 		return 0;
>
> 	/* Print it into ext_text, which is unused. */
> 	dropped_len = snprintf(dropped_msg, sizeof(dropped_msg),
> 		       "** %lu printk messages dropped **\n", con->dropped);
>
> 	/*
> 	 * The buffer might already be full only where the message consist
> 	 * of many very short lines. It is not much realistic.
> 	 */
> 	if (cmsg->outbuf_len + dropped_len + 1 > sizeof(cmsg->outbuf)) {
> 		/* Should never happen. */

This certainly can happen. @text is size CONSOLE_LOG_MAX, which is
LOG_LINE_MAX+PREFIX_MAX. So a totally legal formatted message of length
LOG_LINE_MAX-1 and a prefix will suddenly become truncated.

> 		if (WARN_ON_ONCE(dropped_len + 1 > sizeof(cmsg->outbuf)))
> 			return;
>
> 		/* Trunk the message like in record_print_text() */
> 		cmsg->outbuf_len = sizeof(cmsg->outbuf) - dropped_len;
> 		cmsg->outbuf[cmsg->outbuf_len] = '\0';
> 	}
>
> 	memmove(cmsg->outbuf + dropped_len, cmsg->outbuf, cmsg->outbuf_len + 1);
> 	memcpy(cmsg->outbuf, dropped_msg, dropped_len);
> }

I do not like the idea of increasing stack usage, possibly cutting off
messages, and performing extra memory operations all because of some
variable naming. There is literally a larger unused buffer just sitting
there.

I want struct console_buffers to be a black box of working buffers used
to process the different types of messages. console_get_next_message()
is the only function that should go inside that black box because it is
responsible for creating the actual message.

The caller does not care what is in the black box or how those internal
working buffers are named. The caller just cares about cmsg->outbuf and
cmsg->outbuf_len, which will point to the data that needs to be written
out.

For v3 I would like to try my approach one more time. I will give the
internal buffers new names so as not to mislead their roles. I will
clearly document the black box nature of struct console_buffers.

John Ogness