* Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
@ 2018-03-06 18:22 Alexey Dobriyan
2018-03-06 18:42 ` Adam Borowski
0 siblings, 1 reply; 6+ messages in thread
From: Alexey Dobriyan @ 2018-03-06 18:22 UTC (permalink / raw)
To: kilobyte; +Cc: linux-kernel
> +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
This is getting ridiculous.
Instead of simply printing a pointer as %08lx or %016llx, not only glibc
(null) stupidity is propagated but expanded and "improved".
I assure you reading 0000000000000000 is just as obvious as (null) and
reading fffffffffffffffa is just as good as -ENOMEM.
In fact printing with hex is more information. Maybe it is important
that buggy pointer is small value but it's value is lost.
Sure don't dereference a pointer for very small or very erry values
but print it without all the bell and whistles.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
2018-03-06 18:22 [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs Alexey Dobriyan
@ 2018-03-06 18:42 ` Adam Borowski
0 siblings, 0 replies; 6+ messages in thread
From: Adam Borowski @ 2018-03-06 18:42 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: linux-kernel
On Tue, Mar 06, 2018 at 09:22:17PM +0300, Alexey Dobriyan wrote:
> > +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
>
> This is getting ridiculous.
>
> Instead of simply printing a pointer as %08lx or %016llx, not only glibc
> (null) stupidity is propagated but expanded and "improved".
This is not about printing a pointer, this is about attempting to print an
object referenced by such a bad pointer. Which leads to a crash: in
userspace, you get a segfault; in the kernel, at least in the case I tested,
the system is dead without even a squeal on either console or serial.
> I assure you reading 0000000000000000 is just as obvious as (null) and
> reading fffffffffffffffa is just as good as -ENOMEM.
>
> In fact printing with hex is more information. Maybe it is important
> that buggy pointer is small value but it's value is lost.
>
> Sure don't dereference a pointer for very small or very erry values
> but print it without all the bell and whistles.
That's a reasonable suggestion, but it still needs to be special cased.
Note the difference between printk("%px", 42) and printk("%s", 42).
See this part:
- if (!ptr && *fmt != 'K' && *fmt != 'x') {
+ if (IS_BAD_PTR(ptr) && *fmt != 'K' && *fmt != 'x') {
Printing the pointer is already excluded; what I'm fixing is:
1. lying that the bad pointer was (null) when it was -ENOMEM (commit 1)
2. crash when some bad code tries to printk("%s", -ENOMEM) (commit 2)
So, if what you propose is applying commit 2, and changing 1 to print the
raw value instead of (null)/(err)/(invalid), that sounds good.
Meow!
--
⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can.
⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener.
⠈⠳⣄⠀⠀⠀⠀ A master species delegates.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
2018-03-07 13:17 ` Andy Shevchenko
@ 2018-03-07 13:42 ` Adam Borowski
0 siblings, 0 replies; 6+ messages in thread
From: Adam Borowski @ 2018-03-07 13:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Petr Mladek, Rasmus Villemoes, Tobin C . Harding, Joe Perches,
linux-kernel, Andrew Morton, Michal Hocko
On Wed, Mar 07, 2018 at 03:17:19PM +0200, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
>
> Thanks for the patch, my comments below.
(Review snipped.)
It looks pretty obvious that it'd take a lot less of your time to roll new
patch[es] from scratch than to try to educate me wrt how you'd want to see
it done; thus I'll sit out this one.
Meow!
--
⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ A dumb species has no way to open a tuna can.
⢿⡄⠘⠷⠚⠋⠀ A smart species invents a can opener.
⠈⠳⣄⠀⠀⠀⠀ A master species delegates.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
2018-03-06 18:11 ` [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs Adam Borowski
2018-03-07 13:17 ` Andy Shevchenko
@ 2018-03-07 13:29 ` Andy Shevchenko
1 sibling, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2018-03-07 13:29 UTC (permalink / raw)
To: Adam Borowski, Petr Mladek, Rasmus Villemoes, Tobin C . Harding,
Joe Perches, linux-kernel, Andrew Morton, Michal Hocko
On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
> Attempting to print an object pointed to by a bad (usually ERR_PTR)
> pointer
> is a not so surprising error. Our code handles them inconsistently:
> * two places print (null) if ptr<PAGE_SIZE
> * one place prints (null) if abs(ptr)<PAGE_SIZE
> * one place prints (null) only if !ptr
>
> Obviously, saying (null) for a small but non-0 value is misleading.
> Thus, let's print:
> * (null) for exactly 0
> * (err) if last page && abs(ptr)<=MAX_ERRNO
> * (invalid) otherwise
Ah, and last but not least thing.
Where are the test cases?
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
2018-03-06 18:11 ` [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs Adam Borowski
@ 2018-03-07 13:17 ` Andy Shevchenko
2018-03-07 13:42 ` Adam Borowski
2018-03-07 13:29 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2018-03-07 13:17 UTC (permalink / raw)
To: Adam Borowski, Petr Mladek, Rasmus Villemoes, Tobin C . Harding,
Joe Perches, linux-kernel, Andrew Morton, Michal Hocko
On Tue, 2018-03-06 at 19:11 +0100, Adam Borowski wrote:
Thanks for the patch, my comments below.
> Attempting to print an object pointed to by a bad (usually ERR_PTR)
> pointer
> is a not so surprising error. Our code handles them inconsistently:
> * two places print (null) if ptr<PAGE_SIZE
> * one place prints (null) if abs(ptr)<PAGE_SIZE
> * one place prints (null) only if !ptr
>
> Obviously, saying (null) for a small but non-0 value is misleading.
> Thus, let's print:
> * (null) for exactly 0
> * (err) if last page && abs(ptr)<=MAX_ERRNO
> * (invalid) otherwise
>
First of all, this patch is much more arguable than the other one in
your small series.
"(invalid)" is invalid. Hint: there is a nice comment in the code why.
I'm in principle not putting explanation here to insist people to
eventually _read and understand_ the code before doing anything.
Some comments below.
> +#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" :
> "(invalid)")
It looks ugly.
> /**
> * simple_strtoull - convert a string to an unsigned long long
> * @cp: The start of the string
> @@ -588,7 +590,7 @@ char *string(char *buf, char *end, const char *s,
> struct printf_spec spec)
> size_t lim = spec.precision;
>
> if ((unsigned long)s < PAGE_SIZE)
> - s = "(null)";
> + s = BAD_PTR_STRING(s);
It doesn't make any sense before your patch 2.
> if ((unsigned long)dn < PAGE_SIZE)
> - return string(buf, end, "(null)", spec);
> + return string(buf, end, BAD_PTR_STRING(dn), spec);
It simple doesn't make sense.
The idea is to do it below, in the pointer.
These certain lines are going to be removed by my patch.
> - return string(buf, end, "(null)", spec);
> + return string(buf, end, BAD_PTR_STRING(ptr), spec);
Doesn't make sense before your patch 2.
> if ((unsigned long)save_str > (unsigned
> long)-PAGE_SIZE
> || (unsigned long)save_str <
> PAGE_SIZE)
> - save_str = "(null)";
> + save_str = BAD_PTR_STRING(save_str);
This is perhaps one valid change in such situation.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs
2018-03-06 9:25 [PATCH] vsprintf: Make "null" pointer dereference more robust Petr Mladek
@ 2018-03-06 18:11 ` Adam Borowski
2018-03-07 13:17 ` Andy Shevchenko
2018-03-07 13:29 ` Andy Shevchenko
0 siblings, 2 replies; 6+ messages in thread
From: Adam Borowski @ 2018-03-06 18:11 UTC (permalink / raw)
To: Petr Mladek, Rasmus Villemoes, Andy Shevchenko,
Tobin C . Harding, Joe Perches, linux-kernel, Andrew Morton,
Michal Hocko
Cc: Adam Borowski
Attempting to print an object pointed to by a bad (usually ERR_PTR) pointer
is a not so surprising error. Our code handles them inconsistently:
* two places print (null) if ptr<PAGE_SIZE
* one place prints (null) if abs(ptr)<PAGE_SIZE
* one place prints (null) only if !ptr
Obviously, saying (null) for a small but non-0 value is misleading.
Thus, let's print:
* (null) for exactly 0
* (err) if last page && abs(ptr)<=MAX_ERRNO
* (invalid) otherwise
Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
lib/vsprintf.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..1c2c3cc5a321 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -47,6 +47,8 @@
#include <linux/string_helpers.h>
#include "kstrtox.h"
+#define BAD_PTR_STRING(x) (!(x) ? "(null)" : IS_ERR(x) ? "(err)" : "(invalid)")
+
/**
* simple_strtoull - convert a string to an unsigned long long
* @cp: The start of the string
@@ -588,7 +590,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
size_t lim = spec.precision;
if ((unsigned long)s < PAGE_SIZE)
- s = "(null)";
+ s = BAD_PTR_STRING(s);
while (lim--) {
char c = *s++;
@@ -1582,7 +1584,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
return string(buf, end, "(!OF)", spec);
if ((unsigned long)dn < PAGE_SIZE)
- return string(buf, end, "(null)", spec);
+ return string(buf, end, BAD_PTR_STRING(dn), spec);
/* simple case without anything any more format specifiers */
fmt++;
@@ -1851,12 +1853,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
if (!ptr && *fmt != 'K' && *fmt != 'x') {
/*
- * Print (null) with the same width as a pointer so it makes
- * tabular output look nice.
+ * Print (null)/etc with the same width as a pointer so it
+ * makes tabular output look nice.
*/
if (spec.field_width == -1)
spec.field_width = default_width;
- return string(buf, end, "(null)", spec);
+ return string(buf, end, BAD_PTR_STRING(ptr), spec);
}
switch (*fmt) {
@@ -2575,7 +2577,7 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
if ((unsigned long)save_str > (unsigned long)-PAGE_SIZE
|| (unsigned long)save_str < PAGE_SIZE)
- save_str = "(null)";
+ save_str = BAD_PTR_STRING(save_str);
len = strlen(save_str) + 1;
if (str + len < end)
memcpy(str, save_str, len);
--
2.16.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-07 13:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-06 18:22 [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs Alexey Dobriyan
2018-03-06 18:42 ` Adam Borowski
-- strict thread matches above, loose matches on Subject: below --
2018-03-06 9:25 [PATCH] vsprintf: Make "null" pointer dereference more robust Petr Mladek
2018-03-06 18:11 ` [PATCH 1/2] vsprintf: distinguish between (null), (err) and (invalid) pointer derefs Adam Borowski
2018-03-07 13:17 ` Andy Shevchenko
2018-03-07 13:42 ` Adam Borowski
2018-03-07 13:29 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).