linux-kernel - Re: vprintk_store: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k0tumusi.fsf@jogness.linutronix.de>
Date:   Sun, 06 Dec 2020 23:36:53 +0106
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: vprintk_store: was: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock

On 2020-12-04, Petr Mladek <pmladek@...e.com> wrote:
>> +	/* Strip kernel syslog prefix. */
>
> Syslog actually uses: <level> format. We are skipping log level
> and control flags here.

OK.

>> +	if (facility == 0) {
>> +		while (text_len >= 2 && printk_get_level(text)) {
>> +			text_len -= 2;
>> +			text += 2;
>> +		}
>
> We should avoid two completely different approaches
> that handle printk_level prefix.
>
> One solution is to implement something like:
>
>      static char *parse_prefix(text, &level, &flags)
>
> That would return pointer to the text after the prefix.
> And fill level and flags only when non-NULL pointers are passed.

OK.

> Another solution would be to pass this information from
> vprintk_store(). The prefix has already been parsed
> after all.

Well, there is a vscnprintf() that happens in between and I don't think
we should trust the parsed offset from the first vsnprintf().

>> +
>> +		if (text != orig_text)
>> +			memmove(orig_text, text, text_len);
>> +	}
>
> We should clear the freed space to make the ring buffer as
> human readable as possible when someone just dumps the memory.

Data blocks are currently padded and that padding is not cleared. So it
is already not perfectly human readable on a raw dump.

> Sigh, I have to admit that I missed the problem with prefix and
> trailing '\n' when I suggested to avoid the temporary buffers.
> This memmove() and the space wasting is pity.
>
> Well, it is typically 3 bytes per message. And the copying would
> be necessary even with the temporary buffer. So, I am less convinced
> but I would still try to avoid the temporary buffers for now.

Agreed. I think this approach is better than the temporary buffers I
previously used. Also, if we add a trimming feature to the ringbuffer,
it will keep the ringbuffer mostly clean anyway. Something like this:

prb_rec_init_wr(&r, text_len);
prb_reserve(&e, prb, &r);
text_len = printk_sprint(&r.text_buf[0], text_len, ...);
r.info->text_len = text_len;
prb_trim_rec(&e, &r); <--- try to reduce datablock size to @text_len
prb_commit(&e);

I see no urgency to add such a feature. But I think we should keep it on
our radar.

John Ogness