Re: [PATCH v12 22/22] selftests/vm: Fix deadlock in protection_keys.c

2018-03-16 Thread Dave Hansen 
On 02/21/2018 05:55 PM, Ram Pai wrote:
> From: Thiago Jung Bauermann 
> 
> The sig_chld() handler calls dprintf2() taking care of setting
> dprint_in_signal so that sigsafe_printf() won't call printf().
> Unfortunately, this precaution is is negated by dprintf_level(), which
> has a call to fflush().
> 
> This function acquires a lock, which means that if the signal interrupts an
> ongoing fflush() the process will deadlock. At least on powerpc this is
> easy to trigger, resulting in the following backtrace when attaching to the
> frozen process:

Ugh, yeah, I've run into this too.

Acked-by: Dave Hansen 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v12 22/22] selftests/vm: Fix deadlock in protection_keys.c

2018-02-25 Thread Balbir Singh 
On Wed, 21 Feb 2018 17:55:41 -0800
Ram Pai  wrote:

> From: Thiago Jung Bauermann 
> 
> The sig_chld() handler calls dprintf2() taking care of setting
> dprint_in_signal so that sigsafe_printf() won't call printf().
> Unfortunately, this precaution is is negated by dprintf_level(), which
> has a call to fflush().
>

fflush() is not the signal-safe function list, so this makes sense.
I wonder if fflush() is needed in sigsafe_printf()?

How about?

diff --git a/tools/testing/selftests/x86/pkey-helpers.h 
b/tools/testing/selftests/x86/pkey-helpers.h
index b3cb7670e026..2c3b39851f10 100644
--- a/tools/testing/selftests/x86/pkey-helpers.h
+++ b/tools/testing/selftests/x86/pkey-helpers.h
@@ -29,6 +29,7 @@ static inline void sigsafe_printf(const char *format, ...)
va_start(ap, format);
if (!dprint_in_signal) {
vprintf(format, ap);
+   fflush(NULL);   \
} else {
int ret;
int len = vsnprintf(dprint_in_signal_buffer,
@@ -49,7 +50,6 @@ static inline void sigsafe_printf(const char *format, ...)
 #define dprintf_level(level, args...) do { \
if (level <= DEBUG_LEVEL)   \
sigsafe_printf(args);   \
-   fflush(NULL);   \
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
 #define dprintf1(args...) dprintf_level(1, args)


But both are equivalent I guess, so
Acked-by: Balbir Singh 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v12 22/22] selftests/vm: Fix deadlock in protection_keys.c

2018-02-21 Thread Ram Pai 
From: Thiago Jung Bauermann 

The sig_chld() handler calls dprintf2() taking care of setting
dprint_in_signal so that sigsafe_printf() won't call printf().
Unfortunately, this precaution is is negated by dprintf_level(), which
has a call to fflush().

This function acquires a lock, which means that if the signal interrupts an
ongoing fflush() the process will deadlock. At least on powerpc this is
easy to trigger, resulting in the following backtrace when attaching to the
frozen process:

  (gdb) bt
  #0  0x7fff9f96c7d8 in __lll_lock_wait_private () from 
/lib64/power8/libc.so.6
  #1  0x7fff9f8cba4c in _IO_flush_all_lockp () from /lib64/power8/libc.so.6
  #2  0x7fff9f8cbd1c in __GI__IO_flush_all () from /lib64/power8/libc.so.6
  #3  0x7fff9f8b7424 in fflush () from /lib64/power8/libc.so.6
  #4  0x100504f8 in sig_chld (x=17) at protection_keys.c:283
  #5  
  #6  0x7fff9f8cb8ac in _IO_flush_all_lockp () from /lib64/power8/libc.so.6
  #7  0x7fff9f8cbd1c in __GI__IO_flush_all () from /lib64/power8/libc.so.6
  #8  0x7fff9f8b7424 in fflush () from /lib64/power8/libc.so.6
  #9  0x10050b50 in pkey_get (pkey=7, flags=0) at protection_keys.c:379
  #10 0x10050dc0 in pkey_disable_set (pkey=7, flags=2) at 
protection_keys.c:423
  #11 0x10051414 in pkey_write_deny (pkey=7) at protection_keys.c:486
  #12 0x100556bc in test_ptrace_of_child (ptr=0x7fff9f7f, pkey=7) 
at protection_keys.c:1288
  #13 0x10055f60 in run_tests_once () at protection_keys.c:1414
  #14 0x100561a4 in main () at protection_keys.c:1459

The fix is to refrain from calling fflush() when inside a signal handler.
The output may not be as pretty but at least the testcase will be able to
move on.

cc: Dave Hansen 
cc: Florian Weimer 
Signed-off-by: Ram Pai 
Signed-off-by: Thiago Jung Bauermann 

 tools/testing/selftests/vm/pkey-helpers.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
---
 tools/testing/selftests/vm/pkey-helpers.h |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/testing/selftests/vm/pkey-helpers.h 
b/tools/testing/selftests/vm/pkey-helpers.h
index 67f9b0f..7240598 100644
--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -128,7 +128,8 @@ static inline void sigsafe_printf(const char *format, ...)
 #define dprintf_level(level, args...) do { \
if (level <= DEBUG_LEVEL)   \
sigsafe_printf(args);   \
-   fflush(NULL);   \
+   if (!dprint_in_signal)  \
+   fflush(NULL);   \
 } while (0)
 #define dprintf0(args...) dprintf_level(0, args)
 #define dprintf1(args...) dprintf_level(1, args)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html