linux-kernel - Re: printk: what is going on with additional newlines?

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:   Fri, 01 Sep 2017 10:32:13 -0700
From:   Joe Perches <joe@...ches.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Pavel Machek <pavel@....cz>, Petr Mladek <pmladek@...e.com>,
        Jan Kara <jack@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jiri Slaby <jslaby@...e.com>, Andreas Mohr <andi@...as.de>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: printk: what is going on with additional newlines?

On Fri, 2017-09-01 at 22:19 +0900, Sergey Senozhatsky wrote:
> On (08/29/17 21:10), Steven Rostedt wrote:
> [..]
> > > could do. for a single continuation line printk("%.*s", s.len, s.buffer)
> > > this will work perfectly fine. for a more general case - backtraces, dumps,
> > > etc. - this requires some tweaks.
> > 
> > We could simply add a seq_buf_printk() that is implemented in the printk
> > proper, to parse the seq_buf buffer properly, and add the timestamps and
> > such.
> 
> so I quickly cooked the first version. like really quickly. just to
> check if this is what people might like/use.
> 
> RFC.
> 
> so wondering if this will suffice. the name is somewhat hideous -- prbuf(),
> wanted to keep it short and somehow aligned with pr_foo().

Yes, it's a poor name.  At least keep using a pr_ prefix.

> the patch also defines a number of prbuf_err()/prbuf_cont() macros that
> call __prbuf_write() -- I don't want people to invoke __prbuf_write()
> directly, because we need KERN_FOO prefix for stored messages and people
> tend to forget to provide one.

> prbuf_init() function inits the seq_buf buffer. it takes size and GFP
> mask, just to permit prbuf usage from different contexts. if we fail
> to kmalloc() the buffer, then __prbuf_write() does direct printk().

I think there's relatively little value in multiple line output.
It seems like buffering for buffering's sake.
Just keep it to a single line and simple.

> a usage example:
> 
> 
>        struct seq_buf s;
> 
>        prbuf_init(&s, 256, GFP_KERNEL);
> 
>        prbuf_err(&s, "Opps at %lu\n", _RET_IP_);
>        prbuf_info(&s, "Start of cont line");
>        prbuf_cont(&s, " foo ");
>        prbuf_cont(&s, " bar ");
>        prbuf_cont(&s, " status: %s\n", "done");
> 
>        ret = 0;
>        while (ret++ < 10)
>                prbuf_err(&s, "%x\n", ret);
> 
>        prbuf_flush(&s);
>        prbuf_free(&s);
> 
> 
> this will store everything in conseq logbuf entries. if the buffer
> was too small, we print overflow message.
> 
> any comments?
[]
> diff --git a/include/linux/printk.h b/include/linux/printk.h
[]
> @@ -277,6 +288,29 @@ static inline void printk_safe_flush(void)
>  static inline void printk_safe_flush_on_panic(void)
>  {
>  }
> +
> +struct seq_buf;
> +
> +static inline
> +int prbuf_init(struct seq_buf *s, size_t size, gfp_t flags)
> +{
> +	return 0;
> +}
> +
> +static inlin __printf(2, 3) __cold

uncompiled

> +static int __prbuf_write(struct seq_buf *s, const char *fmt, ...)

inline

> +int prbuf_init(struct seq_buf *s, size_t size, gfp_t flags)
> +{
> +	char *b;
> +
> +	b = kmalloc(size, flags);
> +	seq_buf_init(s, b, size);
> +	return !!b;
> +}

Most of the time, this buffer should be on the stack
and not be malloc'd.