linux-kernel - Re: pr_cat() + CATSTR(name, size)?

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: <1342029074.13724.113.camel@joe2Laptop>
Date:	Wed, 11 Jul 2012 10:51:14 -0700
From:	Joe Perches <joe@...ches.com>
To:	Kay Sievers <kay@...y.org>, Vegard Nossum <vegard.nossum@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: pr_cat() + CATSTR(name, size)?

On Wed, 2012-07-11 at 19:25 +0200, Kay Sievers wrote:
> On Wed, Jul 11, 2012 at 6:55 PM, Joe Perches <joe@...ches.com> wrote:
> > On Wed, 2012-07-11 at 17:48 +0200, Kay Sievers wrote:
> >> On Wed, Jul 11, 2012 at 5:30 PM, Joe Perches <joe@...ches.com> wrote:
> >> > Well, I think the malloc costs are pretty low
> >> > and could devolve pretty easily when OOM.
> >>
> >> We need to avoid allocating memory in situations where we want to
> >> printk(), it's just not possible.
> >
> > "it's just not possible???"  Kay, them's fightin' words. :)
> 
> Nah, I meant it. :) It limits the usefulness of these functions. We
> can not safely allocate memory, or do not get any memory in some
> situations where we want to use printk(). Hey, it might be used to say
> printk("out of memory\n").

:)

> >> That's why all the kmsg/printk can
> >> not really do any plain malloc. All printk memory needs to be static,
> >> on the stack or somehow pre-allocated.
> >
> > Maybe, I was planning to play with it after
> > refactoring printk in the next couple releases.
> 
> Sounds good.
> 
> >> > Anyway, interesting idea, keep at it, see what
> >> > comes out of it.
> >>
> >> Just depends on us, I guess. :)
> 
> > If your solution is just for the dev_<level> messages
> > (ie: with vprintk_emit descriptors), then it's not
> > too ugly.
> 
> Yeah, I thought only about these. But there might be more users where
> it makes sense to do that in a more reliable manner, don't know. It
> was surely no meant to replace the remaining 99.9% of the other cont
> users. :)

I believe your current reassembly code only works
on a maximum of 2 interleaved threads.  Did that change?

> > Did you look at the remaining dev_<level> and printk
> > continuations grep pattern?  There really aren't too
> > many to fix up.
> 
> Yeah, it looks fine to fix these few.
> 
> > Maybe in 3.6.  None of them appear particularly urgent.
> 
> Right.
> 
> > One trivial style note:
> >
> > Maybe CATSTR could use a struct and a DECLARE_ macro?
> >
> > struct printk_continuation_buffer {
> >         size_t length;
> >         size_t pos;
> >         char buf[];
> > }
> 
> Yeah, but then we lose the simplicity of passing the normal string
> around, and we need accessor macros to get to the string when we pass
> it around later. Maybe it's still OK, but it's surely not so intuitive
> anymore.
> 
> > It's a pity gcc doesn't allow non-static declarations like:
> >
> > #define DECLARE_PRINTK_BUF(name, size)          \
> > struct printk_continuation_buffer name = {      \
> >         .length = size;                         \
> >         .pos = 0;                               \
> >         .buf[size] = {0};                       \
> > }
> 
> Yeah, when the size changes, we have different type of struct. So we
> can not name them all "printk_continuation_buffer", every different
> size would conflict with each other.

It doesn't work so it doesn't matter no?

> > So maybe a DECLARE/DESTROY thing could work
> > with the appropriate malloc/free.
> 
> Hmm, I really don't think we can teach the people, or expect them to
> know, that these printk() functions are fragile if used in some
> critical code paths.

Vigilance. (and maybe a checkpatch test :).
There just aren't many critical code paths.

> It would at least need the GFP flags and in many
> cases GFP_ATOMIC which can easily fail, and we would also need to do
> error checking then, and printk() should just never fail, because it
> is used to tell that something went wrong. We have the entire kmsg
> buffer pre-allocated at bootup for that reason.

I still think devolving to direct printks when OOM works
as a fallback just fine.

> I think the only really sane option here is to use the (usually
> ~50-100 bytes) stack. Or did you have another idea here which I
> missed?

Other than malloc, I don't think there's another option.
Anyone else?

Vegard?  Are you still around?

Do you want to revive something like the blocks in:
https://lkml.org/lkml/2007/10/4/367


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/