Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings
On Mon, 2018-04-09 at 14:19 +0200, Petr Mladek wrote: > On Sat 2018-04-07 17:12:35, Andy Shevchenko wrote: > > On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote: > > > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote: > > > > On 2018-04-04 10:58, Petr Mladek wrote: > > > > > > > > > Please leave string() alone, except for moving the < PAGE_SIZE > > > > check > > > > to > > > > a new helper checked_string (feel free to find a better name), > > > > and > > > > use > > > > checked_string for handling %s and possibly the few other cases > > > > where > > > > we're passing a user-supplied pointer. That avoids cluttering > > > > the > > > > entire > > > > file with double-underscore calls, and e.g. in the %pO case, > > > > it's > > > > easier > > > > to understand why one uses two different *string() helpers if > > > > the > > > > name > > > > of one somehow conveys how it is different from the other. > > > > > > I understand your reasoning. I thought about exactly this as well. > > > My problem is that string() will then be unsafe. It might be > > > dangerous > > > when porting patches. > > > > I agree with Rasmus, and your argument here from my point of view > > kinda > > weak. Are we really going to backport this patches? Why? We lived > > w/o > > them for a long time. What's changed now? > > Someone might have out-of-tree patch that adds yet another format > specifier. It might call string() that checks for (null) now but > it it won't if we rename it as you suggest. People used to safe > string() might miss this when the patch is send upstream for > inclusion, ... This is even weaker argument. Sorry, but I don't care about out-of-tree core patches. If they have them, they are doing completely wrong, or the patches are crappy. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings
On Sat 2018-04-07 17:12:35, Andy Shevchenko wrote: > On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote: > > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote: > > > On 2018-04-04 10:58, Petr Mladek wrote: > > > > We are going to check the address using probe_kernel_address(). It > > > > will > > > > be more expensive and it does not make sense for well known > > > > address. > > > > > > > > This patch splits the string() function. The variant without the > > > > check > > > > is then used on locations that handle string constants or strings > > > > defined > > > > as local variables. > > > > > > > > This patch does not change the existing behavior. > > > > > > Please leave string() alone, except for moving the < PAGE_SIZE check > > > to > > > a new helper checked_string (feel free to find a better name), and > > > use > > > checked_string for handling %s and possibly the few other cases > > > where > > > we're passing a user-supplied pointer. That avoids cluttering the > > > entire > > > file with double-underscore calls, and e.g. in the %pO case, it's > > > easier > > > to understand why one uses two different *string() helpers if the > > > name > > > of one somehow conveys how it is different from the other. > > > > I understand your reasoning. I thought about exactly this as well. > > My problem is that string() will then be unsafe. It might be dangerous > > when porting patches. > > I agree with Rasmus, and your argument here from my point of view kinda > weak. Are we really going to backport this patches? Why? We lived w/o > them for a long time. What's changed now? Someone might have out-of-tree patch that adds yet another format specifier. It might call string() that checks for (null) now but it it won't if we rename it as you suggest. People used to safe string() might miss this when the patch is send upstream for inclusion, ... > > Is _string() really that bad? > > I would think so. OK, I am going to use valid_string() instead of _string(). Feel free to suggest anything better. Just please no bike-shedding. Best Regards, Petr
Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings
On Fri, 2018-04-06 at 11:15 +0200, Petr Mladek wrote: > On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote: > > On 2018-04-04 10:58, Petr Mladek wrote: > > > We are going to check the address using probe_kernel_address(). It > > > will > > > be more expensive and it does not make sense for well known > > > address. > > > > > > This patch splits the string() function. The variant without the > > > check > > > is then used on locations that handle string constants or strings > > > defined > > > as local variables. > > > > > > This patch does not change the existing behavior. > > > > Please leave string() alone, except for moving the < PAGE_SIZE check > > to > > a new helper checked_string (feel free to find a better name), and > > use > > checked_string for handling %s and possibly the few other cases > > where > > we're passing a user-supplied pointer. That avoids cluttering the > > entire > > file with double-underscore calls, and e.g. in the %pO case, it's > > easier > > to understand why one uses two different *string() helpers if the > > name > > of one somehow conveys how it is different from the other. > > I understand your reasoning. I thought about exactly this as well. > My problem is that string() will then be unsafe. It might be dangerous > when porting patches. I agree with Rasmus, and your argument here from my point of view kinda weak. Are we really going to backport this patches? Why? We lived w/o them for a long time. What's changed now? > Is _string() really that bad? I would think so. -- Andy Shevchenko Intel Finland Oy
Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings
On Thu 2018-04-05 15:30:51, Rasmus Villemoes wrote: > On 2018-04-04 10:58, Petr Mladek wrote: > > We are going to check the address using probe_kernel_address(). It will > > be more expensive and it does not make sense for well known address. > > > > This patch splits the string() function. The variant without the check > > is then used on locations that handle string constants or strings defined > > as local variables. > > > > This patch does not change the existing behavior. > > Please leave string() alone, except for moving the < PAGE_SIZE check to > a new helper checked_string (feel free to find a better name), and use > checked_string for handling %s and possibly the few other cases where > we're passing a user-supplied pointer. That avoids cluttering the entire > file with double-underscore calls, and e.g. in the %pO case, it's easier > to understand why one uses two different *string() helpers if the name > of one somehow conveys how it is different from the other. I understand your reasoning. I thought about exactly this as well. My problem is that string() will then be unsafe. It might be dangerous when porting patches. This is why I wanted a different name for the variant without the check. But I was not able to come up with anything short and clear at the same time. Is _string() really that bad? I think that it is a rather common practice to use _func() for functions that are less safe than func() variants. People should use _func() variants with care and this is what we want here. In addition, it is an internal API. IMHO, only few people do changes there. They will get used to it quickly. Which is not true for people that might need to port patches. Best Regards, Petr
Re: [PATCH v4 3/9] vsprintf: Do not check address of well-known strings
On 2018-04-04 10:58, Petr Mladek wrote: > We are going to check the address using probe_kernel_address(). It will > be more expensive and it does not make sense for well known address. > > This patch splits the string() function. The variant without the check > is then used on locations that handle string constants or strings defined > as local variables. > > This patch does not change the existing behavior. Please leave string() alone, except for moving the < PAGE_SIZE check to a new helper checked_string (feel free to find a better name), and use checked_string for handling %s and possibly the few other cases where we're passing a user-supplied pointer. That avoids cluttering the entire file with double-underscore calls, and e.g. in the %pO case, it's easier to understand why one uses two different *string() helpers if the name of one somehow conveys how it is different from the other. Rasmus
[PATCH v4 3/9] vsprintf: Do not check address of well-known strings
We are going to check the address using probe_kernel_address(). It will be more expensive and it does not make sense for well known address. This patch splits the string() function. The variant without the check is then used on locations that handle string constants or strings defined as local variables. This patch does not change the existing behavior. Signed-off-by: Petr Mladek --- lib/vsprintf.c | 79 -- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 9ade2c4f21ba..dd71738d7a09 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -582,14 +582,11 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec) } static noinline_for_stack -char *string(char *buf, char *end, const char *s, struct printf_spec spec) +char *__string(char *buf, char *end, const char *s, struct printf_spec spec) { int len = 0; size_t lim = spec.precision; - if ((unsigned long)s < PAGE_SIZE) - s = "(null)"; - while (lim--) { char c = *s++; if (!c) @@ -602,6 +599,16 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec) return widen_string(buf, len, end, spec); } +static noinline_for_stack +char *string(char *buf, char *end, const char *s, + struct printf_spec spec) +{ + if ((unsigned long)s < PAGE_SIZE) + s = "(null)"; + + return __string(buf, end, s, spec); +} + static bool have_filled_random_ptr_key __read_mostly; static siphash_key_t ptr_key __read_mostly; @@ -646,7 +653,7 @@ static char *ptr_to_id(char *buf, char *end, if (unlikely(!have_filled_random_ptr_key)) { spec.field_width = default_width; /* string length must be less than default_width */ - return string(buf, end, "(ptrval)", spec); + return __string(buf, end, "(ptrval)", spec); } #ifdef CONFIG_64BIT @@ -755,7 +762,7 @@ char *symbol_string(char *buf, char *end, void *ptr, else sprint_symbol_no_offset(sym, value); - return string(buf, end, sym, spec); + return __string(buf, end, sym, spec); #else return special_hex_number(buf, end, value, sizeof(void *)); #endif @@ -821,27 +828,27 @@ char *resource_string(char *buf, char *end, struct resource *res, *p++ = '['; if (res->flags & IORESOURCE_IO) { - p = string(p, pend, "io ", str_spec); + p = __string(p, pend, "io ", str_spec); specp = &io_spec; } else if (res->flags & IORESOURCE_MEM) { - p = string(p, pend, "mem ", str_spec); + p = __string(p, pend, "mem ", str_spec); specp = &mem_spec; } else if (res->flags & IORESOURCE_IRQ) { - p = string(p, pend, "irq ", str_spec); + p = __string(p, pend, "irq ", str_spec); specp = &dec_spec; } else if (res->flags & IORESOURCE_DMA) { - p = string(p, pend, "dma ", str_spec); + p = __string(p, pend, "dma ", str_spec); specp = &dec_spec; } else if (res->flags & IORESOURCE_BUS) { - p = string(p, pend, "bus ", str_spec); + p = __string(p, pend, "bus ", str_spec); specp = &bus_spec; } else { - p = string(p, pend, "??? ", str_spec); + p = __string(p, pend, "??? ", str_spec); specp = &mem_spec; decode = 0; } if (decode && res->flags & IORESOURCE_UNSET) { - p = string(p, pend, "size ", str_spec); + p = __string(p, pend, "size ", str_spec); p = number(p, pend, resource_size(res), *specp); } else { p = number(p, pend, res->start, *specp); @@ -852,21 +859,21 @@ char *resource_string(char *buf, char *end, struct resource *res, } if (decode) { if (res->flags & IORESOURCE_MEM_64) - p = string(p, pend, " 64bit", str_spec); + p = __string(p, pend, " 64bit", str_spec); if (res->flags & IORESOURCE_PREFETCH) - p = string(p, pend, " pref", str_spec); + p = __string(p, pend, " pref", str_spec); if (res->flags & IORESOURCE_WINDOW) - p = string(p, pend, " window", str_spec); + p = __string(p, pend, " window", str_spec); if (res->flags & IORESOURCE_DISABLED) - p = string(p, pend, " disabled", str_spec); + p = __string(p, pend, " disabled", str_spec); } else { - p = string(p, pend, " flags ", str_spec); + p = __string(p, pend, " flags ", str_spec); p = number(p, pend, res->flags,