On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>> On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet <edumazet@xxxxxxxxxx> 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...
>>
>> 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
>
> #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.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
