Re: [PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Herbert Xu]

On Thu, Mar 08, 2018 at 08:31:47AM -0800, Eric Dumazet wrote:
>
> Because that would not be nice.
> 
> this_cpu_read() is faster than going through raw_smp_processor_id() and
> per_cpu_ptr()

I see.  In that case I have no objections to this patch.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Eric Dumazet]

On 03/07/2018 11:33 PM, Herbert Xu wrote:

On Wed, Mar 07, 2018 at 11:24:16AM -0800, Greg Hackmann wrote:

f7c83bcbfaf5 ("net: xfrm: use __this_cpu_read per-cpu helper") added a
__this_cpu_read() call inside ipcomp_alloc_tfms().  Since this call was
introduced, the rules around per-cpu accessors have been tightened and
__this_cpu_read() cannot be used in a preemptible context.

syzkaller reported this leading to the following kernel BUG while
fuzzing sendmsg:


How about reverting f7c83bcbfaf5 instead?

Thanks,



Because that would not be nice.

this_cpu_read() is faster than going through raw_smp_processor_id() and 
per_cpu_ptr()

Re: [PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Herbert Xu]

On Wed, Mar 07, 2018 at 11:24:16AM -0800, Greg Hackmann wrote:
> f7c83bcbfaf5 ("net: xfrm: use __this_cpu_read per-cpu helper") added a
> __this_cpu_read() call inside ipcomp_alloc_tfms().  Since this call was
> introduced, the rules around per-cpu accessors have been tightened and
> __this_cpu_read() cannot be used in a preemptible context.
> 
> syzkaller reported this leading to the following kernel BUG while
> fuzzing sendmsg:

How about reverting f7c83bcbfaf5 instead?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Greg Hackmann]

On 03/07/2018 12:21 PM, Eric Dumazet wrote:
> On Wed, 2018-03-07 at 11:24 -0800, Greg Hackmann wrote:
>> f7c83bcbfaf5 ("net: xfrm: use __this_cpu_read per-cpu helper") added
>> a
>> __this_cpu_read() call inside ipcomp_alloc_tfms().  Since this call
>> was
>> introduced, the rules around per-cpu accessors have been tightened
>> and
>> __this_cpu_read() cannot be used in a preemptible context.
>>
>> syzkaller reported this leading to the following kernel BUG while
>> fuzzing sendmsg:
>>
>>
>> Cc: sta...@vger.kernel.org
> 
> Hi Greg, thanks for this patch.
> 
> 1) Please read Documentation/networking/netdev-FAQ.txt
> around line 133
> 
> 2) If you believe this needs to be backported, you must provide
> a Fixes: tag to clearly identify commit that changed the behavior.
> 
> Otherwise dozens of stable teams will have to figure out by themselves,
> wasting precious time.
> 
> Thanks.
> 

Apologies, I forgot that netdev has its own stable rules.  I'll
double-check which kernels are impacted and resend with an updated
commit log.

Re: [PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Eric Dumazet]

On Wed, 2018-03-07 at 11:24 -0800, Greg Hackmann wrote:
> f7c83bcbfaf5 ("net: xfrm: use __this_cpu_read per-cpu helper") added
> a
> __this_cpu_read() call inside ipcomp_alloc_tfms().  Since this call
> was
> introduced, the rules around per-cpu accessors have been tightened
> and
> __this_cpu_read() cannot be used in a preemptible context.
> 
> syzkaller reported this leading to the following kernel BUG while
> fuzzing sendmsg:
> 
> 
> Cc: sta...@vger.kernel.org

Hi Greg, thanks for this patch.

1) Please read Documentation/networking/netdev-FAQ.txt
around line 133

2) If you believe this needs to be backported, you must provide
a Fixes: tag to clearly identify commit that changed the behavior.

Otherwise dozens of stable teams will have to figure out by themselves,
wasting precious time.

Thanks.

[PATCH] net: xfrm: use preempt-safe this_cpu_read() in ipcomp_alloc_tfms()

[Greg Hackmann]

