2018-03-07 19:25:41

by Greg Hackmann

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

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 [00000000] 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:00007ffe14fc15a8 EFLAGS: 00000207 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f0ee73dfb79
RDX: 0000000000000000 RSI: 00000000208befc8 RDI: 0000000000000004
RBP: 00007ffe14fc15b0 R08: 00007ffe14fc15c0 R09: 00007ffe14fc15c0
R10: 0000000000000000 R11: 0000000000000207 R12: 0000000000400440
R13: 00007ffe14fc16b0 R14: 0000000000000000 R15: 0000000000000000

Cc: [email protected]
Signed-off-by: Greg Hackmann <[email protected]>
---
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



2018-03-07 20:22:52

by Eric Dumazet

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

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: [email protected]

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.


2018-03-07 20:51:25

by Greg Hackmann

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

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: [email protected]
>
> 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.

2018-03-08 07:36:30

by Herbert Xu

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2018-03-08 16:33:11

by Eric Dumazet

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



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()


2018-03-08 18:41:54

by Herbert Xu

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

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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt