linux-kernel - Re: [RFC PATCH v1 10/25] printk: redirect emit/store to new ringbuffer

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]

Date:   Wed, 20 Feb 2019 22:25:00 +0100
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Daniel Wang <wonderfly@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Cox <gnomes@...rguk.ukuu.org.uk>,
        Jiri Slaby <jslaby@...e.com>,
        Peter Feiner <pfeiner@...gle.com>,
        linux-serial@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC PATCH v1 10/25] printk: redirect emit/store to new ringbuffer

On 2019-02-20, Petr Mladek <pmladek@...e.com> wrote:
>> vprintk_emit and vprintk_store are the main functions that all printk
>> variants eventually go through. Change these to store the message in
>> the new printk ring buffer that the printk kthread is reading.
>
> We need to switch the two buffers in a single commit
> without disabling important functionality.
>
> By other words, we need to change vprintk_emit(), vprintk_store(),
> console_unlock(), syslog(), devkmsg(), and syslog in one patch.

Agreed. But for the review process I expect it makes things much easier
to change them one at a time. Patch-squashing is not a problem once all
the individuals have been ack'd.

>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 5a5a685bb128..b6a6f1002741 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -584,54 +500,36 @@ static int log_store(int facility, int level,
>>  		     const char *text, u16 text_len)
>
>>  	memcpy(log_dict(msg), dict, dict_len);
>>  	msg->dict_len = dict_len;
>>  	msg->facility = facility;
>>  	msg->level = level & 7;
>>  	msg->flags = flags & 0x1f;
>
> The existing struct printk_log is stored into the data field
> of struct prb_entry. It is because printk_ring_buffer is supposed
> to be a generic ring buffer.

Yes.

> It makes the code more complicated. Also it needs more space for
> the size and seq items from struct prb_entry.
>
> printk() is already very complicated code. We should not make
> it unnecessarily worse.

In my opinion it makes things considerably easier. My experience with
printk-code is that it is so complicated because it is mixing
printk-features with ring buffer handling code. By providing a strict
API (and hiding the details) of the ring buffer, the implementation of
the printk-features became pretty straight forward.

Now I will admit that the ring buffer API I proposed is not easy to
digest. Mostly because I leave a lot of work up to the readers and have
lots of arguments. Your proposed changes of passing a struct and moving
loops under the ring buffer API should provide some major
simplification.

> Please, are there any candidates or plans to reuse the new ring
> buffer implementation?

As you pointed out below, this patch already uses the ring buffer
implementation for a totally different purpose: NMI safe dynamic memory
allocation.

> For example, would it be usable for ftrace? Steven?
>
> If not, I would prefer to make it printk-specific
> and hopefully simplify the code a bit.
>
>
>> -	if (ts_nsec > 0)
>> -		msg->ts_nsec = ts_nsec;
>> -	else
>> -		msg->ts_nsec = local_clock();
>> -	memset(log_dict(msg) + dict_len, 0, pad_len);
>> +	msg->ts_nsec = ts_nsec;
>>  	msg->len = size;
>>  
>>  	/* insert message */
>> -	log_next_idx += msg->len;
>> -	log_next_seq++;
>> +	prb_commit(&h);
>>  
>>  	return msg->text_len;
>>  }
>
> [...]
>
>>  int vprintk_store(int facility, int level,
>>  		  const char *dict, size_t dictlen,
>>  		  const char *fmt, va_list args)
>>  {
>> -	static char textbuf[LOG_LINE_MAX];
>> -	char *text = textbuf;
>> -	size_t text_len;
>> +	return vprintk_emit(facility, level, dict, dictlen, fmt, args);
>> +}
>> +
>> +/* ring buffer used as memory allocator for temporary sprint buffers */
>> +DECLARE_STATIC_PRINTKRB(sprint_rb,
>> +			ilog2(PRINTK_RECORD_MAX + sizeof(struct prb_entry) +
>> +			      sizeof(long)) + 2, &printk_cpulock);
>> +
>> +asmlinkage int vprintk_emit(int facility, int level,
>> +			    const char *dict, size_t dictlen,
>> +			    const char *fmt, va_list args)
>
> [...]
>
>> +	rbuf = prb_reserve(&h, &sprint_rb, PRINTK_SPRINT_MAX);
>
> The second ring buffer for temporary buffers is really interesting
> idea.
>
> Well, it brings some questions. For example, how many users might
> need a reservation in parallel. Or if the nested use might cause
> some problems when we decide to use printk-specific ring buffer
> implementation. I still have to think about it.

Keep in mind that it is only used by the writers, which have the
prb_cpulock. Typically there would only be 2 max users: a non-NMI writer
that was interrupted during the reserve/commit window and the
interrupting NMI that does printk. The only exception would be if the
printk-code code itself triggers a BUG_ON or WARN_ON within the
reserve/commit window. Then you will have an additional user per
recursion level.

John Ogness