f7c83bcbfaf5 ("net: xfrm: use __this_cpu_read per-cpu helper") added a
__this_cpu_read() call inside ipcomp_alloc_tfms().  Since this call was
introduced, the rules around per-cpu accessors have been tightened and
__this_cpu_read() cannot be used in a preemptible context.

syzkaller reported this leading to the following kernel BUG while
fuzzing sendmsg:

BUG: using __this_cpu_read() in preemptible [] code: repro/3101
caller is ipcomp_init_state+0x185/0x990
CPU: 3 PID: 3101 Comm: repro Not tainted 4.16.0-rc4-00123-g86f84779d8e9 #154
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Call Trace:
 dump_stack+0xb9/0x115
 check_preemption_disabled+0x1cb/0x1f0
 ipcomp_init_state+0x185/0x990
 ? __xfrm_init_state+0x876/0xc20
 ? lock_downgrade+0x5e0/0x5e0
 ipcomp4_init_state+0xaa/0x7c0
 __xfrm_init_state+0x3eb/0xc20
 xfrm_init_state+0x19/0x60
 pfkey_add+0x20df/0x36f0
 ? pfkey_broadcast+0x3dd/0x600
 ? pfkey_sock_destruct+0x340/0x340
 ? pfkey_seq_stop+0x80/0x80
 ? __skb_clone+0x236/0x750
 ? kmem_cache_alloc+0x1f6/0x260
 ? pfkey_sock_destruct+0x340/0x340
 ? pfkey_process+0x62a/0x6f0
 pfkey_process+0x62a/0x6f0
 ? pfkey_send_new_mapping+0x11c0/0x11c0
 ? mutex_lock_io_nested+0x1390/0x1390
 pfkey_sendmsg+0x383/0x750
 ? dump_sp+0x430/0x430
 sock_sendmsg+0xc0/0x100
 ___sys_sendmsg+0x6c8/0x8b0
 ? copy_msghdr_from_user+0x3b0/0x3b0
 ? pagevec_lru_move_fn+0x144/0x1f0
 ? find_held_lock+0x32/0x1c0
 ? do_huge_pmd_anonymous_page+0xc43/0x11e0
 ? lock_downgrade+0x5e0/0x5e0
 ? get_kernel_page+0xb0/0xb0
 ? _raw_spin_unlock+0x29/0x40
 ? do_huge_pmd_anonymous_page+0x400/0x11e0
 ? __handle_mm_fault+0x553/0x2460
 ? __fget_light+0x163/0x1f0
 ? __sys_sendmsg+0xc7/0x170
 __sys_sendmsg+0xc7/0x170
 ? SyS_shutdown+0x1a0/0x1a0
 ? __do_page_fault+0x5a0/0xca0
 ? lock_downgrade+0x5e0/0x5e0
 SyS_sendmsg+0x27/0x40
 ? __sys_sendmsg+0x170/0x170
 do_syscall_64+0x19f/0x640
 entry_SYSCALL_64_after_hwframe+0x42/0xb7
RIP: 0033:0x7f0ee73dfb79
RSP: 002b:7ffe14fc15a8 EFLAGS: 0207 ORIG_RAX: 002e
RAX: ffda RBX:  RCX: 7f0ee73dfb79
RDX:  RSI: 208befc8 RDI: 0004
RBP: 7ffe14fc15b0 R08: 7ffe14fc15c0 R09: 7ffe14fc15c0
R10:  R11: 0207 R12: 00400440
R13: 7ffe14fc16b0 R14:  R15: 

Cc: sta...@vger.kernel.org
Signed-off-by: Greg Hackmann 
---
 net/xfrm/xfrm_ipcomp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index ccfdc7115a83..a00ec715aa46 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -283,7 +283,7 @@ static struct crypto_comp * __percpu 
*ipcomp_alloc_tfms(const char *alg_name)
struct crypto_comp *tfm;
 
/* This can be any valid CPU ID so we don't need locking. */
-   tfm = __this_cpu_read(*pos->tfms);
+   tfm = this_cpu_read(*pos->tfms);
 
if (!strcmp(crypto_comp_name(tfm), alg_name)) {
pos->users++;
-- 
2.16.2.395.g2e18187dfd-goog