| Message ID | f7twpsvgyar.fsf@aconole.bos.csb |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2015-12-03 at 13:52 -0500, Aaron Conole wrote: > Dmitry Vyukov <dvyukov@google.com> writes: > > On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet <edumazet@google.com> wrote: > > > On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet wrote: > > > > > > > > > > > > No, I don't. But pr_debug always computes its arguments. See no_printk > > > > > > in printk.h. So this use-after-free happens for all users. > > > > > > > > > > Hmm. > > > > > > > > > > pr_debug() should be a nop unless either DEBUG or > > > > > CONFIG_DYNAMIC_DEBUG are set > > > > > > > > > > On our production kernels, pr_debug() is a nop. > > > > > > > > > > Can you double check ? Thanks ! > > > > > > > > > > > > Why should it be nop? no_printk thing in printk.h pretty much > > > > explicitly makes it not a nop... > > Because it was until commit 5264f2f75d8. It also violates my reading of > the following from printk.h: > > * All of these will print unconditionally, although note that pr_debug() > * and other debug macros are compiled out unless either DEBUG is defined > * or CONFIG_DYNAMIC_DEBUG is set. > > > > > > > > > Double-checked: debug_post_sfx leads to some generated code: > > > > > > > > debug_post_sfx(); > > > > ffffffff8229f256: 48 8b 85 58 fe ff ff mov -0x1a8(%rbp),%rax > > > > ffffffff8229f25d: 48 85 c0 test %rax,%rax > > > > ffffffff8229f260: 74 24 je > > > > ffffffff8229f286 > > > > ffffffff8229f262: 8b b0 a8 00 00 00 mov 0xa8(%rax),%esi > > > > ffffffff8229f268: 48 8b 85 60 fe ff ff mov -0x1a0(%rbp),%rax > > > > ffffffff8229f26f: 44 89 85 74 fe ff ff mov %r8d,-0x18c(%rbp) > > > > ffffffff8229f276: 48 8b 78 20 mov 0x20(%rax),%rdi > > > > ffffffff8229f27a: e8 71 28 01 00 callq > > > > ffffffff822b1af0 > > > > ffffffff8229f27f: 44 8b 85 74 fe ff ff mov -0x18c(%rbp),%r8d > > > > > > > > return error; > > > > } > > > > ffffffff8229f286: 48 81 c4 a0 01 00 00 add $0x1a0,%rsp > > > > ffffffff8229f28d: 44 89 c0 mov %r8d,%eax > > > > ffffffff8229f290: 5b pop %rbx > > > > ffffffff8229f291: 41 5c pop %r12 > > > > ffffffff8229f293: 41 5d pop %r13 > > > > ffffffff8229f295: 41 5e pop %r14 > > > > ffffffff8229f297: 41 5f pop %r15 > > > > ffffffff8229f299: 5d pop %rbp > > > > ffffffff8229f29a: c3 retq > > > > > > This is a serious concern, because we let in the past lot of patches > > > converting traditional > > +1 > > > > #ifdef DEBUG > > > # define some_hand_coded_ugly_debug() printk( ...._ > > > #else > > > # define some_hand_coded_ugly_debug() > > > #endif > > > > > > On the premise pr_debug() would be a nop. > > > > > > It seems it is not always the case. This is a very serious problem. > > +1 > > > > We probably have hundred of potential bugs, because few people > > > actually make sure all debugging stuff is correct, > > > like comments can be wrong because they are not updated properly as > > > time flies. > > > > > > It is definitely a nop for many cases. > > > > > > +void eric_test_pr_debug(struct sock *sk) > > > +{ > > > + if (atomic_read(&sk->sk_omem_alloc)) > > > + pr_debug("%s: optmem leakage for sock %p\n", > > > + __func__, sk); > > > +} > > > > > > -> > > > > > > 0000000000004740 : > > > 4740: e8 00 00 00 00 callq 4745 > > > 4741: R_X86_64_PC32 __fentry__-0x4 > > > 4745: 55 push %rbp > > > 4746: 8b 87 24 01 00 00 mov 0x124(%rdi),%eax // > > > atomic_read() but nothing follows > > > 474c: 48 89 e5 mov %rsp,%rbp > > > 474f: 5d pop %rbp > > > 4750: c3 retq > > > > > > > > I would expect that it is nop when argument evaluation does not have > > side-effects. For example, for a load of a variable compiler will most > > likely elide it (though, it does not have to elide it, because the > > load is spelled in the code, so it can also legally emit the load and > > doesn't use the result). > > But if argument computation has side-effect (or compiler can't prove > > otherwise), it must emit code. It must emit code for function calls > > when the function is defined in a different translation unit, and for > > volatile accesses (most likely including atomic accesses), etc > > This isn't 100% true. As you state, in order to reach the return 0, all > side effects must be evaluated. Load generally does not have side > effects, so it can be safely elided, but function() must be emitted. > > However, that is _not_ required to get the desired warning emission on a > printf argument function, see http://pastebin.com/UHuaydkj for an > example. > > I think that as a minimum, the following patch should be evaluted, but am > unsure to whom I should submit it (after I test): Andrew Morton <akpm@linux-foundation.org> (cc'd) > diff --git a/include/linux/printk.h b/include/linux/printk.h > index 9729565..cd24d2d 100644 > --- a/include/linux/printk.h > +++ b/include/linux/printk.h > @@ -286,7 +286,7 @@ extern asmlinkage void dump_stack(void) __cold; > printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > #else > #define pr_debug(fmt, ...) \ > - no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > + ({ if(0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0;}) More common is to use do {} while (0) instead of a statement expression. I think it'd be good to change pr_debug and variants to do { if (0) no_printk(...) } while (0) or some other form that completely eliminates all the side-effects/function evaluations. I think the same should be true when CONFIG_PRINTK is not enabled. https://lkml.org/lkml/2014/12/3/696 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2015 01:52 PM, Aaron Conole wrote: > Dmitry Vyukov <dvyukov@google.com> writes: >> On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet <edumazet@google.com> wrote: >>> On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >>>> On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet <edumazet@google.com> wrote: >>>>>> >>>>>> No, I don't. But pr_debug always computes its arguments. See no_printk >>>>>> in printk.h. So this use-after-free happens for all users. >>>>> >>>>> Hmm. >>>>> >>>>> pr_debug() should be a nop unless either DEBUG or >>>>> CONFIG_DYNAMIC_DEBUG are set >>>>> >>>>> On our production kernels, pr_debug() is a nop. >>>>> >>>>> Can you double check ? Thanks ! >>>> >>>> >>>> Why should it be nop? no_printk thing in printk.h pretty much >>>> explicitly makes it not a nop... > > Because it was until commit 5264f2f75d8. It also violates my reading of > the following from printk.h: > > * All of these will print unconditionally, although note that pr_debug() > * and other debug macros are compiled out unless either DEBUG is defined > * or CONFIG_DYNAMIC_DEBUG is set. > >>>> >>>> Double-checked: debug_post_sfx leads to some generated code: >>>> >>>> debug_post_sfx(); >>>> ffffffff8229f256: 48 8b 85 58 fe ff ff mov -0x1a8(%rbp),%rax >>>> ffffffff8229f25d: 48 85 c0 test %rax,%rax >>>> ffffffff8229f260: 74 24 je >>>> ffffffff8229f286 <sctp_do_sm+0x176> >>>> ffffffff8229f262: 8b b0 a8 00 00 00 mov 0xa8(%rax),%esi >>>> ffffffff8229f268: 48 8b 85 60 fe ff ff mov -0x1a0(%rbp),%rax >>>> ffffffff8229f26f: 44 89 85 74 fe ff ff mov %r8d,-0x18c(%rbp) >>>> ffffffff8229f276: 48 8b 78 20 mov 0x20(%rax),%rdi >>>> ffffffff8229f27a: e8 71 28 01 00 callq >>>> ffffffff822b1af0 <sctp_id2assoc> >>>> ffffffff8229f27f: 44 8b 85 74 fe ff ff mov -0x18c(%rbp),%r8d >>>> >>>> return error; >>>> } >>>> ffffffff8229f286: 48 81 c4 a0 01 00 00 add $0x1a0,%rsp >>>> ffffffff8229f28d: 44 89 c0 mov %r8d,%eax >>>> ffffffff8229f290: 5b pop %rbx >>>> ffffffff8229f291: 41 5c pop %r12 >>>> ffffffff8229f293: 41 5d pop %r13 >>>> ffffffff8229f295: 41 5e pop %r14 >>>> ffffffff8229f297: 41 5f pop %r15 >>>> ffffffff8229f299: 5d pop %rbp >>>> ffffffff8229f29a: c3 retq >>> >>> This is a serious concern, because we let in the past lot of patches >>> converting traditional > > +1 > >>> #ifdef DEBUG >>> # define some_hand_coded_ugly_debug() printk( ...._ >>> #else >>> # define some_hand_coded_ugly_debug() >>> #endif >>> >>> On the premise pr_debug() would be a nop. >>> >>> It seems it is not always the case. This is a very serious problem. > > +1 > >>> We probably have hundred of potential bugs, because few people >>> actually make sure all debugging stuff is correct, >>> like comments can be wrong because they are not updated properly as >>> time flies. >>> >>> It is definitely a nop for many cases. >>> >>> +void eric_test_pr_debug(struct sock *sk) >>> +{ >>> + if (atomic_read(&sk->sk_omem_alloc)) >>> + pr_debug("%s: optmem leakage for sock %p\n", >>> + __func__, sk); >>> +} >>> >>> -> >>> >>> 0000000000004740 <eric_test_pr_debug>: >>> 4740: e8 00 00 00 00 callq 4745 <eric_test_pr_debug+0x5> >>> 4741: R_X86_64_PC32 __fentry__-0x4 >>> 4745: 55 push %rbp >>> 4746: 8b 87 24 01 00 00 mov 0x124(%rdi),%eax // >>> atomic_read() but nothing follows >>> 474c: 48 89 e5 mov %rsp,%rbp >>> 474f: 5d pop %rbp >>> 4750: c3 retq >> >> >> >> I would expect that it is nop when argument evaluation does not have >> side-effects. For example, for a load of a variable compiler will most >> likely elide it (though, it does not have to elide it, because the >> load is spelled in the code, so it can also legally emit the load and >> doesn't use the result). >> But if argument computation has side-effect (or compiler can't prove >> otherwise), it must emit code. It must emit code for function calls >> when the function is defined in a different translation unit, and for >> volatile accesses (most likely including atomic accesses), etc > > This isn't 100% true. As you state, in order to reach the return 0, all > side effects must be evaluated. Load generally does not have side > effects, so it can be safely elided, but function() must be emitted. > > However, that is _not_ required to get the desired warning emission on a > printf argument function, see http://pastebin.com/UHuaydkj for an > example. > > I think that as a minimum, the following patch should be evaluted, but am > unsure to whom I should submit it (after I test): Agreed - the intention here is certainly to have no side effects. It looks like 'no_printk()' is used in quite a few other places that would benefit from this change. So we probably want a generic 'really_no_printk()' macro. Thanks, -Jason > > diff --git a/include/linux/printk.h b/include/linux/printk.h > index 9729565..cd24d2d 100644 > --- a/include/linux/printk.h > +++ b/include/linux/printk.h > @@ -286,7 +286,7 @@ extern asmlinkage void dump_stack(void) __cold; > printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > #else > #define pr_debug(fmt, ...) \ > - no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) > + ({ if(0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0;}) > #endif > > /* > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > I think that as a minimum, the following patch should be evaluted, > > but am unsure to whom I should submit it (after I test): [] > Agreed - the intention here is certainly to have no side effects. It > looks like 'no_printk()' is used in quite a few other places that would > benefit from this change. So we probably want a generic > 'really_no_printk()' macro. https://lkml.org/lkml/2012/6/17/231 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2015 03:03 PM, Joe Perches wrote: > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >> On 12/03/2015 01:52 PM, Aaron Conole wrote: >>> I think that as a minimum, the following patch should be evaluted, >>> but am unsure to whom I should submit it (after I test): > [] >> Agreed - the intention here is certainly to have no side effects. It >> looks like 'no_printk()' is used in quite a few other places that would >> benefit from this change. So we probably want a generic >> 'really_no_printk()' macro. > > https://lkml.org/lkml/2012/6/17/231 > I don't see this in the tree. Also maybe we should just convert no_printk() to do what your 'eliminated_printk()'. So we can convert all users with this change? Thanks, -Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: > On 12/03/2015 03:03 PM, Joe Perches wrote: > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > > > I think that as a minimum, the following patch should be evaluted, > > > > but am unsure to whom I should submit it (after I test): > > [] > > > Agreed - the intention here is certainly to have no side effects. It > > > looks like 'no_printk()' is used in quite a few other places that would > > > benefit from this change. So we probably want a generic > > > 'really_no_printk()' macro. > > > > https://lkml.org/lkml/2012/6/17/231 > > I don't see this in the tree. It never got applied. > Also maybe we should just convert > no_printk() to do what your 'eliminated_printk()'. Some of them at least. > So we can convert all users with this change? I don't think so, I think there are some function evaluation/side effects that are required. I believe some do hardware I/O. It'd be good to at least isolate them. I'm not sure how to find them via some automated tool/mechanism though. I asked Julia Lawall about it once in this thread: https://lkml.org/lkml/2014/12/3/696 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/03/2015 03:24 PM, Joe Perches wrote: > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >> On 12/03/2015 03:03 PM, Joe Perches wrote: >>> On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >>>> On 12/03/2015 01:52 PM, Aaron Conole wrote: >>>>> I think that as a minimum, the following patch should be evaluted, >>>>> but am unsure to whom I should submit it (after I test): >>> [] >>>> Agreed - the intention here is certainly to have no side effects. It >>>> looks like 'no_printk()' is used in quite a few other places that would >>>> benefit from this change. So we probably want a generic >>>> 'really_no_printk()' macro. >>> >>> https://lkml.org/lkml/2012/6/17/231 >> >> I don't see this in the tree. > > It never got applied. > >> Also maybe we should just convert >> no_printk() to do what your 'eliminated_printk()'. > > Some of them at least. > >> So we can convert all users with this change? > > I don't think so, I think there are some > function evaluation/side effects that are > required. I believe some do hardware I/O. > > It'd be good to at least isolate them. > > I'm not sure how to find them via some > automated tool/mechanism though. > > I asked Julia Lawall about it once in this > thread: https://lkml.org/lkml/2014/12/3/696 > Seems rather fragile to have side effects that we rely upon hidden in a printk(). Just convert them and see what breaks :) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(adding lkml as this is likely better discussed there) On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: > On 12/03/2015 03:24 PM, Joe Perches wrote: > > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: > > > On 12/03/2015 03:03 PM, Joe Perches wrote: > > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: > > > > > > I think that as a minimum, the following patch should be evaluted, > > > > > > but am unsure to whom I should submit it (after I test): > > > > [] > > > > > Agreed - the intention here is certainly to have no side effects. It > > > > > looks like 'no_printk()' is used in quite a few other places that would > > > > > benefit from this change. So we probably want a generic > > > > > 'really_no_printk()' macro. > > > > > > > > https://lkml.org/lkml/2012/6/17/231 > > > > > > I don't see this in the tree. > > > > It never got applied. > > > > > Also maybe we should just convert > > > no_printk() to do what your 'eliminated_printk()'. > > > > Some of them at least. > > > > > So we can convert all users with this change? > > > > I don't think so, I think there are some > > function evaluation/side effects that are > > required. I believe some do hardware I/O. > > > > It'd be good to at least isolate them. > > > > I'm not sure how to find them via some > > automated tool/mechanism though. > > > > I asked Julia Lawall about it once in this > > thread: https://lkml.org/lkml/2014/12/3/696 > > > > Seems rather fragile to have side effects that we rely > upon hidden in a printk(). Yup. > Just convert them and see what breaks :) I appreciate your optimism. It's very 1995. Try it and see what happens. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: > (adding lkml as this is likely better discussed there) > > On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: >> On 12/03/2015 03:24 PM, Joe Perches wrote: >> > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >> > > On 12/03/2015 03:03 PM, Joe Perches wrote: >> > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >> > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: >> > > > > > I think that as a minimum, the following patch should be evaluted, >> > > > > > but am unsure to whom I should submit it (after I test): >> > > > [] >> > > > > Agreed - the intention here is certainly to have no side effects. It >> > > > > looks like 'no_printk()' is used in quite a few other places that would >> > > > > benefit from this change. So we probably want a generic >> > > > > 'really_no_printk()' macro. >> > > > >> > > > https://lkml.org/lkml/2012/6/17/231 >> > > >> > > I don't see this in the tree. >> > >> > It never got applied. >> > >> > > Also maybe we should just convert >> > > no_printk() to do what your 'eliminated_printk()'. >> > >> > Some of them at least. >> > >> > > So we can convert all users with this change? >> > >> > I don't think so, I think there are some >> > function evaluation/side effects that are >> > required. I believe some do hardware I/O. >> > >> > It'd be good to at least isolate them. >> > >> > I'm not sure how to find them via some >> > automated tool/mechanism though. >> > >> > I asked Julia Lawall about it once in this >> > thread: https://lkml.org/lkml/2014/12/3/696 >> > >> >> Seems rather fragile to have side effects that we rely >> upon hidden in a printk(). > > Yup. > >> Just convert them and see what breaks :) > > I appreciate your optimism. It's very 1995. > Try it and see what happens. Whatever is the resolution for pr_debug, we still need to fix this particular use-after-free. It affects stability of debug builds, gives invalid debug output, prevents us from finding more bugs in SCTP. And maybe somebody uses CONFIG_DYNAMIC_DEBUG in production. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 04, 2015 at 11:40:02AM +0100, Dmitry Vyukov wrote: > On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: > > (adding lkml as this is likely better discussed there) > > > > On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: > >> On 12/03/2015 03:24 PM, Joe Perches wrote: > >> > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: > >> > > On 12/03/2015 03:03 PM, Joe Perches wrote: > >> > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: > >> > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: > >> > > > > > I think that as a minimum, the following patch should be evaluted, > >> > > > > > but am unsure to whom I should submit it (after I test): > >> > > > [] > >> > > > > Agreed - the intention here is certainly to have no side effects. It > >> > > > > looks like 'no_printk()' is used in quite a few other places that would > >> > > > > benefit from this change. So we probably want a generic > >> > > > > 'really_no_printk()' macro. > >> > > > > >> > > > https://lkml.org/lkml/2012/6/17/231 > >> > > > >> > > I don't see this in the tree. > >> > > >> > It never got applied. > >> > > >> > > Also maybe we should just convert > >> > > no_printk() to do what your 'eliminated_printk()'. > >> > > >> > Some of them at least. > >> > > >> > > So we can convert all users with this change? > >> > > >> > I don't think so, I think there are some > >> > function evaluation/side effects that are > >> > required. I believe some do hardware I/O. > >> > > >> > It'd be good to at least isolate them. > >> > > >> > I'm not sure how to find them via some > >> > automated tool/mechanism though. > >> > > >> > I asked Julia Lawall about it once in this > >> > thread: https://lkml.org/lkml/2014/12/3/696 > >> > > >> > >> Seems rather fragile to have side effects that we rely > >> upon hidden in a printk(). > > > > Yup. > > > >> Just convert them and see what breaks :) > > > > I appreciate your optimism. It's very 1995. > > Try it and see what happens. > > > Whatever is the resolution for pr_debug, we still need to fix this > particular use-after-free. It affects stability of debug builds, gives > invalid debug output, prevents us from finding more bugs in SCTP. And > maybe somebody uses CONFIG_DYNAMIC_DEBUG in production. Agreed. I'm already working on a fix for this particular use-after-free. Another interesting thing about this is that sctp_do_sm() is called for nearly every movement that happens on a sctp socket. Said that, that always-running IDR search hidden on that debug statement do have some nasty performance impact, specially because it's serialized on a spinlock. This wouldn't be happening if it was fully ellided and would be ok if that pr_debug() was really being printed, but not as it is. Kudos to this report that I could notice this. I'm trying to fix this on SCTP-side as well. Marcelo -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2015 07:55 AM, Marcelo Ricardo Leitner wrote: > On Fri, Dec 04, 2015 at 11:40:02AM +0100, Dmitry Vyukov wrote: >> On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: >>> (adding lkml as this is likely better discussed there) >>> >>> On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: >>>> On 12/03/2015 03:24 PM, Joe Perches wrote: >>>>> On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >>>>>> On 12/03/2015 03:03 PM, Joe Perches wrote: >>>>>>> On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >>>>>>>> On 12/03/2015 01:52 PM, Aaron Conole wrote: >>>>>>>>> I think that as a minimum, the following patch should be evaluted, >>>>>>>>> but am unsure to whom I should submit it (after I test): >>>>>>> [] >>>>>>>> Agreed - the intention here is certainly to have no side effects. It >>>>>>>> looks like 'no_printk()' is used in quite a few other places that would >>>>>>>> benefit from this change. So we probably want a generic >>>>>>>> 'really_no_printk()' macro. >>>>>>> >>>>>>> https://lkml.org/lkml/2012/6/17/231 >>>>>> >>>>>> I don't see this in the tree. >>>>> >>>>> It never got applied. >>>>> >>>>>> Also maybe we should just convert >>>>>> no_printk() to do what your 'eliminated_printk()'. >>>>> >>>>> Some of them at least. >>>>> >>>>>> So we can convert all users with this change? >>>>> >>>>> I don't think so, I think there are some >>>>> function evaluation/side effects that are >>>>> required. I believe some do hardware I/O. >>>>> >>>>> It'd be good to at least isolate them. >>>>> >>>>> I'm not sure how to find them via some >>>>> automated tool/mechanism though. >>>>> >>>>> I asked Julia Lawall about it once in this >>>>> thread: https://lkml.org/lkml/2014/12/3/696 >>>>> >>>> >>>> Seems rather fragile to have side effects that we rely >>>> upon hidden in a printk(). >>> >>> Yup. >>> >>>> Just convert them and see what breaks :) >>> >>> I appreciate your optimism. It's very 1995. >>> Try it and see what happens. >> >> >> Whatever is the resolution for pr_debug, we still need to fix this >> particular use-after-free. It affects stability of debug builds, gives >> invalid debug output, prevents us from finding more bugs in SCTP. And >> maybe somebody uses CONFIG_DYNAMIC_DEBUG in production. > > Agreed. I'm already working on a fix for this particular use-after-free. > > Another interesting thing about this is that sctp_do_sm() is called for > nearly every movement that happens on a sctp socket. Said that, that > always-running IDR search hidden on that debug statement do have some > nasty performance impact, specially because it's serialized on a > spinlock. YUCK! I didn't really pay much attention to those debug macros before, but debug_post_sfx() is truly awful. This wasn't such a bad thing where these macros depended on CONFIG_SCTP_DEBUG, but now that they are always built, we need fix them. -vlad > This wouldn't be happening if it was fully ellided and would > be ok if that pr_debug() was really being printed, but not as it is. > Kudos to this report that I could notice this. I'm trying to fix this on > SCTP-side as well. > > Marcelo > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Vlad Yasevich <vyasevich@gmail.com> writes: > On 12/04/2015 07:55 AM, Marcelo Ricardo Leitner wrote: >> On Fri, Dec 04, 2015 at 11:40:02AM +0100, Dmitry Vyukov wrote: >>> On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: >>>> (adding lkml as this is likely better discussed there) >>>> >>>> On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: >>>>> On 12/03/2015 03:24 PM, Joe Perches wrote: >>>>>> On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >>>>>>> On 12/03/2015 03:03 PM, Joe Perches wrote: >>>>>>>> On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >>>>>>>>> On 12/03/2015 01:52 PM, Aaron Conole wrote: >>>>>>>>>> I think that as a minimum, the following patch should be evaluted, >>>>>>>>>> but am unsure to whom I should submit it (after I test): >>>>>>>> [] >>>>>>>>> Agreed - the intention here is certainly to have no side effects. It >>>>>>>>> looks like 'no_printk()' is used in quite a few other places that would >>>>>>>>> benefit from this change. So we probably want a generic >>>>>>>>> 'really_no_printk()' macro. >>>>>>>> >>>>>>>> https://lkml.org/lkml/2012/6/17/231 >>>>>>> >>>>>>> I don't see this in the tree. >>>>>> >>>>>> It never got applied. >>>>>> >>>>>>> Also maybe we should just convert >>>>>>> no_printk() to do what your 'eliminated_printk()'. >>>>>> >>>>>> Some of them at least. >>>>>> >>>>>>> So we can convert all users with this change? >>>>>> >>>>>> I don't think so, I think there are some >>>>>> function evaluation/side effects that are >>>>>> required. I believe some do hardware I/O. >>>>>> >>>>>> It'd be good to at least isolate them. >>>>>> >>>>>> I'm not sure how to find them via some >>>>>> automated tool/mechanism though. >>>>>> >>>>>> I asked Julia Lawall about it once in this >>>>>> thread: https://lkml.org/lkml/2014/12/3/696 >>>>>> >>>>> >>>>> Seems rather fragile to have side effects that we rely >>>>> upon hidden in a printk(). >>>> >>>> Yup. >>>> >>>>> Just convert them and see what breaks :) >>>> >>>> I appreciate your optimism. It's very 1995. >>>> Try it and see what happens. >>> >>> >>> Whatever is the resolution for pr_debug, we still need to fix this >>> particular use-after-free. It affects stability of debug builds, gives >>> invalid debug output, prevents us from finding more bugs in SCTP. And >>> maybe somebody uses CONFIG_DYNAMIC_DEBUG in production. >> >> Agreed. I'm already working on a fix for this particular use-after-free. >> >> Another interesting thing about this is that sctp_do_sm() is called for >> nearly every movement that happens on a sctp socket. Said that, that >> always-running IDR search hidden on that debug statement do have some >> nasty performance impact, specially because it's serialized on a >> spinlock. > > YUCK! I didn't really pay much attention to those debug macros before, but > debug_post_sfx() is truly awful. > > This wasn't such a bad thing where these macros depended on CONFIG_SCTP_DEBUG, > but now that they are always built, we need fix them. I've proposed a patch to linux-kernel to fix them, but I don't think it's really as bad as folks imagine. Ubuntu, RHEL, and Fedora all use DYNAMIC_DEBUG configuration option, which means that the code is getting emitted anyway (correctly, I'll add) and is shunted out by a dynamic debug flag. So for the average user, it's not even really a blip. That does mean there's a cool side-effect of the entire print-macro setup which implies we execute less code when running with DYNAMIC_DEBUG=y in the "normal" case. "Turn on the dynamic debugging config and watch everything get better" isn't the worst mantra, is it? :) > -vlad > > > >> This wouldn't be happening if it was fully ellided and would >> be ok if that pr_debug() was really being printed, but not as it is. >> Kudos to this report that I could notice this. I'm trying to fix this on >> SCTP-side as well. >> >> Marcelo >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: > (adding lkml as this is likely better discussed there) > > On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: >> On 12/03/2015 03:24 PM, Joe Perches wrote: >> > On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >> > > On 12/03/2015 03:03 PM, Joe Perches wrote: >> > > > On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >> > > > > On 12/03/2015 01:52 PM, Aaron Conole wrote: >> > > > > > I think that as a minimum, the following patch should be evaluted, >> > > > > > but am unsure to whom I should submit it (after I test): >> > > > [] >> > > > > Agreed - the intention here is certainly to have no side effects. It >> > > > > looks like 'no_printk()' is used in quite a few other places that would >> > > > > benefit from this change. So we probably want a generic >> > > > > 'really_no_printk()' macro. >> > > > >> > > > https://lkml.org/lkml/2012/6/17/231 >> > > >> > > I don't see this in the tree. >> > >> > It never got applied. >> > >> > > Also maybe we should just convert >> > > no_printk() to do what your 'eliminated_printk()'. >> > >> > Some of them at least. >> > >> > > So we can convert all users with this change? >> > >> > I don't think so, I think there are some >> > function evaluation/side effects that are >> > required. I believe some do hardware I/O. >> > >> > It'd be good to at least isolate them. >> > >> > I'm not sure how to find them via some >> > automated tool/mechanism though. >> > >> > I asked Julia Lawall about it once in this >> > thread: https://lkml.org/lkml/2014/12/3/696 >> > >> >> Seems rather fragile to have side effects that we rely >> upon hidden in a printk(). > > Yup. > >> Just convert them and see what breaks :) > > I appreciate your optimism. It's very 1995. > Try it and see what happens. But Aaron says that DYNAMIC_DEBUG is enabled in most major distributions, and all these side-effects don't happen with DYNAMIC_DEBUG. This suggests that we can make these side-effects not happen without DYNAMIC_DEBUG as well. Or I am missing something here? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2015 11:12 AM, Dmitry Vyukov wrote: > On Thu, Dec 3, 2015 at 9:51 PM, Joe Perches <joe@perches.com> wrote: >> (adding lkml as this is likely better discussed there) >> >> On Thu, 2015-12-03 at 15:42 -0500, Jason Baron wrote: >>> On 12/03/2015 03:24 PM, Joe Perches wrote: >>>> On Thu, 2015-12-03 at 15:10 -0500, Jason Baron wrote: >>>>> On 12/03/2015 03:03 PM, Joe Perches wrote: >>>>>> On Thu, 2015-12-03 at 14:32 -0500, Jason Baron wrote: >>>>>>> On 12/03/2015 01:52 PM, Aaron Conole wrote: >>>>>>>> I think that as a minimum, the following patch should be evaluted, >>>>>>>> but am unsure to whom I should submit it (after I test): >>>>>> [] >>>>>>> Agreed - the intention here is certainly to have no side effects. It >>>>>>> looks like 'no_printk()' is used in quite a few other places that would >>>>>>> benefit from this change. So we probably want a generic >>>>>>> 'really_no_printk()' macro. >>>>>> >>>>>> https://lkml.org/lkml/2012/6/17/231 >>>>> >>>>> I don't see this in the tree. >>>> >>>> It never got applied. >>>> >>>>> Also maybe we should just convert >>>>> no_printk() to do what your 'eliminated_printk()'. >>>> >>>> Some of them at least. >>>> >>>>> So we can convert all users with this change? >>>> >>>> I don't think so, I think there are some >>>> function evaluation/side effects that are >>>> required. I believe some do hardware I/O. >>>> >>>> It'd be good to at least isolate them. >>>> >>>> I'm not sure how to find them via some >>>> automated tool/mechanism though. >>>> >>>> I asked Julia Lawall about it once in this >>>> thread: https://lkml.org/lkml/2014/12/3/696 >>>> >>> >>> Seems rather fragile to have side effects that we rely >>> upon hidden in a printk(). >> >> Yup. >> >>> Just convert them and see what breaks :) >> >> I appreciate your optimism. It's very 1995. >> Try it and see what happens. > > > But Aaron says that DYNAMIC_DEBUG is enabled in most major > distributions, and all these side-effects don't happen with > DYNAMIC_DEBUG. When DYNAMIC_DEBUG is enabled we have this wrapper from include/linux/dynamic_debug.h: if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)) <do debug stuff> So the compiler is not emitting the side-effects in this case. >This suggests that we can make these side-effects not > happen without DYNAMIC_DEBUG as well. > Or I am missing something here? > When DYNAMIC_DEBUG is disabled we are instead replacing pr_debug() with the 'no_printk()' function as you've pointed out. We are changing this to emit no code at all: http://marc.info/?l=linux-kernel&m=144918276518878&w=2 Thanks, -Jason -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-12-04 at 11:47 -0500, Jason Baron wrote: > When DYNAMIC_DEBUG is enabled we have this wrapper from > include/linux/dynamic_debug.h: > > if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)) > <do debug stuff> > > So the compiler is not emitting the side-effects in this > case. Huh? Do I misunderstand what you are writing? You are testing a variable that is not generally set so the call is not being performed in the general case, but the compiler can not elide the code. If the variable was enabled via the control file, the __dynamic_pr_debug would be performed with the use-after-free. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/04/2015 12:03 PM, Joe Perches wrote: > On Fri, 2015-12-04 at 11:47 -0500, Jason Baron wrote: >> When DYNAMIC_DEBUG is enabled we have this wrapper from >> include/linux/dynamic_debug.h: >> >> if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT)) >> <do debug stuff> >> >> So the compiler is not emitting the side-effects in this >> case. > > Huh? Do I misunderstand what you are writing? Yes, I wasn't terribly clear - I was trying to say that the 'side-effects', in this case the debug code and use-after-free, are hidden behind the branch. They aren't invoked unless we enable the debug statement. Thanks, -Jason > > You are testing a variable that is not generally set > so the call is not being performed in the general case, > but the compiler can not elide the code. > > If the variable was enabled via the control file, the > __dynamic_pr_debug would be performed with the > use-after-free. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/printk.h b/include/linux/printk.h index 9729565..cd24d2d 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -286,7 +286,7 @@ extern asmlinkage void dump_stack(void) __cold; printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) #else #define pr_debug(fmt, ...) \ - no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__) + ({ if(0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0;}) #endif /*