Re: [PATCH 2/2] lib/vsprintf: reduce space taken by no_hash_pointers warning
On Mon, Mar 8, 2021 at 4:51 AM Marco Elver wrote:
> If we do __initconst change we need to manually remove the duplicate
> lines because we're asking the compiler to create a large array (and
> there's no more auto-dedup). If we do not remove the duplicate lines,
> the __initconst-only approach would create a larger image and result
> in subtly increased memory consumption during init. The additional
> code together with manual dedup should offset that. (I can split this
> patch as Andy suggests, but first need confirmation what people
> actually want.)
>
> I have no idea what the right trade-off is, and appeal to Geert to
> suggest what would be acceptable to him.
Maybe we can have only the message itself wrapped in an #ifdef CONFIG_
of some kind. For example:
+#ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
pr_warn("**\n");
pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
pr_warn("** **\n");
pr_warn("** This system shows unhashed kernel memory addresses **\n");
...
+#endif
return 0;
}
In other words, if space is really constrained, then don't include the
message. Or maybe just include part of the message.
Re: [PATCH 2/2] lib/vsprintf: reduce space taken by no_hash_pointers warning
On Fri, Mar 5, 2021 at 1:46 PM Marco Elver wrote:
> +static const char no_hash_pointers_warning[8][55] __initconst = {
> + "**",
> + " NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE ",
> + " This system shows unhashed kernel memory addresses ",
> + " via the console, logs, and other interfaces. This",
> + " might reduce the security of your system.",
> + " If you see this message and you are not debugging",
> + " the kernel, report this immediately to your system ",
> + " administrator! ",
> +};
> +
> static int __init no_hash_pointers_enable(char *str)
> {
> + /* Indices into no_hash_pointers_warning; -1 is an empty line. */
> + const int lines[] = { 0, 1, -1, 2, 3, 4, -1, 5, 6, 7, -1, 1, 0 };
You can save a few more bytes by making this an array of s8.
I agree with the __initconst. The rest seems overkill to me, but I
won't reject it.
Acked-by: Timur Tabi
Re: [PATCH 0/3][v4] add support for never printing hashed addresses
On 2/14/21 10:13 AM, Timur Tabi wrote: Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. I believe that this version addresses all outstanding issues, so unless there are any complaints, I would like for this patch set to be merged for 5.12-rc1. I don't know who should pick it up, though. Thanks.
[PATCH 1/3] [v4] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers
Instead of defining the total/failed test counters manually, test drivers that are clients of kselftest should use the macro created for this purpose. Signed-off-by: Timur Tabi Reviewed-by: Petr Mladek Acked-by: Marco Elver --- lib/test_bitmap.c | 3 +-- lib/test_printf.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c index 4425a1dd4ef1..0ea0e8258f14 100644 --- a/lib/test_bitmap.c +++ b/lib/test_bitmap.c @@ -16,8 +16,7 @@ #include "../tools/testing/selftests/kselftest_module.h" -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); static char pbl_buffer[PAGE_SIZE] __initdata; diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..ad2bcfa8caa1 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -30,8 +30,8 @@ #define PAD_SIZE 16 #define FILL_CHAR '$' -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); + static char *test_buffer __initdata; static char *alloced_buffer __initdata; -- 2.25.1
[PATCH 2/3] [v4] kselftest: add support for skipped tests
Update the kselftest framework to allow client drivers to
specify that some tests were skipped.
Signed-off-by: Timur Tabi
Reviewed-by: Petr Mladek
Tested-by: Petr Mladek
Acked-by: Marco Elver
---
tools/testing/selftests/kselftest_module.h | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_module.h
b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
#define KSTM_MODULE_GLOBALS() \
static unsigned int total_tests __initdata;\
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata; \
+static unsigned int skipped_tests __initdata
#define KSTM_CHECK_ZERO(x) do {
\
total_tests++; \
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
} \
} while (0)
-static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests,
+ unsigned int skipped_tests)
{
- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
+ if (failed_tests == 0) {
+ if (skipped_tests) {
+ pr_info("skipped %u tests\n", skipped_tests);
+ pr_info("remaining %u tests passed\n", total_tests);
+ } else
+ pr_info("all %u tests passed\n", total_tests);
+ } else
pr_warn("failed %u out of %u tests\n", failed_tests,
total_tests);
return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void)
\
{ \
pr_info("loaded.\n"); \
selftest(); \
- return kstm_report(total_tests, failed_tests); \
+ return kstm_report(total_tests, failed_tests, skipped_tests); \
} \
static void __exit __module##_exit(void) \
{ \
--
2.25.1
[PATCH 3/3] [v4] lib/vsprintf: no_hash_pointers prints all addresses as unhashed
If the no_hash_pointers command line parameter is set, then
printk("%p") will print pointers as unhashed, which is useful for
debugging purposes. This change applies to any function that uses
vsprintf, such as print_hex_dump() and seq_buf_printf().
A large warning message is displayed if this option is enabled.
Unhashed pointers expose kernel addresses, which can be a security
risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi
Acked-by: Petr Mladek
Acked-by: Randy Dunlap
Acked-by: Sergey Senozhatsky
Acked-by: Vlastimil Babka
Acked-by: Marco Elver
---
.../admin-guide/kernel-parameters.txt | 15
lib/test_printf.c | 8 +
lib/vsprintf.c| 36 +--
3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..c8993a296e71 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3281,6 +3281,21 @@
in certain environments such as networked servers or
real-time systems.
+ no_hash_pointers
+ Force pointers printed to the console or buffers to be
+ unhashed. By default, when a pointer is printed via %p
+ format string, that pointer is "hashed", i.e. obscured
+ by hashing the pointer value. This is a security
feature
+ that hides actual kernel addresses from unprivileged
+ users, but it also makes debugging the kernel more
+ difficult since unequal pointers can no longer be
+ compared. However, if this command-line option is
+ specified, then all normal pointers will have their true
+ value printed. Pointers printed via %pK may still be
+ hashed. This option should only be specified when
+ debugging the kernel. Please do not use on production
+ kernels.
+
nohibernate [HIBERNATION] Disable hibernation and resume.
nohz= [KNL] Boottime enable/disable dynamic ticks
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..a6755798e9e6 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
+extern bool no_hash_pointers;
+
static int __printf(4, 0) __init
do_test(int bufsize, const char *expect, int elen,
const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
{
int err;
+ if (no_hash_pointers) {
+ pr_warn("skipping plain 'p' tests");
+ skipped_tests += 2;
+ return;
+ }
+
err = plain_hash();
if (err) {
pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..41ddc353ebb8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,32 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+bool no_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(no_hash_pointers);
+
+static int __init no_hash_pointers_enable(char *str)
+{
+ no_hash_pointers = true;
+
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** This system shows unhashed kernel memory addresses **\n");
+ pr_warn("** via the console, logs, and other interfaces. This**\n");
+ pr_warn("** might reduce the security of your system.**\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your system **\n");
+ pr_warn("** administrator! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("**\n");
+
+ return 0;
+}
+early_param("no_hash_pointers", no_hash_poin
[PATCH 0/3][v4] add support for never printing hashed addresses
Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. To avoid having to choose between %p and %px, it's easier to add a kernel command line that treats all %p as %px. This encourages developers to use %p more without making debugging more difficult. Patches #1 and #2 upgrade the kselftest framework so that it can report on tests that were skipped outright. This is needed for the test_printf module which will now skip %p hashing tests if hashing is disabled. Patch #2 upgrades the printf library to check the command line. It also updates test_printf(). Timur Tabi (3): [v4] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers [v4] kselftest: add support for skipped tests [v4] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed .../admin-guide/kernel-parameters.txt | 15 lib/test_bitmap.c | 3 +- lib/test_printf.c | 12 +- lib/vsprintf.c| 38 ++- tools/testing/selftests/kselftest_module.h| 18 ++--- 5 files changed, 74 insertions(+), 12 deletions(-) -- 2.25.1
Re: [PATCH 3/3] [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed
On 2/12/21 4:01 AM, Petr Mladek wrote: no_hash_pointers ? I am fine with this. I am still a bit scared of a bikeshedng. But AFAIK, Mathew was most active on proposing clear names. So, when he is fine with this... Anyway, we should use the same name also for the variable. Ok, unless there are any objections, I will change the parameter and variable to "no_hash_pointers" in v4, which I will send out later today. I hope this patch set gets accepted for 5.12.
Re: [PATCH 3/3] [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed
On 2/11/21 11:23 AM, Petr Mladek wrote: There was some pushback against this feature in general. It should be used deliberately and people must be aware of the consequences. This is why it is only boot option and why it prints such a huge warning. The long clear name helps as well. This is my thinking as well. I wanted it to be a bit obnoxious.
Re: [PATCH 3/3] [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed
On 2/11/21 11:53 AM, Petr Mladek wrote:
I would really like to make it clear here that it is not only about
consoles. Most people will see only this message. Only few people read
documentation. Many people will learn the parameter name from another
context by googling.
I know that it is not easy to find good words. Especially because
pointers printed by %pK might still be hashed.
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** reduce the security of your system. **\n");
What about replacing the first two paragraphs with something like:
"This system shows unhashed kernel memory addresses via logs and
other interfaces. It might reduce the security of your system."
That works, thanks. I'll post a v4 soon.
Re: [PATCH 3/3] [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed
On 2/11/21 6:31 AM, Pavel Machek wrote: Can we make this something shorter? Clearly you don't want people placing this in their grub config, so they'll be most likely typing this a lot... debug_pointers or debug_ptrs would be better. dbg_unhash_ptrs? "debug_ptrs" is too vague IMHO, and I want to keep the word "hash" somewhere there to indicate exactly what's happening.
[PATCH 3/3] [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed
If the debug_never_hash_pointers command line parameter is set, then
printk("%p") will print pointers as unhashed, which is useful for
debugging purposes. This also applies to any function that uses
vsprintf, such as print_hex_dump() and seq_buf_printf().
A large warning message is displayed if this option is enabled.
Unhashed pointers expose kernel addresses, which can be a security
risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi
Acked-by: Petr Mladek
Acked-by: Randy Dunlap
Acked-by: Sergey Senozhatsky
Acked-by: Vlastimil Babka
---
.../admin-guide/kernel-parameters.txt | 15
lib/test_printf.c | 8
lib/vsprintf.c| 38 ++-
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..2a97e787f49c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -810,6 +810,21 @@
1 will print _a lot_ more information - normally
only useful to kernel developers.
+ debug_never_hash_pointers
+ Force pointers printed to the console or buffers to be
+ unhashed. By default, when a pointer is printed via %p
+ format string, that pointer is "hashed", i.e. obscured
+ by hashing the pointer value. This is a security
feature
+ that hides actual kernel addresses from unprivileged
+ users, but it also makes debugging the kernel more
+ difficult since unequal pointers can no longer be
+ compared. However, if this command-line option is
+ specified, then all normal pointers will have their true
+ value printed. Pointers printed via %pK may still be
+ hashed. This option should only be specified when
+ debugging the kernel. Please do not use on production
+ kernels.
+
debug_objects [KNL] Enable object debugging
no_debug_objects
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..b0b62d76e598 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
+extern bool debug_never_hash_pointers;
+
static int __printf(4, 0) __init
do_test(int bufsize, const char *expect, int elen,
const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
{
int err;
+ if (debug_never_hash_pointers) {
+ pr_warn("skipping plain 'p' tests");
+ skipped_tests += 2;
+ return;
+ }
+
err = plain_hash();
if (err) {
pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..b4e07ecb1cb2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+bool debug_never_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will**\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** reduce the security of your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your system **\n");
+ pr_warn("** administrator! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE
[PATCH 1/3] [v3] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers
Instead of defining the total/failed test counters manually, test drivers that are clients of kselftest should use the macro created for this purpose. Signed-off-by: Timur Tabi Reviewed-by: Petr Mladek --- lib/test_bitmap.c | 3 +-- lib/test_printf.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c index 4425a1dd4ef1..0ea0e8258f14 100644 --- a/lib/test_bitmap.c +++ b/lib/test_bitmap.c @@ -16,8 +16,7 @@ #include "../tools/testing/selftests/kselftest_module.h" -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); static char pbl_buffer[PAGE_SIZE] __initdata; diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..ad2bcfa8caa1 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -30,8 +30,8 @@ #define PAD_SIZE 16 #define FILL_CHAR '$' -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); + static char *test_buffer __initdata; static char *alloced_buffer __initdata; -- 2.25.1
[PATCH 0/3][v3] add support for never printing hashed addresses
[The list of email addresses on CC: is getting quite lengthy, so I hope I've included everyone.] Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. To avoid having to choose between %p and %px, it's easier to add a kernel command line that treats all %p as %px. This encourages developers to use %p more without making debugging more difficult. Patches #1 and #2 upgrade the kselftest framework so that it can report on tests that were skipped outright. This is needed for the test_printf module which will now skip %p hashing tests if hashing is disabled. Patch #2 upgrades the printf library to check the command line. It also updates test_printf(). Full series: Acked-by: Marco Elver Timur Tabi (3): [v3] lib: use KSTM_MODULE_GLOBALS macro in kselftest drivers [v3] kselftest: add support for skipped tests [v3] lib/vsprintf: debug_never_hash_pointers prints all addresses as unhashed .../admin-guide/kernel-parameters.txt | 15 lib/test_bitmap.c | 3 +- lib/test_printf.c | 12 +- lib/vsprintf.c| 38 ++- tools/testing/selftests/kselftest_module.h| 18 ++--- 5 files changed, 74 insertions(+), 12 deletions(-) -- 2.25.1
[PATCH 2/3] [v3] kselftest: add support for skipped tests
Update the kselftest framework to allow client drivers to
specify that some tests were skipped.
Signed-off-by: Timur Tabi
---
tools/testing/selftests/kselftest_module.h | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_module.h
b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
#define KSTM_MODULE_GLOBALS() \
static unsigned int total_tests __initdata;\
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata; \
+static unsigned int skipped_tests __initdata
#define KSTM_CHECK_ZERO(x) do {
\
total_tests++; \
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
} \
} while (0)
-static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests,
+ unsigned int skipped_tests)
{
- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
+ if (failed_tests == 0) {
+ if (skipped_tests) {
+ pr_info("skipped %u tests\n", skipped_tests);
+ pr_info("remaining %u tests passed\n", total_tests);
+ } else
+ pr_info("all %u tests passed\n", total_tests);
+ } else
pr_warn("failed %u out of %u tests\n", failed_tests,
total_tests);
return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void)
\
{ \
pr_info("loaded.\n"); \
selftest(); \
- return kstm_report(total_tests, failed_tests); \
+ return kstm_report(total_tests, failed_tests, skipped_tests); \
} \
static void __exit __module##_exit(void) \
{ \
--
2.25.1
Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses
On 2/10/21 5:11 AM, Marco Elver wrote: I wanted to test this for deciding if we can show sensitive info in KFENCE reports, which works just fine now that debug_never_hash_pointers is non-static. FWIW, Acked-by: Marco Elver Thank you. But unfortunately this series broke some other test: | In file included from lib/test_bitmap.c:17: | lib/test_bitmap.c: In function ‘test_bitmap_init’: | lib/../tools/testing/selftests/kselftest_module.h:45:48: error: ‘skipped_tests’ undeclared (first use in this function); did you mean ‘failed_tests’? This will be fixed in v3. Thanks.
Re: [PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/10/21 7:41 AM, Petr Mladek wrote:
The option causes that vsprintf() will not hash pointers. Yes, it is
primary used by printk(). But it is used also in some other
interfaces, especially trace_printk(), seq_buf() API. The naked
pointers might appear more or less anywhere, including procfs,
sysfs, debugfs.
Fair point. Shouldn't calls to seq_buf_printf() (and any printk usage
that always exists in the context of a user-space process) use %pK anyway?
Hmmm maybe vsprintf() should automatically replace %p with %pK if it
detects a user-space context?
IMHO, we should fix this. The long discussion was about how to make
this option safe. Users should be aware that it is not only about
the kernel log.
Agreed.
I suggest to rename the parameter "debug_never_hash_pointer" and use
the same name for the parameter and the variable.
Will do.
We also should make the warning more generic. I suggest to replace the
first paragraph with something like:
pr_warn("** The hashing of printed pointers has been disabled **\n");
pr_warn("** for debugging purposes. **\n");
Feel free to use a better wording. I am not a native speaker.
You could have fooled me.
Of course, also kernel-parameters.txt has to be updated accordingly.
Ok.
Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses
On 2/10/21 10:46 AM, Steven Rostedt wrote: Now the question is, why do you need the unhashed pointer? Currently, the instruction pointer is what is fine right? You get the a function and its offset. If there's something that is needed, perhaps we should look at how to fix that, instead of just unhashing all pointers by default. The original version of this patch only fixed print_hex_dump(), because hashed addresses didn't make any sense for that. Each address is incremented by 16 or 32, but since they were all hashed, they may as well have been random numbers.
Re: [PATCH 0/3][RESEND] add support for never printing hashed addresses
On 2/10/21 5:47 AM, Andy Shevchenko wrote: It's a bit hard in some mailers (like Gmail) to see the different versions of your patches. Can you use in the future - either `git format-patch -v ...`, where is a version - or `git format-patch --subject-prefix="PATCH vX / RESEND / etc" ...` ? Yes, of course. Sorry about that. Like I said earlier, I really shouldn't be posting patches late at night, when I will just forget important details.
Re: [PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro
On 2/9/21 11:18 PM, Timur Tabi wrote: Instead of defining the total/failed test counters manually, test_printf should use the kselftest macro created for this purpose. Signed-off-by: Timur Tabi Ugh, I really need to stop sending patches late at night. This is again the wrong email address. I'm sure I'll need to post another version of these patches, so I'll just fix it then.
[PATCH 3/3] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
If the make-printk-non-secret command line parameter is set, then
printk("%p") will print pointers as unhashed. This is useful for
debugging purposes.
A large warning message is displayed if this option is enabled.
Unhashed pointers, while useful for debugging, expose kernel
addresses which can be a security risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi
Acked-by: Petr Mladek
Acked-by: Randy Dunlap
Acked-by: Sergey Senozhatsky
---
.../admin-guide/kernel-parameters.txt | 15
lib/test_printf.c | 8
lib/vsprintf.c| 38 ++-
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a10b545c2070..6962379469e4 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2613,6 +2613,21 @@
different yeeloong laptops.
Example: machtype=lemote-yeeloong-2f-7inch
+make-printk-non-secret
+ Force pointers printed to the console to be unhashed.
+ By default, when a pointer is printed to the kernel
+ console (via %p format string), that pointer is
+ "hashed", i.e. obscured by hashing the pointer value.
+ This is a security feature that hides actual kernel
+ addresses from unprivileged users, but it also makes
+ debugging the kernel more difficult since unequal
+ pointers can no longer be compared. If this option is
+ specified, then all normal pointers will have their
+ true value printed. Pointers printed via %pK may
+ still be hashed. This option should only be specified
+ when debugging the kernel. Please do not use on
+ production kernels.
+
max_addr=nn[KMG][KNL,BOOT,ia64] All physical memory greater
than or equal to this physical address is ignored.
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..b0b62d76e598 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
+extern bool debug_never_hash_pointers;
+
static int __printf(4, 0) __init
do_test(int bufsize, const char *expect, int elen,
const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
{
int err;
+ if (debug_never_hash_pointers) {
+ pr_warn("skipping plain 'p' tests");
+ skipped_tests += 2;
+ return;
+ }
+
err = plain_hash();
if (err) {
pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..1296d9b0b328 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+bool debug_never_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will**\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** reduce the security of your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your system **\n");
+ pr_warn("** administrator! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("
[PATCH 2/3] kselftest: add support for skipped tests
Update the kselftest framework to all testing clients to
specify that some tests were skipped.
Signed-off-by: Timur Tabi
---
tools/testing/selftests/kselftest_module.h | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_module.h
b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
#define KSTM_MODULE_GLOBALS() \
static unsigned int total_tests __initdata;\
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata; \
+static unsigned int skipped_tests __initdata
#define KSTM_CHECK_ZERO(x) do {
\
total_tests++; \
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
} \
} while (0)
-static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests,
+ unsigned int skipped_tests)
{
- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
+ if (failed_tests == 0) {
+ if (skipped_tests) {
+ pr_info("skipped %u tests\n", skipped_tests);
+ pr_info("remaining %u tests passed\n", total_tests);
+ } else
+ pr_info("all %u tests passed\n", total_tests);
+ } else
pr_warn("failed %u out of %u tests\n", failed_tests,
total_tests);
return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void)
\
{ \
pr_info("loaded.\n"); \
selftest(); \
- return kstm_report(total_tests, failed_tests); \
+ return kstm_report(total_tests, failed_tests, skipped_tests); \
} \
static void __exit __module##_exit(void) \
{ \
--
2.25.1
[PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro
Instead of defining the total/failed test counters manually, test_printf should use the kselftest macro created for this purpose. Signed-off-by: Timur Tabi --- lib/test_printf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..ad2bcfa8caa1 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -30,8 +30,8 @@ #define PAD_SIZE 16 #define FILL_CHAR '$' -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); + static char *test_buffer __initdata; static char *alloced_buffer __initdata; -- 2.25.1
[PATCH 0/3][RESEND] add support for never printing hashed addresses
[accidentally sent from the wrong email address, so resending] [The list of email addresses on CC: is getting quite lengthy, so I hope I've included everyone.] Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. To avoid having to choose between %p and %px, it's easier to add a kernel command line that treats all %p as %px. This encourages developers to use %p more without making debugging more difficult. Patches #1 and #2 upgrade the kselftest framework so that it can report on tests that were skipped outright. This is needed for the test_printf module which will now skip %p hashing tests if hashing is disabled. Patch #2 upgrades the printf library to check the command line. It also updates test_printf(). Timur Tabi (3): lib/test_printf: use KSTM_MODULE_GLOBALS macro kselftest: add support for skipped tests [v2] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed .../admin-guide/kernel-parameters.txt | 15 +++ lib/test_printf.c | 12 +- lib/vsprintf.c| 40 ++- tools/testing/selftests/kselftest_module.h| 18 ++--- 4 files changed, 75 insertions(+), 10 deletions(-) -- 2.25.1
[PATCH 3/3] [v2] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
From: Timur Tabi
If the make-printk-non-secret command line parameter is set, then
printk("%p") will print pointers as unhashed. This is useful for
debugging purposes.
A large warning message is displayed if this option is enabled.
Unhashed pointers, while useful for debugging, expose kernel
addresses which can be a security risk.
Also update test_printf to skip the hashed pointer tests if the
command-line option is set.
Signed-off-by: Timur Tabi
Acked-by: Petr Mladek
Acked-by: Randy Dunlap
Acked-by: Sergey Senozhatsky
---
.../admin-guide/kernel-parameters.txt | 15
lib/test_printf.c | 8
lib/vsprintf.c| 38 ++-
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 9e3cdb271d06..e639b0f32a6c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2613,6 +2613,21 @@
different yeeloong laptops.
Example: machtype=lemote-yeeloong-2f-7inch
+make-printk-non-secret
+ Force pointers printed to the console to be unhashed.
+ By default, when a pointer is printed to the kernel
+ console (via %p format string), that pointer is
+ "hashed", i.e. obscured by hashing the pointer value.
+ This is a security feature that hides actual kernel
+ addresses from unprivileged users, but it also makes
+ debugging the kernel more difficult since unequal
+ pointers can no longer be compared. If this option is
+ specified, then all normal pointers will have their
+ true value printed. Pointers printed via %pK may
+ still be hashed. This option should only be specified
+ when debugging the kernel. Please do not use on
+ production kernels.
+
max_addr=nn[KMG][KNL,BOOT,ia64] All physical memory greater
than or equal to this physical address is ignored.
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ad2bcfa8caa1..b0b62d76e598 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -35,6 +35,8 @@ KSTM_MODULE_GLOBALS();
static char *test_buffer __initdata;
static char *alloced_buffer __initdata;
+extern bool debug_never_hash_pointers;
+
static int __printf(4, 0) __init
do_test(int bufsize, const char *expect, int elen,
const char *fmt, va_list ap)
@@ -301,6 +303,12 @@ plain(void)
{
int err;
+ if (debug_never_hash_pointers) {
+ pr_warn("skipping plain 'p' tests");
+ skipped_tests += 2;
+ return;
+ }
+
err = plain_hash();
if (err) {
pr_warn("plain 'p' does not appear to be hashed\n");
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..1296d9b0b328 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,34 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+bool debug_never_hash_pointers __ro_after_init;
+EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will**\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** reduce the security of your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your system **\n");
+ pr_warn("** administrator! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("***
[PATCH 2/3] kselftest: add support for skipped tests
Update the kselftest framework to all testing clients to
specify that some tests were skipped.
Signed-off-by: Timur Tabi
---
tools/testing/selftests/kselftest_module.h | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_module.h
b/tools/testing/selftests/kselftest_module.h
index e8eafaf0941a..e2ea41de3f35 100644
--- a/tools/testing/selftests/kselftest_module.h
+++ b/tools/testing/selftests/kselftest_module.h
@@ -11,7 +11,8 @@
#define KSTM_MODULE_GLOBALS() \
static unsigned int total_tests __initdata;\
-static unsigned int failed_tests __initdata
+static unsigned int failed_tests __initdata; \
+static unsigned int skipped_tests __initdata
#define KSTM_CHECK_ZERO(x) do {
\
total_tests++; \
@@ -21,11 +22,16 @@ static unsigned int failed_tests __initdata
} \
} while (0)
-static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests)
+static inline int kstm_report(unsigned int total_tests, unsigned int
failed_tests,
+ unsigned int skipped_tests)
{
- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
+ if (failed_tests == 0) {
+ if (skipped_tests) {
+ pr_info("skipped %u tests\n", skipped_tests);
+ pr_info("remaining %u tests passed\n", total_tests);
+ } else
+ pr_info("all %u tests passed\n", total_tests);
+ } else
pr_warn("failed %u out of %u tests\n", failed_tests,
total_tests);
return failed_tests ? -EINVAL : 0;
@@ -36,7 +42,7 @@ static int __init __module##_init(void)
\
{ \
pr_info("loaded.\n"); \
selftest(); \
- return kstm_report(total_tests, failed_tests); \
+ return kstm_report(total_tests, failed_tests, skipped_tests); \
} \
static void __exit __module##_exit(void) \
{ \
--
2.25.1
[PATCH 1/3] lib/test_printf: use KSTM_MODULE_GLOBALS macro
Instead of defining the total/failed test counters manually, test_printf should use the kselftest macro created for this purpose. Signed-off-by: Timur Tabi --- lib/test_printf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..ad2bcfa8caa1 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -30,8 +30,8 @@ #define PAD_SIZE 16 #define FILL_CHAR '$' -static unsigned total_tests __initdata; -static unsigned failed_tests __initdata; +KSTM_MODULE_GLOBALS(); + static char *test_buffer __initdata; static char *alloced_buffer __initdata; -- 2.25.1
[PATCH 0/3] add support for never printing hashed addresses
[The list of email addresses on CC: is getting quite lengthy, so I hope I've included everyone.] Although hashing addresses printed via printk does make the kernel more secure, it interferes with debugging, especially with some functions like print_hex_dump() which always uses hashed addresses. To avoid having to choose between %p and %px, it's easier to add a kernel command line that treats all %p as %px. This encourages developers to use %p more without making debugging more difficult. Patches #1 and #2 upgrade the kselftest framework so that it can report on tests that were skipped outright. This is needed for the test_printf module which will now skip %p hashing tests if hashing is disabled. Patch #2 upgrades the printf library to check the command line. It also updates test_printf(). Timur Tabi (3): lib/test_printf: use KSTM_MODULE_GLOBALS macro kselftest: add support for skipped tests [v2] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed .../admin-guide/kernel-parameters.txt | 15 +++ lib/test_printf.c | 12 +- lib/vsprintf.c| 40 ++- tools/testing/selftests/kselftest_module.h| 18 ++--- 4 files changed, 75 insertions(+), 10 deletions(-) -- 2.25.1
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/9/21 3:59 PM, Marco Elver wrote: Would it be reasonable to make this non-static? Or somehow make it possible to get this flag from other subsystems? There are other places in the kernel that dump sensitive data such as registers. We'd like to be able to use 'debug_never_hash_pointers' to decide if our debugging tools can dump registers etc. What we really need is info if the kernel is in debug mode and we can dump all kinds of sensitive info; debug_never_hash_pointers is would be a good enough proxy for that. The next version of my patch (coming soon) will export the symbol. It's intended for test_printf, but if you think it can be used by others, so much the better.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/5/21 4:59 AM, Vlastimil Babka wrote: Thanks a lot. Should this also affect %pK though? IIUC, there's currently no way to achieve non-mangled %pK in all cases, even with the most permissive kptr_restrict=1 setting: - in IRQ, there's "pK-error" instead - in a context of non-CAP_SYSLOG process, nulls are printed Hmmm.. I thought %pK prints an unhashed pointer when the user is root, at least in situations where the user can be known (e.g. during an ioctl call). Yes, neither should matter if %pK were only used for prints that generate content of some kind of /proc file read by a CAP_SYSLOG process, but that doesn't seem to be the case and there are %pK used for printing to dmesg too... I thought about that. On one hand, people who use %pK probably really wanted a hashed pointer printed. On the other hand, I agree that %pK should not be used for dmesg prints. I get the feeling that some (most?) people who use %pK don't really understand how it's supposed to be used. I can extend make-printk-non-secret to %pK if everyone agrees.
Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/4/21 4:17 PM, Kees Cook wrote: It's just semantics. Printing addresses DOES weaken the security of a system, especially when we know attackers have and do use stuff from dmesg to tune their attacks. How about "reduces the security of your system"? I think we're bikeshedding now, but I can replace "compromise" with "reduce". "Kernel memory addresses are exposed, which may reduce the security of your system."
Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/4/21 3:49 PM, Pavel Machek wrote: This machine is insecure. Yet I don't see ascii-art *** all around.. "Kernel memory addresses are exposed, which is bad for security." I'll use whatever wording everyone can agree on, but I really don't see much difference between "which may compromise security on your system" and "which is bad for security". "may compromise" doesn't see any more alarmist than "bad". Frankly, "bad" is a very generic term. I think the reason behind the large banner has less to do how insecure the system is, and more about making sure vendors and sysadmins don't enable it by default everywhere.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/3/21 2:47 PM, Steven Rostedt wrote:
static void __init
plain(void)
{
int err;
+ if (debug_never_hash_pointers)
+ return;
So, I have a stupid question. What's the best way for test_printf.c to
read the command line parameter? Should I just do this in vsprintf.c:
/* Disable pointer hashing if requested */
static bool debug_never_hash_pointers __ro_after_init;
EXPORT_SYMBOL_GPL(debug_never_hash_pointers);
I'm not crazy about exporting this variable to other drivers. It could
be used to disable hashing by any driver.
AFAIK, the only command-line parameter code that works in drivers is
module_parm, and that expects the module prefix on the command-line.
Re: [PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/3/21 7:31 AM, Petr Mladek wrote: Also please make sure that lib/test_printf.c will work with the new option. As you suspected, it doesn't work: [ 206.966478] test_printf: loaded. [ 206.966528] test_printf: plain 'p' does not appear to be hashed [ 206.966740] test_printf: failed 1 out of 388 tests What should I do about this? On one hand, it is working as expected: %p is not hashed, and that should be a warning. On the other hand, maybe test_printf should be aware of the command line parameter and test to make sure that %p is NOT hashed?
Re: [PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
On 2/2/21 3:52 PM, Kees Cook wrote: A large warning message is displayed if this option is enabled, because unhashed addresses, while useful for debugging, exposes kernel addresses which can be a security risk. Linus has expressly said "no" to things like this in the past: https://lore.kernel.org/lkml/ca+55afwiec1-nas+nfq9rtwar8ef9hwa4mjnbwl41f-8wm4...@mail.gmail.com/ Maybe I misunderstood, but I thought this is what Vlastimil, Petr, Sergey, John, and Steven asked for.
[PATCH][RESEND] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
If the make-printk-non-secret command-line parameter is set, then
printk("%p") will print addresses as unhashed. This is useful for
debugging purposes.
A large warning message is displayed if this option is enabled,
because unhashed addresses, while useful for debugging, exposes
kernel addresses which can be a security risk.
Signed-off-by: Timur Tabi
---
lib/vsprintf.c | 34 --
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..b9f87084afb0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,30 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+static bool debug_never_hash_pointers __ro_after_init;
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will**\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** compromise security on your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your vendor! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("**\n");
+ return 0;
+}
+early_param("make-printk-non-secret", debug_never_hash_pointers_enable);
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -2297,8 +2321,14 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
}
}
- /* default is to _not_ leak addresses, hash before printing */
- return ptr_to_id(buf, end, ptr, spec);
+ /*
+* default is to _not_ leak addresses, so hash before printing, unless
+* make-printk-non-secret is specified on the command line.
+*/
+ if (unlikely(debug_never_hash_pointers))
+ return pointer_string(buf, end, ptr, spec);
+ else
+ return ptr_to_id(buf, end, ptr, spec);
}
/*
--
2.25.1
[PATCH] lib/vsprintf: make-printk-non-secret printks all addresses as unhashed
If the make-printk-non-secret command-line parameter is set, then
printk("%p") will print addresses as unhashed. This is useful for
debugging purposes.
A large warning message is displayed if this option is enabled,
because unhashed addresses, while useful for debugging, exposes
kernel addresses which can be a security risk.
Signed-off-by: Timur Tabi
---
lib/vsprintf.c | 34 --
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..b9f87084afb0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2090,6 +2090,30 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
return widen_string(buf, buf - buf_start, end, spec);
}
+/* Disable pointer hashing if requested */
+static bool debug_never_hash_pointers __ro_after_init;
+
+static int __init debug_never_hash_pointers_enable(char *str)
+{
+ debug_never_hash_pointers = true;
+ pr_warn("**\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("** **\n");
+ pr_warn("** All pointers that are printed to the console will**\n");
+ pr_warn("** be printed as unhashed. **\n");
+ pr_warn("** **\n");
+ pr_warn("** Kernel memory addresses are exposed, which may **\n");
+ pr_warn("** compromise security on your system. **\n");
+ pr_warn("** **\n");
+ pr_warn("** If you see this message and you are not debugging**\n");
+ pr_warn("** the kernel, report this immediately to your vendor! **\n");
+ pr_warn("** **\n");
+ pr_warn("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n");
+ pr_warn("**\n");
+ return 0;
+}
+early_param("make-printk-non-secret", debug_never_hash_pointers_enable);
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -2297,8 +2321,14 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
}
}
- /* default is to _not_ leak addresses, hash before printing */
- return ptr_to_id(buf, end, ptr, spec);
+ /*
+* default is to _not_ leak addresses, so hash before printing, unless
+* make-printk-non-secret is specified on the command line.
+*/
+ if (unlikely(debug_never_hash_pointers))
+ return pointer_string(buf, end, ptr, spec);
+ else
+ return ptr_to_id(buf, end, ptr, spec);
}
/*
--
2.25.1
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/26/21 10:47 AM, Vlastimil Babka wrote: Given Linus' current stance later in this thread, could we revive the idea of a boot time option, or at least a CONFIG (I assume a runtime toggle would be too much, even if limited to !kernel_lockdown:) , that would disable all hashing? It would be really useful for a development/active debugging, as evidenced below. Thanks. So you're saying: if CONFIG_PRINTK_NEVER_HASH is disabled, then %p prints hashed addresses and %px prints unhashed. If CONFIG_PRINTK_NEVER_HASH is enabled, then %p and %px both print unhashed addresses. I like this idea, and I would accept it as a solution if I had to, but I still would also like for an option for print_hex_dump() to print unhashed addresses even when CONFIG_PRINTK_NEVER_HASH is disabled. I can't always recompile the entire kernel for my testing purposes. The only drawback to this idea is: what happens if distros start enabling CONFIG_PRINTK_NEVER_HASH by default, just because it makes debugging easier?
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/26/21 8:11 PM, Sergey Senozhatsky wrote: +1 for boot param with a scary name and dmesg WARNING WARNING WARNING that should look even scarier. Timur, do you have time to take a look and submit a patch? Yes, I'll submit a patch.
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/26/21 11:14 AM, Vlastimil Babka wrote: If it was a boot option, I would personally be for leaving hashing enabled by default, with opt-in boot option to disable it. A boot option would solve all my problems. I wouldn't need to recompile the kernel, and it would apply to all variations of printk.
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/19/21 3:15 PM, Steven Rostedt wrote: When it's not related to any symbol, doesn't it still produce an offset with something close by, that could still give you information that's better than a hashed number. No. I often need the actual unhashed address in the hex dump so that I can see if some other pointer is correct. For example, I could be doing pointer math to calculate the address of some data inside a block. In this case, I would %px the pointer, and then hex_dump the block. I can then see not only where inside the block the pointer is pointing to, but what data it points to.
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/19/21 2:10 PM, Steven Rostedt wrote: I'm curious, what is the result if you replaced %p with %pS? That way you get a kallsyms offset version of the output, which could still be very useful depending on what you are dumping. %pS versatile_init+0x0/0x110 The address is question is often not related to any symbol, so it wouldn't make sense to use %pS. Maybe you meant %pK? I'm okay with that instead of %px.
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/19/21 1:45 PM, Kees Cook wrote:
How about this so the base address is hashed once, with the offset added
to it for each line instead of each line having a "new" hash that makes
no sense:
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 9301578f98e8..20264828752d 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -242,12 +242,17 @@ void print_hex_dump(const char *level, const char
*prefix_str, int prefix_type,
const void *buf, size_t len, bool ascii)
{
const u8 *ptr = buf;
+ const u8 *addr;
int i, linelen, remaining = len;
unsigned char linebuf[32 * 3 + 2 + 32 + 1];
if (rowsize != 16 && rowsize != 32)
rowsize = 16;
+ if (prefix_type == DUMP_PREFIX_ADDRESS &&
+ ptr_to_hashval(ptr, &addr))
+ addr = 0;
+
for (i = 0; i < len; i += rowsize) {
linelen = min(remaining, rowsize);
remaining -= rowsize;
@@ -258,7 +263,7 @@ void print_hex_dump(const char *level, const char
*prefix_str, int prefix_type,
switch (prefix_type) {
case DUMP_PREFIX_ADDRESS:
printk("%s%s%p: %s\n",
- level, prefix_str, ptr + i, linebuf);
+ level, prefix_str, addr + i, linebuf);
Well, this is better than nothing, but not by much. Again, as long as
%px exists for printk(), I just cannot understand any resistance to
allowing it in print_hex_dump().
Frankly, I think this patch and my patch should both be added. During
debugging, it's very difficult if not impossible to work with hashed
addresses. I use print_hex_dump() with an unhashed address all the
time, either by applying my patch to my own kernel or just replacing the
%p with %px.
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/18/21 6:53 PM, Sergey Senozhatsky wrote: I like the idea of a more radical name, e.g. DUMP_PREFIX_RAW_POINTERS or something similar. Is "raw pointer" the common term for unhashed pointers?
Re: [PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
On 1/18/21 12:26 PM, Matthew Wilcox wrote: Don't make it easy. And don't make it look like they're doing something innocent. DUMP_PREFIX_SECURITY_HOLE would be OK by me. DUMP_PREFIX_LEAK_INFORMATION would work fine too. DUMP_PREFIX_MAKE_ATTACKERS_LIFE_EASY might be a bit too far. It's already extremely easy to replace %p with %px in your own printks, so I don't really understand your argument. Seriously, this patch should not be so contentious. If you want hashed addresses, then nothing changes. If you need unhashed addresses while debugging, then use DUMP_PREFIX_UNHASHED. Just like you can use %px in printk. I never use %p in my printks, but then I never submit code upstream that prints addresses, hashed or unhashed.
Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
On 1/18/21 11:14 AM, Andy Shevchenko wrote: But isn't it good to expose those issues (and fix them)? I suppose. Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too long. I think DUMP_PREFIX_ADDRESS_UNHASHED is too long. What about introducing new two like these: DUMP_PREFIX_OFFSET, DUMP_PREFIX_ADDRESS, DUMP_PREFIX_ADDR_UNHASHED, DUMP_PREFIX_ADDR_HASHED, I think we're approaching bike-shedding. DUMP_PREFIX_ADDR_HASHED and DUMP_PREFIX_ADDRESS are the same thing. I don't want people to have to move from DUMP_PREFIX_ADDRESS to some other enum for no change in functionality. I'm willing to rearrange the code so that it's enumerated more consistently, but I don't think there's anything wrong with DUMP_PREFIX_UNHASHED. It's succinct and clear.
Re: [PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
On 1/18/21 4:03 AM, Andy Shevchenko wrote: On Sun, Jan 17, 2021 at 12:12 AM Timur Tabi wrote: (Hint: -v to the git format-patch will create a versioned subject prefix for you automatically) I like to keep the version in the git repo itself so that I don't need to keep track of it separately, but thanks for the hint. I might use it somewhere else. Hashed addresses are useless in hexdumps unless you're comparing with other hashed addresses, which is unlikely. However, there's no need to break existing code, so introduce a new prefix type that prints unhashed addresses. Any user of this? (For the record, I don't see any other mail except this one) It's patch #2 of this set. They were all sent together. http://lkml.iu.edu/hypermail/linux/kernel/2101.2/00245.html Let me know what you think. DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS, - DUMP_PREFIX_OFFSET + DUMP_PREFIX_OFFSET, + DUMP_PREFIX_UNHASHED, Since it's an address, I would like to group them together, i.e. put after DUMP_PREFIX_ADDRESS. I didn't want to change the numbering of any existing enums, just in case there are users that accidentally hard-code the values. I'm trying to make this patch as unobtrusive as possible. > Perhaps even add _ADDRESS to DUMP_PREFIX_UNHASHED, but this maybe too long. I think DUMP_PREFIX_ADDRESS_UNHASHED is too long. + * @prefix_type: controls whether prefix of an offset, hashed address, + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) Yeah, exactly, here you use different ordering. That's because it's a comment. + * @prefix_type: controls whether prefix of an offset, hashed address, + * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET, + * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE) In both cases I would rather use colon and list one per line. What do you think? H if I'm going to change the patch anyway, sure. + case DUMP_PREFIX_UNHASHED: Here is a third type of ordering, can you please be consistent? case DUMP_PREFIX_ADDRESS: Fair enough.
[PATCH 1/2] [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
Hashed addresses are useless in hexdumps unless you're comparing
with other hashed addresses, which is unlikely. However, there's
no need to break existing code, so introduce a new prefix type
that prints unhashed addresses.
Signed-off-by: Timur Tabi
Reviewed-by: Petr Mladek
Cc: Roman Fietze
---
fs/seq_file.c | 3 +++
include/linux/printk.h | 8 +---
lib/hexdump.c | 9 +++--
lib/seq_buf.c | 9 +++--
4 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 03a369ccd28c..b5b49a855894 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -864,6 +864,9 @@ void seq_hex_dump(struct seq_file *m, const char
*prefix_str, int prefix_type,
remaining -= rowsize;
switch (prefix_type) {
+ case DUMP_PREFIX_UNHASHED:
+ seq_printf(m, "%s%px: ", prefix_str, ptr + i);
+ break;
case DUMP_PREFIX_ADDRESS:
seq_printf(m, "%s%p: ", prefix_str, ptr + i);
break;
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..d3c08095a9a3 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -567,7 +567,8 @@ extern const struct file_operations kmsg_fops;
enum {
DUMP_PREFIX_NONE,
DUMP_PREFIX_ADDRESS,
- DUMP_PREFIX_OFFSET
+ DUMP_PREFIX_OFFSET,
+ DUMP_PREFIX_UNHASHED,
};
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
@@ -612,8 +613,9 @@ static inline void print_hex_dump_debug(const char
*prefix_str, int prefix_type,
* print_hex_dump_bytes - shorthand form of print_hex_dump() with default
params
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
* @buf: data blob to dump
* @len: number of bytes in the @buf
*
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 9301578f98e8..b5acfc4168a8 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -211,8 +211,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* @level: kernel log level (e.g. KERN_DEBUG)
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @buf: data blob to dump
@@ -256,6 +257,10 @@ void print_hex_dump(const char *level, const char
*prefix_str, int prefix_type,
linebuf, sizeof(linebuf), ascii);
switch (prefix_type) {
+ case DUMP_PREFIX_UNHASHED:
+ printk("%s%s%px: %s\n",
+ level, prefix_str, ptr + i, linebuf);
+ break;
case DUMP_PREFIX_ADDRESS:
printk("%s%s%p: %s\n",
level, prefix_str, ptr + i, linebuf);
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 707453f5d58e..017c4d7e93f1 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -335,8 +335,9 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf,
int cnt)
* @s: seq_buf descriptor
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @buf: data blob to dump
@@ -374,6 +375,10 @@ int seq_buf_hex_dump(struct seq_buf *s, const char
*prefix_str, int prefix_type,
linebuf, sizeof(linebuf), ascii);
switch (prefix_type) {
+ case DUMP_PREFIX_UNHASHED:
+
[PATCH 2/2] mm/page_poison: use unhashed address in hexdump for check_poison_mem()
Now that print_hex_dump() is capable of printing unhashed addresses,
use that feature in the code that reports memory errors. Hashed
addresses don't tell you where the corrupted page actually is.
Signed-off-by: Timur Tabi
---
mm/page_poison.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 65cdf844c8ad..4902f3261ee4 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -67,7 +67,7 @@ static void check_poison_mem(unsigned char *mem, size_t bytes)
else
pr_err("pagealloc: memory corruption\n");
- print_hex_dump(KERN_ERR, "", DUMP_PREFIX_ADDRESS, 16, 1, start,
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_UNHASHED, 16, 1, start,
end - start + 1, 1);
dump_stack();
}
--
2.25.1
[PATCH 0/2] introduce DUMP_PREFIX_UNHASHED for hex dumps
First patch updates print_hex_dump() and related functions to allow callers to print hex dumps with unhashed addresses. It adds a new prefix type, so existing code is unchanged. Second patch changes a page poising function to use the new address type. This is just an example of a change. If it's wrong, it doesn't need to be applied. IMHO, hashed addresses make very little sense for hex dumps, which print addresses in 16- or 32-byte increments. Typical use-case is to correlate an addresses in between one of these increments with some other address, but that can't be done if the addresses are hashed. I expect most developers to want to replace their usage of DUMP_PREFIX_ADDRESS with DUMP_PREFIX_UNHASHED, now that they have the opportunity. Timur Tabi (2): [v2] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses mm/page_poison: use unhashed address in hexdump for check_poison_mem() fs/seq_file.c | 3 +++ include/linux/printk.h | 8 +--- lib/hexdump.c | 9 +++-- lib/seq_buf.c | 9 +++-- mm/page_poison.c | 2 +- 5 files changed, 23 insertions(+), 8 deletions(-) -- 2.25.1
Re: [PATCH] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
On Fri, Jan 15, 2021 at 3:24 AM Petr Mladek wrote: > By other words, every print_hex_dump() used need to be reviewed in > which context might be called. I did a rough analysis of all current usage of DUMP_PREFIX_ADDRESS in the kernel, compared to the introduction of %px and hashed addresses, using git-blame to find the most recent commit that modifies a line that contains _ADDRESS. Of 150 cases, 110 of them are newer than %px, so I'm assuming that these developers chose _ADDRESS instead of _OFFSET knowing that the addresses are hashed. > > If you want, I can include a patch that changes a few callers of > > print_hex_dump() to use DUMP_PREFIX_UNHASHED, based on what I think would be > > useful. > > It would be nice. I have a v2 that I'm about to post, and I included a patch that modifies check_poison_mem() in mm/page_poison.c. I chose this file because I figured actual addresses would probably be necessary for tracking memory-related errors. Also, that call to print_hex_dump() was added back in 2009, so it predates the introduction of %px.
Re: [PATCH] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
On 1/11/21 7:30 PM, Andrew Morton wrote: I doubt if Kees (or I or anyone else) can review this change because there are no callers which actually use the new DUMP_PREFIX_UNHASHED. Is it intended that some other places in the kernel be changed to use this? If so, please describe where and why, so that others can better understand both the requirement and the security implications. In my opinion, hashed addresses make no sense in a hexdump, so I would say that ALL callers should change. But none of the drivers I've written call print_hex_dump(), so I can't make those changes myself. If it is intended that this be used mainly for developer debug and not to be shipped in the mainline kernel then let's get this info into the changelog as well. I definitely want this patch included in the mainline kernel. Just because there aren't any users today doesn't mean that there won't be. In fact, I suspect that most current users haven't noticed that the addresses have changed or don't care any more, but if they were to write the code today, they would use unhashed addresses. If you want, I can include a patch that changes a few callers of print_hex_dump() to use DUMP_PREFIX_UNHASHED, based on what I think would be useful.
[PATCH] lib/hexdump: introduce DUMP_PREFIX_UNHASHED for unhashed addresses
Hashed addresses are useless in hexdumps unless you're comparing
with other hashed addresses, which is unlikely. However, there's
no need to break existing code, so introduce a new prefix type
that prints unhashed addresses.
Signed-off-by: Timur Tabi
Cc: Roman Fietze
---
include/linux/printk.h | 3 ++-
lib/hexdump.c | 9 +++--
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..5d833bad785c 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -567,7 +567,8 @@ extern const struct file_operations kmsg_fops;
enum {
DUMP_PREFIX_NONE,
DUMP_PREFIX_ADDRESS,
- DUMP_PREFIX_OFFSET
+ DUMP_PREFIX_OFFSET,
+ DUMP_PREFIX_UNHASHED,
};
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 9301578f98e8..b5acfc4168a8 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -211,8 +211,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* @level: kernel log level (e.g. KERN_DEBUG)
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @prefix_type: controls whether prefix of an offset, hashed address,
+ * unhashed address, or none is printed (%DUMP_PREFIX_OFFSET,
+ * %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_UNHASHED, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @buf: data blob to dump
@@ -256,6 +257,10 @@ void print_hex_dump(const char *level, const char
*prefix_str, int prefix_type,
linebuf, sizeof(linebuf), ascii);
switch (prefix_type) {
+ case DUMP_PREFIX_UNHASHED:
+ printk("%s%s%px: %s\n",
+ level, prefix_str, ptr + i, linebuf);
+ break;
case DUMP_PREFIX_ADDRESS:
printk("%s%s%p: %s\n",
level, prefix_str, ptr + i, linebuf);
--
2.25.1
Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
On 9/18/20 9:21 AM, Viorel Suman (OSS) wrote:
+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /*
+one bit 6, 12 ? */
What's the meaning of the comments?
Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout
when
one bit audio stream is transmitted - I was wandering how can this be enforced.
Is a @todo like of comment.
Please don't add comments that other developers could never understand.
The text that you just wrote here would be a great starting point for a
better comment.
Re: [PATCH] print_hex_dump: use %px when using DUMP_PREFIX_ADDRESS
On Wed, Jun 24, 2020 at 7:23 AM Roman Fietze wrote: > > This function is mainly used for debugging. But for that purpose the > hashed memory address of the dumped data provides no useful > information at all. > > Note: There are only very few locations in the kernel, where > print_hex_dump is not used with KERN_DEBUG and DUMP_PREFIX_ADDRESS. > > Signed-off-by: Roman Fietze Is this patch going to get picked up? I was hoping it would be in 5.9.
Re: [PATCH net] net: qcom/emac: Fix missing clk_disable_unprepare() in error path of emac_probe
On 8/6/20 8:54 PM, wanghai (M) wrote: Thanks for your suggestion. May I fix it like this? Yes, this is what I had in mind. Thanks. Acked-by: Timur Tabi
Re: [PATCH net] net: qcom/emac: Fix missing clk_disable_unprepare() in error path of emac_probe
On 8/6/20 9:06 AM, Wang Hai wrote: In emac_clks_phase1_init() of emac_probe(), there may be a situation in which some clk_prepare_enable() succeed and others fail. If emac_clks_phase1_init() fails, goto err_undo_clocks to clean up the clk that was successfully clk_prepare_enable(). Good catch, however, I think the proper fix is to fix this in emac_clks_phase1_init(), so that if some clocks fail, the other clocks are cleaned up and then an error is returned.
Re: [PATCH] fbdev: fsl-diu: remove redundant null check on cmap
On 12/3/18 12:43 AM, Wen Yang wrote: The null check on &info->cmap is redundant since cmap is a struct inside fb_info and can never be null, so the check is always true. we may remove it. Signed-off-by: Wen Yang Acked-by: Timur Tabi
Enable tracing only for one function and its children?
Is there a way to enable ftrace tracing only for one specific function and all the functions it calls? Then when the function returns, disable tracing until the next time? When I pass the function name only to set_ftrace_filter, it literally only traces that function, which doesn't help me. I tried setting writing "nvidia_ioctl:traceon" to set_ftrace_filter, but that just gave me an "invalid argument" error. And even if that did work, I don't know what function to use for :traceoff.
Re: [PATCH v5 2/3] pinctrl: msm: Use init_valid_mask exported function
On 10/5/18 1:52 AM, Ricardo Ribalda Delgado wrote: The current code produces XPU violation if get_direction is called just after the initialization. Signed-off-by: Ricardo Ribalda Delgado I'm not the maintainer of pinctrl-msm, but this looks okay to me. Acked-by: Timur Tabi
Re: [PATCH v5 3/3] gpiolib: Show correct direction from the beginning
On Fri, Oct 5, 2018 at 11:54 AM Timur Tabi wrote: > If you want, just put a printk(... offset) in msm_gpio_get() and read > from a GPIO that you know you have access to, and make sure the > 'offset' is correct. > > Then try reading from a GPIO that you don't have access to, and make > sure you get a failure before msm_gpio_get() is called. Just FYI, if you use sysfs to write to the GPIO, based on the kernel log you pasted, GPIO 362 in sysfs is actually pin 0 in the TLMM. If you use gpio-test, it uses gpiolib which figures this all out for you. There should still be the gpio-test wiki page I wrote that tells you how to use it.
Re: [PATCH v5 3/3] gpiolib: Show correct direction from the beginning
On 10/05/2018 11:17 AM, Jeffrey Hugo wrote:> > Looks like the driver is initing just fine to me. Is setting up the > jumpers and running gpio-test warranted? Well, that test only makes sure that input/output is actually working on the hardware level, but it also makes sure that the GPIOs are numbered correctly. If you want, just put a printk(... offset) in msm_gpio_get() and read from a GPIO that you know you have access to, and make sure the 'offset' is correct. Then try reading from a GPIO that you don't have access to, and make sure you get a failure before msm_gpio_get() is called.
Re: [PATCH v3 3/3] gpiolib: Show correct direction from the beginning
On 10/2/18 3:27 AM, Ricardo Ribalda Delgado wrote: + /* REVISIT: most hardware initializes GPIOs as inputs (often +* with pullups enabled) so power usage is minimized. Linux +* code should set the gpio direction first thing; but until +* it does, and in case chip->get_direction is not set, we may +* expose the wrong direction in sysfs. +*/ I don't think this comment applies any more. Also, please don't use ti...@codeaurora.org any more. That address is no longer valid. Use ti...@kernel.org instead.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On 10/2/18 2:38 AM, Linus Walleij wrote: But as today the only driver that seems to be using valid_mask is msm, so perhaps a hack is something better and then when we have a second driver that requires it we figure out the real requirements. But it is definately your decision;) Please note that MSM is supposed to be the *first* driver, not the only, driver that needs valid_mask. So let's not make any code changes that limit this feature to the MSM driver. I would just add some exported function to gpiolib to do what you need so you can set up the valid_mask before calling gpiochip_add*. I think that should be okay. Drivers should know pretty early whether they need valid_mask or not.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On 9/29/18 8:21 AM, Timur Tabi wrote: Is it possible to access the hardware? Linaro and some Linux OSVs should still have systems, but I usually just ask Jeff to test code for me. Alternatively, you can just add valid_mask support to your driver, and add a check to your get_direction() function to see if it ever is asked to access an invalid GPIO.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On 9/29/18 1:23 AM, Ricardo Ribalda Delgado wrote: In fact gpiochip_init_valid_mask is called some lines after in the same function. We could reorder the function. Would that work for you? It might. It might break something else, though. The driver breaking is upstream? Yes. Is it possible to access the hardware? Linaro and some Linux OSVs should still have systems, but I usually just ask Jeff to test code for me.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On Fri, Sep 28, 2018 at 2:14 PM Jeffrey Hugo wrote: > Nack. Causes a XPU violation to the GPIO config registers. That doesn't surprise me at all. I believe that the problem is that gpiochip_line_is_valid() is being called before gpiochip_irqchip_init_valid_mask() is called, which means that gpiochip_line_is_valid() always returns true.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On 9/27/18 9:04 AM, Jeffrey Hugo wrote: I guess its lucky I saw this then. Did you not get this email: https://lore.kernel.org/patchwork/patch/989545/#1173771
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
On 9/27/18 1:51 AM, Stephen Boyd wrote: Looks OK to me visually. I haven't tested it because I don't have access to the locked down hardware anymore. Same here. Please wait for Jeff Hugo to test it before applying. Thanks.
Re: [PATCH v2] gpiolib: Show correct direction from the beginning
Jeff, can you test these two patches on Amberwing to make sure that they don't cause an XPU violation on boot? The call to gpiochip_line_is_valid() should return false on any GPIOs that aren't listed in the ACPI table. My concern is that this patch might be calling gpiochip_line_is_valid() too early, before all the arrays have been set up. Thanks. On 9/21/18 5:36 AM, Ricardo Ribalda Delgado wrote: Current code assumes that the direction is input if direction_input function is set. This might not be the case on GPIOs with programmable direction. Signed-off-by: Ricardo Ribalda Delgado --- drivers/gpio/gpiolib.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4b45de883ada..00c17f64d9ff 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1352,7 +1352,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, * it does, and in case chip->get_direction is not set, we may * expose the wrong direction in sysfs. */ - desc->flags = !chip->direction_input ? (1 << FLAG_IS_OUT) : 0; + if (chip->get_direction && gpiochip_line_is_valid(chip, i)) + desc->flags = !chip->get_direction(chip, i) ? + (1 << FLAG_IS_OUT) : 0; + else + desc->flags = !chip->direction_input ? + (1 << FLAG_IS_OUT) : 0; } #ifdef CONFIG_PINCTRL
Re: [PATCH] gpiolib: Show correct direction from the beginning
On 9/20/18 5:36 PM, Linus Walleij wrote: What I mean is that $SUBJECT patch might not hurt Qualcomms GPIOs (not crash the platform) if and only if it is augmented to not try to get the initial direction from lines masked off in .valid_mask if .need_valid_mask is true. Whether it makes sense semantically is a different debate, but it seems possible to reintroduce calling .get_direction() without hurting anyone. That means that all the logic for checking valid_mask needs to be added to the chip driver's .get_direction() function. We can add that logic to msm_gpio_get_direction (at one point, I had a patch that did that, but it was rejected). My concern is: what if a driver depends on a .request call being made (in order to configure muxes, for example) before touching the hardware? I wonder if this is something that really should be handled in the driver's .probe function. The driver should collect that information and pass it to add_data.
Re: [PATCH] gpiolib: Show correct direction from the beginning
On 9/20/18 12:23 AM, Linus Walleij wrote: I think most gpiochips easily survives calling the .get_direction() early, Qualcomm's stand out here. Now that we have .valid_mask in the gpiochip could we simply just add this back, resepecting valid_mask and avoid checking the direction of precisely these GPIOs? Can you be more specific? One of the proposals made previously was to add a check in msm_gpio_get_direction(), but that was rejected because the consensus was the valid_mask checks in gpiolib are sufficient.
Re: [PATCH] gpiolib: Show correct direction from the beginning
On 9/19/18 10:27 AM, Ricardo Ribalda Delgado wrote: "The get_direction callback normally triggers a read/write to hardware, but we shouldn't be touching the hardware for an individual GPIO until after it's been properly claimed." is an statement specific for your platform That is definitely not true. and should be fixed in your driver. There is no bug in my driver. The driver reports only a subset of the GPIOs, because that's all that are available. Attempting to access an invalid GPIO generates an XPU violation. The original code was attempting to access GPIOs that the driver said don't exist. The code that we have today is the result of months of discussion, negotiation, and trial-and-error.
Re: [PATCH] gpiolib: Show correct direction from the beginning
On 09/19/2018 10:27 AM, Ricardo Ribalda Delgado wrote: Let me explain my current setup I have a board with input and output gpios, the direction is defined via pdata. When I run gpioinfo all the gpios are shown as input, regardless if they are input or outputs: Eg: root@qt5022:/tmp# ./gpioinfo gpiochip0 - 16 lines: line 0: "PROG_B" unused input active-high line 1: "M0" unused input active-high line 2: "M1" unused input active-high line 3: "M2" unused input active-high line 4:"DIN" unused input active-high line 5: "CCLK" unused input active-high line 6: unnamed unused input active-high line 7: unnamed unused input active-high line 8: "DONE" unused input active-high line 9: "INIT_B" unused input active-high line 10: unnamed unused input active-high line 11: unnamed unused input active-high line 12: unnamed unused input active-high line 13: unnamed unused input active-high line 14: unnamed unused input active-high line 15: unnamed unused input active-high Yes, this is a known problem that should be fixed. That is wrong and very confusing to the user, it can also lead to a mayor fuckup if the user decides to connect two output gpio pins because he expects that both are input. (This is the programming port, but I also have 24 V -high current GPIOs) Users are expected to program the direction for every GPIO they want to use, regardless of whatever it's set to before they open it. There is a function in the API to tell libgpio if a gpio is out our in. Why not use it? Because calling that API before properly claiming the GPIO is a programming error. - If the configuration is hardcoded, the driver will return a fixed value - If it is cheap to query the hardware, the driver will query the hardware, - If it is expensive to query the hardware the driver can either return a cached value or a fake value (current situation) The reason why the Qualcomm driver is impacted the most is because on ACPI platforms, the GPIO map is "sparse". That is, not every GPIO between 0 and n-1 actually exists. So reading a GPIO that doesn't exist is invalid. The way to protect against that is to claim the GPIO first. If the claim is rejected, then you know that you can't access that GPIO. The bug is that the original code that I deleted (and that you're trying to put back) doesn't claim the GPIO first. From my point of view: "The get_direction callback normally triggers a read/write to hardware, but we shouldn't be touching the hardware for an individual GPIO until after it's been properly claimed." is an statement specific for your platform and should be fixed in your driver. Either that, or I have completely missunderstund the purpouse of gpiod :), and that could easily be the case. It's not a platform-specific statement. It applies to all drivers. In some drivers, the get_direction function had side-effects (like programming muxes, IIRC) that no one really cared about but was technically wrong. I'm not sure how to properly fix this, but I wonder if we need some kind of late-stage initialization where gpiolib scans all the GPIOs by claiming them first, reading the directions, and then releasing them.
Re: [PATCH] gpiolib: Show correct direction from the beginning
On 9/18/18 11:04 PM, Ricardo Ribalda Delgado wrote: And should't that be tacked in qcom hardware with something like: if (!priv->initialized) return INPUT; if you or Timur point me to the harware that was crashing I would not mind looking into that, but the current situations seems to me like a hack. I'd say the previous code was the hack. My comment about not touching the hardware until it is properly claimed is valid, and it applies to all platforms.
[PATCH] MAINTAINERS: Timur has a kernel.org address
Timur Tabi no longer works for Qualcomm, and he now has a kernel.org email address, so update MAINTAINERS accordingly. Signed-off-by: Timur Tabi --- MAINTAINERS | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index e19ec6d90a44..192e05b8a39a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5669,7 +5669,7 @@ F:drivers/crypto/caam/ F: Documentation/devicetree/bindings/crypto/fsl-sec4.txt FREESCALE DIU FRAMEBUFFER DRIVER -M: Timur Tabi +M: Timur Tabi L: linux-fb...@vger.kernel.org S: Maintained F: drivers/video/fbdev/fsl-diu-fb.* @@ -5769,7 +5769,7 @@ S:Maintained F: drivers/net/wan/fsl_ucc_hdlc* FREESCALE QUICC ENGINE UCC UART DRIVER -M: Timur Tabi +M: Timur Tabi L: linuxppc-...@lists.ozlabs.org S: Maintained F: drivers/tty/serial/ucc_uart.c @@ -5793,7 +5793,7 @@ F:drivers/net/ethernet/freescale/fs_enet/ F: include/linux/fs_enet_pd.h FREESCALE SOC SOUND DRIVERS -M: Timur Tabi +M: Timur Tabi M: Nicolin Chen M: Xiubo Li R: Fabio Estevam @@ -11809,9 +11809,9 @@ F: Documentation/devicetree/bindings/opp/kryo-cpufreq.txt F: drivers/cpufreq/qcom-cpufreq-kryo.c QUALCOMM EMAC GIGABIT ETHERNET DRIVER -M: Timur Tabi +M: Timur Tabi L: net...@vger.kernel.org -S: Supported +S: Maintained F: drivers/net/ethernet/qualcomm/emac/ QUALCOMM HEXAGON ARCHITECTURE -- 2.17.1
Re: [PATCH V2] PCI: Enable PASID when End-to-End TLP is supported by all bridges
On 6/19/18 9:14 PM, Sinan Kaya wrote: + if (!(cap & PCI_EXP_DEVCAP2_E2ETLP)) + return; + + dev->eetlp_prefix = 1; How about: if (cap & PCI_EXP_DEVCAP2_E2ETLP) dev->eetlp_prefix = 1; -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 4.16 269/272] pinctrl: msm: Use dynamic GPIO numbering
On 5/31/18 11:57 AM, Bjorn Andersson wrote: (Although that would probably break if Timur's customers move their user space to the new platform as "the first instance" isn't deterministic). Users of platforms that have multiple TLMMs should be required to use gpiolib instead of sysfs. I've already updated all our internal code to use it, and there is no external code that does GPIO on our server SoCs anyway. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 4.16 269/272] pinctrl: msm: Use dynamic GPIO numbering
On 5/31/18 7:12 AM, Greg Kroah-Hartman wrote: Why is it somehow ok for "future" kernels? You can't break the api in the future for no reason. So this needs to be the same everywhere. If it is broken in 4.17-rc, it needs to be reverted. This was discussed here: https://www.spinics.net/lists/linux-arm-msm/msg32572.html The code can't program the value to 0 *and* support multiple TLMM devices at the same time. Prior to 4.18, it is not possible to support multiple TLMMs anyway, so blindly setting the base to -1 in the 4.17 kernel just breaks existing user-space code that uses the legacy GPIO API. (This patch in 4.18 is what provides support for multiple TLMMs: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/pinctrl/qcom?id=f265e8b91bb50c7a732a171ddaeb0eef143bacd9) So you can see in the thread that I proposed a compromise: https://www.spinics.net/lists/linux-arm-msm/msg32596.html but that was rejected -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 4.16 269/272] pinctrl: msm: Use dynamic GPIO numbering
On 5/31/18 6:53 AM, Sebastian Gottschall wrote: i checked initially 4.9 with latest patches and 4.14 and reverted this line to get back to the old behaviour but a which view in the current 4.17 tree shows that the same patch has been included in 4.17. it was introduced in the kernel mainline on 12.feb 2018 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/qcom/pinctrl-msm.c?h=v4.17-rc7&id=a7aa75a2a7dba32594291a71c3704000a2fd7089 I believe that this patch should not be applied to *any* stable kernel. It completely breaks legacy GPIO numbering, and it does so intentionally. That may be okay for future kernels, but IMHO it's wrong for older kernels. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH, net-next] qcom-emag: hide ACPI specific functions
On 5/25/18 7:22 PM, Timur Tabi wrote: -phy->open = emac_sgmii_open; -phy->close = emac_sgmii_close; -phy->link_up = emac_sgmii_link_up; -phy->link_down = emac_sgmii_link_down; I'll take it look at it next week when I'm back in the office. I posted a patch that fixes this problem and also retains device-tree support. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH, net-next] qcom-emag: hide ACPI specific functions
On 5/25/18 4:37 PM, Arnd Bergmann wrote:
+#ifdef CONFIG_ACPI
static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u8 irq_bits)
{
struct emac_sgmii *phy = &adpt->phy;
@@ -288,6 +289,7 @@ static struct sgmii_ops qdf2400_ops = {
.link_change = emac_sgmii_common_link_change,
.reset = emac_sgmii_common_reset,
};
+#endif
This seems wrong. The SGMII interrupt handler should still be viable on
a device-tree system. There is a DT compatibility entry for the qdf2432.
Looks like that most recent patch on net-next broke DT support, when it
removed these lines:
- phy->open = emac_sgmii_open;
- phy->close = emac_sgmii_close;
- phy->link_up = emac_sgmii_link_up;
- phy->link_down = emac_sgmii_link_down;
I'll take it look at it next week when I'm back in the office.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] net: qcom/emac: Allocate buffers from local node
On 5/17/18 3:28 AM, Hemanth Puranik wrote: Currently we use non-NUMA aware allocation for TPD and RRD buffers, this patch modifies to use NUMA friendly allocation. Signed-off-by: Hemanth Puranik Acked-by: Timur Tabi -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [RFC PATCH] efi/fb: Convert PCI bus address to resource if translated by the bridge
On 05/16/2018 01:23 PM, Sinan Kaya wrote: + win_start = window->res->start - window->offset; Can you guarantee that window->res->start is always >= window->offset? + win_size = window->res->end - window->res->start + 1; Use resource_size() instead. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] net: qcom/emac: Encapsulate sgmii ops under one structure
On 5/15/18 8:10 PM, Hemanth Puranik wrote: This patch introduces ops structure for sgmii, This by ensures that we do not need dummy functions in case of emulation platforms. Signed-off-by: Hemanth Puranik Acked-by: Timur Tabi -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v4 0/5] Support qcom pinctrl protected pins
On 03/23/2018 11:34 AM, Stephen Boyd wrote: Stephen Boyd (5): dt-bindings: gpio: Add a gpio-reserved-ranges property gpiolib: Extract mask allocation into subroutine gpiolib: Change bitmap allocation to kmalloc_array gpiolib: Support 'gpio-reserved-ranges' property pinctrl: qcom: Don't allow protected pins to be requested ACPI parts: Tested-by: Timur Tabi I posted a pair of patches that should be applied on top of yours. The first one fixed pinctrl-msm when there is more than one TLMM device. The second adds support for my SOC. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 3/3] pinctrl: qcom: Don't allow protected pins to be requested
On 03/23/2018 11:21 AM, Stephen Boyd wrote:
I'll send the updated patches now.
Thanks.
BTW, I just discovered that the static globals in pinctrl-msm are
breaking the ability to support more than one TLMM:
static struct pinctrl_desc msm_pinctrl_desc = {
static struct irq_chip msm_gpio_irq_chip = {
static struct msm_pinctrl *poweroff_pctrl;
I'm going to have to fix all of these.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 3/3] pinctrl: qcom: Don't allow protected pins to be requested
On 3/23/18 6:34 AM, Andy Shevchenko wrote: No, it seems you missed %p vs. %px discussion. The pointers printed by %p nowadays are hashed values, not the original ones. I totally forgot about that, thanks. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 3/3] pinctrl: qcom: Don't allow protected pins to be requested
On 03/22/2018 07:23 PM, Timur Tabi wrote: Also, you don't allocate chip->valid_mask anywhere. So I see now where it's allocated, but something is fishy. I have three TLMMs on my chip: [ 67.107018] gpiochip_init_valid_mask:351 gpiochip->need_valid_mask=1 [ 67.153747] gpiochip_init_valid_mask:356 gpiochip->ngpio=72 [ 67.195324] gpiochip_init_valid_mask:361 gpiochip->valid_mask=70b1a4b6 [ 68.532992] gpiochip_init_valid_mask:356 gpiochip->ngpio=44 [ 68.574496] gpiochip_init_valid_mask:361 gpiochip->valid_mask=2f33b8a3 [ 68.709378] msm_gpio_init_valid_mask:837 ret=44 max_gpios=44 chip->valid_mask=2f33b8a3 [ 69.726502] gpiochip_init_valid_mask:351 gpiochip->need_valid_mask=1 [ 69.772960] gpiochip_init_valid_mask:356 gpiochip->ngpio=54 [ 69.814084] gpiochip_init_valid_mask:361 gpiochip->valid_mask=1a53c932 Are these normal addresses for kcalloc() to return? They're not even word-aligned. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 3/3] pinctrl: qcom: Don't allow protected pins to be requested
On 03/22/2018 07:16 PM, Timur Tabi wrote: This needs to be ret <= max_gpios, otherwise it will fail if every GPIO is available. And it should print an error message and return an error code if ret > max_gpios. Also, you don't allocate chip->valid_mask anywhere. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 3/3] pinctrl: qcom: Don't allow protected pins to be requested
On 03/21/2018 11:58 AM, Stephen Boyd wrote:
+static int msm_gpio_init_valid_mask(struct gpio_chip *chip,
+ struct msm_pinctrl *pctrl)
+{
+ int ret;
+ unsigned int len, i;
+ unsigned int max_gpios = pctrl->soc->ngpios;
+
+ /* The number of GPIOs in the ACPI tables */
+ ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+ if (ret > 0 && ret < max_gpios) {
This needs to be ret <= max_gpios, otherwise it will fail if every GPIO
is available.
And it should print an error message and return an error code if ret >
max_gpios.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 1/3] dt-bindings: pinctrl: Add a reserved-gpio-ranges property
On 3/19/18 10:36 PM, Linus Walleij wrote: This looks fine except Andy's note to rename this ranges to gpio-reserved-ranges for namespacing. Are you reposting this series as v3 with this fixed or does someone else need to pick it up? I will pick this up if Stephen wants me to. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
On Fri, Mar 16, 2018 at 11:25 PM, Sinan Kaya wrote:
> @@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16
> inc, union t4_wr *wqe)
> (u64 *)wqe);
> } else {
> pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> - wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> + __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
I think Jason might be right that drivers shouldn't be calling the
__raw_write functions. You definitely need to run this by the PPC
developers, specifically Ben Herrenschmidt.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs
On 3/16/18 6:04 PM, Steve Wise wrote: Anybody understand why the PPC implementation of writeX_relaxed() isn't relaxed? You probably should ask that on the linuxppc-...@lists.ozlabs.org mailing list. I've always wondered why PowerPC has non-standard I/O accessors. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs
On 3/13/18 10:20 PM, Sinan Kaya wrote:
+/* Assumes caller has executed a write barrier to order memory and device
+ * requests.
+ */
static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 value)
{
- writel(value, ring->tail);
+ writel_relaxed(value, ring->tail);
}
Why not put the wmb() in this function, or just get rid of the wmb() in
the rest of the file and keep this as writel? That way, you can avoid
the comment and the risk that comes with it.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] net: qcom/emac: Use proper free methods during TX
On 3/5/18 8:48 PM, Hemanth Puranik wrote: This patch fixes the warning messages/call traces seen if DMA debug is enabled, In case of fragmented skb's memory was allocated using dma_map_page but freed using dma_unmap_single. This patch modifies buffer allocations in TX path to use dma_map_page in all the places and dma_unmap_page while freeing the buffers. Signed-off-by: Hemanth Puranik Acked-by: Timur Tabi -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] arm64: KVM: Use SMCCC_ARCH_WORKAROUND_1 for Falkor BP hardening
On Fri, Mar 2, 2018 at 3:50 PM, Shanker Donthineni wrote: > diff --git a/arch/arm64/include/asm/cpucaps.h > b/arch/arm64/include/asm/cpucaps.h > index bb26382..6ecc249 100644 > --- a/arch/arm64/include/asm/cpucaps.h > +++ b/arch/arm64/include/asm/cpucaps.h > @@ -43,7 +43,7 @@ > #define ARM64_SVE 22 > #define ARM64_UNMAP_KERNEL_AT_EL0 23 > #define ARM64_HARDEN_BRANCH_PREDICTOR 24 > -#define ARM64_HARDEN_BP_POST_GUEST_EXIT25 > +/* #define ARM64_UNALLOCATED_ENTRY 25 */ Why not just delete the entry? Is ARM64_NCAPS never supposed to get smaller?
Re: [PATCH v2 0/3] Support qcom pinctrl protected pins
On 02/23/2018 10:54 AM, Bjorn Andersson wrote: I haven't found the time to review the reuse of the irq valid mask or the effort needed to replace this, other than that I think the series looks good. So is that an ACK? -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v2 0/3] Support qcom pinctrl protected pins
On 01/25/2018 07:13 PM, Stephen Boyd wrote: This patchset proposes a solution to describing the valid pins for a pin controller in a semi-generic way so that qcom platforms can expose the pins that are really available. Typically, this has been done by having drivers and firmware descriptions only use pins they know they have access to, and that still works now because we no longer read the pin direction at boot. But there are still some userspace drivers and debugfs facilities that don't know what pins are available and attempt to read everything they can. On qcom platforms, this may lead to a system hang, which isn't very nice behavior, even if root is the only user that can trigger it. Any progress on this patch set? Stephen no longer works for Qualcomm, so I don't know what the next step is, and I really want this feature in 4.17 (we've missed so many merge windows already). -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] pinctrl: msm: Use dynamic GPIO numbering
On 02/07/2018 07:52 AM, Linus Walleij wrote: OK I put this in devel for v4.17, let's see if something explodes in linux-next else we can go with this. Otherwise we need something that conserves base 0 for singular TLMM drivers and make it -1 for newer platforms with several instances. I don't see this patch in linusw/linux-gpio.git anywhere. Do you want me to submit another patch that implements this: static int base = 0; chip->base = base; base = -1; This preserves existing behavior for current drivers. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
