* [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
@ 2013-10-14 16:03 Michal Kubecek
2013-10-15 8:33 ` Steffen Klassert
2013-10-16 12:32 ` [PATCH ipsec] " Herbert Xu
0 siblings, 2 replies; 19+ messages in thread
From: Michal Kubecek @ 2013-10-14 16:03 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev
In ipcomp_compress(), sortirq is enabled too early, allowing the
per-cpu scratch buffer to be rewritten by ipcomp_decompress()
(called on the same CPU in softirq context) between populating
the buffer and copying the compressed data to the skb.
Add similar protection into ipcomp_decompress() as it can be
called from process context as well (even if such scenario seems
a bit artificial).
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
net/xfrm/xfrm_ipcomp.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 2906d52..96946fb 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -48,9 +48,11 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
const int cpu = get_cpu();
u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
- int err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
+ int err;
int len;
+ local_bh_disable();
+ err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
if (err)
goto out;
@@ -103,6 +105,7 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
err = 0;
out:
+ local_bh_enable();
put_cpu();
return err;
}
@@ -148,7 +151,6 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
local_bh_disable();
err = crypto_comp_compress(tfm, start, plen, scratch, &dlen);
- local_bh_enable();
if (err)
goto out;
@@ -158,12 +160,14 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
}
memcpy(start + sizeof(struct ip_comp_hdr), scratch, dlen);
+ local_bh_enable();
put_cpu();
pskb_trim(skb, dlen + sizeof(struct ip_comp_hdr));
return 0;
out:
+ local_bh_enable();
put_cpu();
return err;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-14 16:03 [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
@ 2013-10-15 8:33 ` Steffen Klassert
2013-10-15 8:59 ` Fan Du
` (2 more replies)
2013-10-16 12:32 ` [PATCH ipsec] " Herbert Xu
1 sibling, 3 replies; 19+ messages in thread
From: Steffen Klassert @ 2013-10-15 8:33 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Herbert Xu, David S. Miller, netdev
On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
> In ipcomp_compress(), sortirq is enabled too early, allowing the
> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> (called on the same CPU in softirq context) between populating
> the buffer and copying the compressed data to the skb.
>
> Add similar protection into ipcomp_decompress() as it can be
> called from process context as well (even if such scenario seems
> a bit artificial).
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/xfrm/xfrm_ipcomp.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
> index 2906d52..96946fb 100644
> --- a/net/xfrm/xfrm_ipcomp.c
> +++ b/net/xfrm/xfrm_ipcomp.c
> @@ -48,9 +48,11 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
> const int cpu = get_cpu();
> u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
> struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
> - int err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
> + int err;
> int len;
>
> + local_bh_disable();
Maybe we could disable the BHs before we fetch the percpu pointers.
Then we can use smp_processor_id() to get the cpu. With that we
could get rid of a (now useless) preempt_disable()/preempt_enable()
pair. Same could be done in ipcomp_compress().
Looks ok otherwise. Thanks!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-15 8:33 ` Steffen Klassert
@ 2013-10-15 8:59 ` Fan Du
2013-10-15 9:46 ` Steffen Klassert
2013-10-15 20:55 ` Michal Kubecek
2013-10-15 21:40 ` [PATCH ipsec v2] " Michal Kubecek
2 siblings, 1 reply; 19+ messages in thread
From: Fan Du @ 2013-10-15 8:59 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Michal Kubecek, Herbert Xu, David S. Miller, netdev
On 2013年10月15日 16:33, Steffen Klassert wrote:
> On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
>> In ipcomp_compress(), sortirq is enabled too early, allowing the
>> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
>> (called on the same CPU in softirq context) between populating
>> the buffer and copying the compressed data to the skb.
>>
>> Add similar protection into ipcomp_decompress() as it can be
>> called from process context as well (even if such scenario seems
>> a bit artificial).
>>
>> Signed-off-by: Michal Kubecek<mkubecek@suse.cz>
>> ---
>> net/xfrm/xfrm_ipcomp.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
>> index 2906d52..96946fb 100644
>> --- a/net/xfrm/xfrm_ipcomp.c
>> +++ b/net/xfrm/xfrm_ipcomp.c
>> @@ -48,9 +48,11 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
>> const int cpu = get_cpu();
>> u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
>> struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
>> - int err = crypto_comp_decompress(tfm, start, plen, scratch,&dlen);
>> + int err;
>> int len;
>>
>> + local_bh_disable();
>
> Maybe we could disable the BHs before we fetch the percpu pointers.
> Then we can use smp_processor_id() to get the cpu. With that we
> could get rid of a (now useless) preempt_disable()/preempt_enable()
> pair. Same could be done in ipcomp_compress().
Is it possible that two tasks race scratch buffer when both of them trying to compress data
without preempt disabled? for example, when task A working on compression, then task B
with higher priority preempts task A, and try to touch scratch buffer, which leaves stale
data for task A after then.
I think we needs preempt disabled for such case, otherwise I overlook codes in somewhere else.
> Looks ok otherwise. Thanks!
>
> --
> 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
>
--
浮沉随浪只记今朝笑
--fan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-15 8:59 ` Fan Du
@ 2013-10-15 9:46 ` Steffen Klassert
0 siblings, 0 replies; 19+ messages in thread
From: Steffen Klassert @ 2013-10-15 9:46 UTC (permalink / raw)
To: Fan Du; +Cc: Michal Kubecek, Herbert Xu, David S. Miller, netdev
On Tue, Oct 15, 2013 at 04:59:04PM +0800, Fan Du wrote:
>
>
> On 2013年10月15日 16:33, Steffen Klassert wrote:
> >
> >Maybe we could disable the BHs before we fetch the percpu pointers.
> >Then we can use smp_processor_id() to get the cpu. With that we
> >could get rid of a (now useless) preempt_disable()/preempt_enable()
> >pair. Same could be done in ipcomp_compress().
>
> Is it possible that two tasks race scratch buffer when both of them trying to compress data
> without preempt disabled? for example, when task A working on compression, then task B
> with higher priority preempts task A, and try to touch scratch buffer, which leaves stale
> data for task A after then.
>
> I think we needs preempt disabled for such case, otherwise I overlook codes in somewhere else.
>
You overlook that preemption is disabled if the BHs are disabled.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-15 8:33 ` Steffen Klassert
2013-10-15 8:59 ` Fan Du
@ 2013-10-15 20:55 ` Michal Kubecek
2013-10-15 21:40 ` [PATCH ipsec v2] " Michal Kubecek
2 siblings, 0 replies; 19+ messages in thread
From: Michal Kubecek @ 2013-10-15 20:55 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev
On Tue, Oct 15, 2013 at 10:33:48AM +0200, Steffen Klassert wrote:
> > diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
> > index 2906d52..96946fb 100644
> > --- a/net/xfrm/xfrm_ipcomp.c
> > +++ b/net/xfrm/xfrm_ipcomp.c
> > @@ -48,9 +48,11 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
> > const int cpu = get_cpu();
> > u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
> > struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
> > - int err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
> > + int err;
> > int len;
> >
> > + local_bh_disable();
>
> Maybe we could disable the BHs before we fetch the percpu pointers.
> Then we can use smp_processor_id() to get the cpu. With that we
> could get rid of a (now useless) preempt_disable()/preempt_enable()
> pair. Same could be done in ipcomp_compress().
Sounds like a good idea. I'll send v2 after some basic testing.
Michal Kubecek
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH ipsec v2] xfrm: prevent ipcomp scratch buffer race condition
2013-10-15 8:33 ` Steffen Klassert
2013-10-15 8:59 ` Fan Du
2013-10-15 20:55 ` Michal Kubecek
@ 2013-10-15 21:40 ` Michal Kubecek
2013-10-15 22:44 ` Eric Dumazet
2 siblings, 1 reply; 19+ messages in thread
From: Michal Kubecek @ 2013-10-15 21:40 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev
In ipcomp_compress(), sortirq is enabled too early, allowing the
per-cpu scratch buffer to be rewritten by ipcomp_decompress()
(called on the same CPU in softirq context) between populating
the buffer and copying the compressed data to the skb.
Add similar protection into ipcomp_decompress() as it can be
called from process context as well (even if such scenario seems
a bit artificial).
v2: as pointed out by Steffen Klassert, if we also move the
local_bh_disable() before reading the per-cpu pointers, we can
get rid of get_cpu()/put_cpu().
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
net/xfrm/xfrm_ipcomp.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 2906d52..9fe4f42 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -45,12 +45,17 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
const int plen = skb->len;
int dlen = IPCOMP_SCRATCH_SIZE;
const u8 *start = skb->data;
- const int cpu = get_cpu();
- u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
- struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
- int err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
+ struct crypto_comp *tfm;
+ u8 *scratch;
+ int cpu;
+ int err;
int len;
+ local_bh_disable();
+ cpu = smp_processor_id();
+ scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
+ tfm = *per_cpu_ptr(ipcd->tfms, cpu);
+ err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
if (err)
goto out;
@@ -103,7 +108,7 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
err = 0;
out:
- put_cpu();
+ local_bh_enable();
return err;
}
@@ -141,14 +146,16 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
const int plen = skb->len;
int dlen = IPCOMP_SCRATCH_SIZE;
u8 *start = skb->data;
- const int cpu = get_cpu();
- u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
- struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
+ struct crypto_comp *tfm;
+ u8 *scratch;
+ int cpu;
int err;
local_bh_disable();
+ cpu = smp_processor_id();
+ scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
+ tfm = *per_cpu_ptr(ipcd->tfms, cpu);
err = crypto_comp_compress(tfm, start, plen, scratch, &dlen);
- local_bh_enable();
if (err)
goto out;
@@ -158,13 +165,13 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
}
memcpy(start + sizeof(struct ip_comp_hdr), scratch, dlen);
- put_cpu();
+ local_bh_enable();
pskb_trim(skb, dlen + sizeof(struct ip_comp_hdr));
return 0;
out:
- put_cpu();
+ local_bh_enable();
return err;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec v2] xfrm: prevent ipcomp scratch buffer race condition
2013-10-15 21:40 ` [PATCH ipsec v2] " Michal Kubecek
@ 2013-10-15 22:44 ` Eric Dumazet
0 siblings, 0 replies; 19+ messages in thread
From: Eric Dumazet @ 2013-10-15 22:44 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Steffen Klassert, Herbert Xu, David S. Miller, netdev
On Tue, 2013-10-15 at 23:40 +0200, Michal Kubecek wrote:
> In ipcomp_compress(), sortirq is enabled too early, allowing the
> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> (called on the same CPU in softirq context) between populating
> the buffer and copying the compressed data to the skb.
>
> Add similar protection into ipcomp_decompress() as it can be
> called from process context as well (even if such scenario seems
> a bit artificial).
>
> v2: as pointed out by Steffen Klassert, if we also move the
> local_bh_disable() before reading the per-cpu pointers, we can
> get rid of get_cpu()/put_cpu().
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/xfrm/xfrm_ipcomp.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
> index 2906d52..9fe4f42 100644
> --- a/net/xfrm/xfrm_ipcomp.c
> +++ b/net/xfrm/xfrm_ipcomp.c
> @@ -45,12 +45,17 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
> const int plen = skb->len;
> int dlen = IPCOMP_SCRATCH_SIZE;
> const u8 *start = skb->data;
> - const int cpu = get_cpu();
> - u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
> - struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
> - int err = crypto_comp_decompress(tfm, start, plen, scratch, &dlen);
> + struct crypto_comp *tfm;
> + u8 *scratch;
> + int cpu;
> + int err;
> int len;
>
> + local_bh_disable();
> + cpu = smp_processor_id();
> + scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
> + tfm = *per_cpu_ptr(ipcd->tfms, cpu);
Have you tried this_cpu_ptr() instead ?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-14 16:03 [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
2013-10-15 8:33 ` Steffen Klassert
@ 2013-10-16 12:32 ` Herbert Xu
2013-10-17 9:55 ` Steffen Klassert
2013-10-17 11:01 ` [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
1 sibling, 2 replies; 19+ messages in thread
From: Herbert Xu @ 2013-10-16 12:32 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Steffen Klassert, David S. Miller, netdev
On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
> In ipcomp_compress(), sortirq is enabled too early, allowing the
> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> (called on the same CPU in softirq context) between populating
> the buffer and copying the compressed data to the skb.
Good catch.
> Add similar protection into ipcomp_decompress() as it can be
> called from process context as well (even if such scenario seems
> a bit artificial).
I don't think this is possible or otherwise xfrm_input will
dead-lock.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-16 12:32 ` [PATCH ipsec] " Herbert Xu
@ 2013-10-17 9:55 ` Steffen Klassert
2013-10-17 13:07 ` [PATCH ipsec v3] " Michal Kubecek
2013-10-17 11:01 ` [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
1 sibling, 1 reply; 19+ messages in thread
From: Steffen Klassert @ 2013-10-17 9:55 UTC (permalink / raw)
To: Herbert Xu; +Cc: Michal Kubecek, David S. Miller, netdev
On Wed, Oct 16, 2013 at 08:32:05PM +0800, Herbert Xu wrote:
> On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
> > In ipcomp_compress(), sortirq is enabled too early, allowing the
> > per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> > (called on the same CPU in softirq context) between populating
> > the buffer and copying the compressed data to the skb.
>
> Good catch.
>
> > Add similar protection into ipcomp_decompress() as it can be
> > called from process context as well (even if such scenario seems
> > a bit artificial).
>
> I don't think this is possible or otherwise xfrm_input will
> dead-lock.
>
Michal, please incorporate the feedback from Herbert and Eric,
I'll take it into the ipsec tree then. Thanks!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-16 12:32 ` [PATCH ipsec] " Herbert Xu
2013-10-17 9:55 ` Steffen Klassert
@ 2013-10-17 11:01 ` Michal Kubecek
2013-10-17 11:04 ` Herbert Xu
1 sibling, 1 reply; 19+ messages in thread
From: Michal Kubecek @ 2013-10-17 11:01 UTC (permalink / raw)
To: Herbert Xu; +Cc: Steffen Klassert, David S. Miller, netdev
On Wed, Oct 16, 2013 at 08:32:05PM +0800, Herbert Xu wrote:
> On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
>
> > Add similar protection into ipcomp_decompress() as it can be
> > called from process context as well (even if such scenario seems
> > a bit artificial).
>
> I don't think this is possible or otherwise xfrm_input will
> dead-lock.
When I was thinking about this, I came with two situations when this
might IMHO happen:
1. We are sending a packet to ourselves; this could mean not only a
local loop but also e.g. a packets between an LXC container and the host
or between two LXC containers. While it doesn't make much sense to use
compression in such case, it might happen while testing a solution which
is going to be used in a normal setup later.
2. The packet was passed to tun/tap device by a userspace application.
Am I wrong?
Michal Kubecek
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 11:01 ` [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
@ 2013-10-17 11:04 ` Herbert Xu
2013-10-17 13:13 ` Michal Kubecek
0 siblings, 1 reply; 19+ messages in thread
From: Herbert Xu @ 2013-10-17 11:04 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Steffen Klassert, David S. Miller, netdev
On Thu, Oct 17, 2013 at 01:01:49PM +0200, Michal Kubecek wrote:
>
> When I was thinking about this, I came with two situations when this
> might IMHO happen:
>
> 1. We are sending a packet to ourselves; this could mean not only a
> local loop but also e.g. a packets between an LXC container and the host
> or between two LXC containers. While it doesn't make much sense to use
> compression in such case, it might happen while testing a solution which
> is going to be used in a normal setup later.
>
> 2. The packet was passed to tun/tap device by a userspace application.
>
> Am I wrong?
I hope so :) Because otherwise xfrm_input will surely dead-lock since
it uses spin_lock.
The cases you mentioned should all go through netif_rx which will
then do the processing in BH context.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH ipsec v3] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 9:55 ` Steffen Klassert
@ 2013-10-17 13:07 ` Michal Kubecek
2013-10-17 13:38 ` Eric Dumazet
2013-10-18 9:25 ` Steffen Klassert
0 siblings, 2 replies; 19+ messages in thread
From: Michal Kubecek @ 2013-10-17 13:07 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev, Eric Dumazet
In ipcomp_compress(), sortirq is enabled too early, allowing the
per-cpu scratch buffer to be rewritten by ipcomp_decompress()
(called on the same CPU in softirq context) between populating
the buffer and copying the compressed data to the skb.
v2: as pointed out by Steffen Klassert, if we also move the
local_bh_disable() before reading the per-cpu pointers, we can
get rid of get_cpu()/put_cpu().
v3: removed ipcomp_decompress part (as explained by Herbert Xu,
it cannot be called from process context), get rid of cpu
variable (thanks to Eric Dumazet)
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
net/xfrm/xfrm_ipcomp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 2906d52..3be02b6 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -141,14 +141,14 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
const int plen = skb->len;
int dlen = IPCOMP_SCRATCH_SIZE;
u8 *start = skb->data;
- const int cpu = get_cpu();
- u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
- struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
+ struct crypto_comp *tfm;
+ u8 *scratch;
int err;
local_bh_disable();
+ scratch = *this_cpu_ptr(ipcomp_scratches);
+ tfm = *this_cpu_ptr(ipcd->tfms);
err = crypto_comp_compress(tfm, start, plen, scratch, &dlen);
- local_bh_enable();
if (err)
goto out;
@@ -158,13 +158,13 @@ static int ipcomp_compress(struct xfrm_state *x, struct sk_buff *skb)
}
memcpy(start + sizeof(struct ip_comp_hdr), scratch, dlen);
- put_cpu();
+ local_bh_enable();
pskb_trim(skb, dlen + sizeof(struct ip_comp_hdr));
return 0;
out:
- put_cpu();
+ local_bh_enable();
return err;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 11:04 ` Herbert Xu
@ 2013-10-17 13:13 ` Michal Kubecek
0 siblings, 0 replies; 19+ messages in thread
From: Michal Kubecek @ 2013-10-17 13:13 UTC (permalink / raw)
To: Herbert Xu; +Cc: Steffen Klassert, David S. Miller, netdev
On Thu, Oct 17, 2013 at 07:04:20PM +0800, Herbert Xu wrote:
V> On Thu, Oct 17, 2013 at 01:01:49PM +0200, Michal Kubecek wrote:
> >
> > Am I wrong?
>
> I hope so :) Because otherwise xfrm_input will surely dead-lock since
> it uses spin_lock.
>
> The cases you mentioned should all go through netif_rx which will
> then do the processing in BH context.
You are right. Thank you for the correction.
Michal Kubecek
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec v3] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 13:07 ` [PATCH ipsec v3] " Michal Kubecek
@ 2013-10-17 13:38 ` Eric Dumazet
2013-10-17 13:39 ` Herbert Xu
2013-10-18 9:25 ` Steffen Klassert
1 sibling, 1 reply; 19+ messages in thread
From: Eric Dumazet @ 2013-10-17 13:38 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Steffen Klassert, Herbert Xu, David S. Miller, netdev
On Thu, 2013-10-17 at 15:07 +0200, Michal Kubecek wrote:
> In ipcomp_compress(), sortirq is enabled too early, allowing the
> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> (called on the same CPU in softirq context) between populating
> the buffer and copying the compressed data to the skb.
>
> v2: as pointed out by Steffen Klassert, if we also move the
> local_bh_disable() before reading the per-cpu pointers, we can
> get rid of get_cpu()/put_cpu().
>
> v3: removed ipcomp_decompress part (as explained by Herbert Xu,
> it cannot be called from process context), get rid of cpu
> variable (thanks to Eric Dumazet)
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec v3] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 13:38 ` Eric Dumazet
@ 2013-10-17 13:39 ` Herbert Xu
0 siblings, 0 replies; 19+ messages in thread
From: Herbert Xu @ 2013-10-17 13:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Michal Kubecek, Steffen Klassert, David S. Miller, netdev
On Thu, Oct 17, 2013 at 06:38:37AM -0700, Eric Dumazet wrote:
> On Thu, 2013-10-17 at 15:07 +0200, Michal Kubecek wrote:
> > In ipcomp_compress(), sortirq is enabled too early, allowing the
> > per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> > (called on the same CPU in softirq context) between populating
> > the buffer and copying the compressed data to the skb.
> >
> > v2: as pointed out by Steffen Klassert, if we also move the
> > local_bh_disable() before reading the per-cpu pointers, we can
> > get rid of get_cpu()/put_cpu().
> >
> > v3: removed ipcomp_decompress part (as explained by Herbert Xu,
> > it cannot be called from process context), get rid of cpu
> > variable (thanks to Eric Dumazet)
> >
> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> > ---
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec v3] xfrm: prevent ipcomp scratch buffer race condition
2013-10-17 13:07 ` [PATCH ipsec v3] " Michal Kubecek
2013-10-17 13:38 ` Eric Dumazet
@ 2013-10-18 9:25 ` Steffen Klassert
2013-10-18 10:54 ` [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches Eric Dumazet
1 sibling, 1 reply; 19+ messages in thread
From: Steffen Klassert @ 2013-10-18 9:25 UTC (permalink / raw)
To: Michal Kubecek; +Cc: Herbert Xu, David S. Miller, netdev, Eric Dumazet
On Thu, Oct 17, 2013 at 03:07:40PM +0200, Michal Kubecek wrote:
> In ipcomp_compress(), sortirq is enabled too early, allowing the
> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
> (called on the same CPU in softirq context) between populating
> the buffer and copying the compressed data to the skb.
>
> v2: as pointed out by Steffen Klassert, if we also move the
> local_bh_disable() before reading the per-cpu pointers, we can
> get rid of get_cpu()/put_cpu().
>
> v3: removed ipcomp_decompress part (as explained by Herbert Xu,
> it cannot be called from process context), get rid of cpu
> variable (thanks to Eric Dumazet)
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Applied to the ipsec tree, thanks everyone!
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches
2013-10-18 9:25 ` Steffen Klassert
@ 2013-10-18 10:54 ` Eric Dumazet
2013-10-18 12:46 ` Herbert Xu
2013-10-21 14:48 ` Steffen Klassert
0 siblings, 2 replies; 19+ messages in thread
From: Eric Dumazet @ 2013-10-18 10:54 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Herbert Xu, David S. Miller, netdev
From: Eric Dumazet <edumazet@google.com>
scratches are per cpu, we can use vmalloc_node() for proper
NUMA affinity.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/xfrm/xfrm_ipcomp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index 2906d52..b943c7f 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -220,8 +220,8 @@ static void ipcomp_free_scratches(void)
static void * __percpu *ipcomp_alloc_scratches(void)
{
- int i;
void * __percpu *scratches;
+ int i;
if (ipcomp_scratch_users++)
return ipcomp_scratches;
@@ -233,7 +233,9 @@ static void * __percpu *ipcomp_alloc_scratches(void)
ipcomp_scratches = scratches;
for_each_possible_cpu(i) {
- void *scratch = vmalloc(IPCOMP_SCRATCH_SIZE);
+ void *scratch;
+
+ scratch = vmalloc_node(IPCOMP_SCRATCH_SIZE, cpu_to_node(i));
if (!scratch)
return NULL;
*per_cpu_ptr(scratches, i) = scratch;
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches
2013-10-18 10:54 ` [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches Eric Dumazet
@ 2013-10-18 12:46 ` Herbert Xu
2013-10-21 14:48 ` Steffen Klassert
1 sibling, 0 replies; 19+ messages in thread
From: Herbert Xu @ 2013-10-18 12:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Steffen Klassert, David S. Miller, netdev
On Fri, Oct 18, 2013 at 03:54:16AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> scratches are per cpu, we can use vmalloc_node() for proper
> NUMA affinity.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
I agree completely.
Thanks!
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches
2013-10-18 10:54 ` [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches Eric Dumazet
2013-10-18 12:46 ` Herbert Xu
@ 2013-10-21 14:48 ` Steffen Klassert
1 sibling, 0 replies; 19+ messages in thread
From: Steffen Klassert @ 2013-10-21 14:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Herbert Xu, David S. Miller, netdev
On Fri, Oct 18, 2013 at 03:54:16AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> scratches are per cpu, we can use vmalloc_node() for proper
> NUMA affinity.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied to ipsec-next, thanks a lot Eric!
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-10-21 14:48 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-14 16:03 [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
2013-10-15 8:33 ` Steffen Klassert
2013-10-15 8:59 ` Fan Du
2013-10-15 9:46 ` Steffen Klassert
2013-10-15 20:55 ` Michal Kubecek
2013-10-15 21:40 ` [PATCH ipsec v2] " Michal Kubecek
2013-10-15 22:44 ` Eric Dumazet
2013-10-16 12:32 ` [PATCH ipsec] " Herbert Xu
2013-10-17 9:55 ` Steffen Klassert
2013-10-17 13:07 ` [PATCH ipsec v3] " Michal Kubecek
2013-10-17 13:38 ` Eric Dumazet
2013-10-17 13:39 ` Herbert Xu
2013-10-18 9:25 ` Steffen Klassert
2013-10-18 10:54 ` [PATCH ipsec-next] xfrm: use vmalloc_node() for percpu scratches Eric Dumazet
2013-10-18 12:46 ` Herbert Xu
2013-10-21 14:48 ` Steffen Klassert
2013-10-17 11:01 ` [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition Michal Kubecek
2013-10-17 11:04 ` Herbert Xu
2013-10-17 13:13 ` Michal Kubecek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.