On (06/25/19 11:06), Petr Mladek wrote:
> On Tue 2019-06-25 10:44:19, John Ogness wrote:
> > On 2019-06-25, Sergey Senozhatsky <[email protected]> wrote:
> > > In vprintk_emit(), are we going to always reserve 1024-byte
> > > records, since we don't know the size in advance, e.g.
> > >
> > > printk("%pS %s\n", regs->ip, current->name)
> > > prb_reserve(&e, &rb, ????);
> > >
> > > or are we going to run vscnprintf() on a NULL buffer first,
> > > then reserve the exactly required number of bytes and afterwards
> > > vscnprintf(s) -> prb_commit(&e)?
> >
> > (As suggested by Petr) I want to use vscnprintf() on a NULL
> > buffer. However, a NULL buffer is not sufficient because things like the
> > loglevel are sometimes added via %s (for example, in /dev/kmsg). So
> > rather than a NULL buffer, I would use a small buffer on the stack
> > (large enough to store loglevel/cont information). This way we can use
> > vscnprintf() to get the exact size _and_ printk_get_level() will see
> > enough of the formatted string to parse what it needs.
>
> vscnprintf() with NULL pointer is perfectly fine. Only the formatted
> string has variable size.
Yeah, that should work. Probably. Can't think of any issues, except
for increased CPU usage. Some sprintf() format specifiers are heavier
than the rest (pS/pF on ia64/ppc/hppa).
OK, very theoretically.
There is a difference.
Doing "sz = vscprintf(NULL, msg); vscnprintf(buf, sz, msg)" for
msg_print_text() and msg_print_ext_header() was safe, because the
data - msg - would not change under us, we would work with logbuf
records, IOW with data which is owned by printk() and printk only.
But doing
sz = vcsprintf(NULL, "xxx", random_pointer);
if ((buf = prb_reserve(... sz))) {
vscnprintf(buf, sz, "xxx", random_pointer);
prb_commit(...);
}
might have different outcome sometimes. We probably (!!!) can have
some race conditions. The problem is that, unlike msg_print_text()
and msg_print_ext_header(), printk() works with pointers which it
does not own nor control. IOW within single printk() we will access
some random kernel pointers, then do prb stuff, then access those
same pointers, expecting that none of them will ever change their
state. A very simple example
printk("Comm %s\n", current->comm)
Suppose printk on CPU0 and ia64_mca_modify_comm on CPU1
CPU0 CPU1
printk(...)
sz = vscprintf(NULL, "Comm %s\n", current->comm);
ia64_mca_modify_comm()
snprintf(comm, sizeof(comm), "%s %d", current->comm, previous_current->pid);
memcpy(current->comm, comm, sizeof(current->comm));
if ((buf = prb_reserve(... sz))) {
vscnprintf(buf, "Comm %s\n", current->comm);
^^^^^^^^^^^^^^ ->comm has changed.
Nothing critical, we
should not corrupt
anything, but we will
truncate ->comm if its
new size is larger than
what it used to be when
we did vscprintf(NULL).
prb_commit(...);
}
Probably there can be other examples.
-ss
