From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-17 14:57:25
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Thanks!
Daniel Borkmann (3):
mm, percpu: add support for __GFP_NOWARN flag
bpf: fix splat for illegal devmap percpu allocation
bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
kernel/bpf/arraymap.c | 2 +-
kernel/bpf/devmap.c | 5 +++--
kernel/bpf/hashtab.c | 4 ----
mm/percpu.c | 15 ++++++++++-----
4 files changed, 14 insertions(+), 12 deletions(-)
--
1.9.3
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-17 14:56:27
Add an option for pcpu_alloc() to support __GFP_NOWARN flag.
Currently, we always throw a warning when size or alignment
is unsupported (and also dump stack on failed allocation
requests). The warning itself is harmless since we return
NULL anyway for any failed request, which callers are
required to handle anyway. However, it becomes harmful when
panic_on_warn is set.
The rationale for the WARN() in pcpu_alloc() is that it can
be tracked when larger than supported allocation requests are
made such that allocations limits can be tweaked if warranted.
This makes sense for in-kernel users, however, there are users
of pcpu allocator where allocation size is derived from user
space requests, e.g. when creating BPF maps. In these cases,
the requests should fail gracefully without throwing a splat.
The current work-around was to check allocation size against
the upper limit of PCPU_MIN_UNIT_SIZE from call-sites for
bailing out prior to a call to pcpu_alloc() in order to
avoid throwing the WARN(). This is bad in multiple ways since
PCPU_MIN_UNIT_SIZE is an implementation detail, and having
the checks on call-sites only complicates the code for no
good reason. Thus, lets fix it generically by supporting the
__GFP_NOWARN flag that users can then use with calling the
__alloc_percpu_gfp() helper instead.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
mm/percpu.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-17 14:56:29
It was reported that syzkaller was able to trigger a splat on
devmap percpu allocation due to illegal/unsupported allocation
request size passed to __alloc_percpu():
[ 70.094249] illegal size (32776) or align (8) for percpu allocation
[ 70.094256] ------------[ cut here ]------------
[ 70.094259] WARNING: CPU: 3 PID: 3451 at mm/percpu.c:1365 pcpu_alloc+0x96/0x630
[...]
[ 70.094325] Call Trace:
[ 70.094328] __alloc_percpu_gfp+0x12/0x20
[ 70.094330] dev_map_alloc+0x134/0x1e0
[ 70.094331] SyS_bpf+0x9bc/0x1610
[ 70.094333] ? selinux_task_setrlimit+0x5a/0x60
[ 70.094334] ? security_task_setrlimit+0x43/0x60
[ 70.094336] entry_SYSCALL_64_fastpath+0x1a/0xa5
This was due to too large max_entries for the map such that we
surpassed the upper limit of PCPU_MIN_UNIT_SIZE. It's fine to
fail naturally here, so switch to __alloc_percpu_gfp() and pass
__GFP_NOWARN instead.
Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Shankara Pailoor <redacted>
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/devmap.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -111,8 +111,9 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)err=-ENOMEM;/* A per cpu bitfield with a bit per possible net device */-dtab->flush_needed=__alloc_percpu(dev_map_bitmap_size(attr),-__alignof__(unsignedlong));+dtab->flush_needed=__alloc_percpu_gfp(dev_map_bitmap_size(attr),+__alignof__(unsignedlong),+GFP_KERNEL|__GFP_NOWARN);if(!dtab->flush_needed)gotofree_dtab;
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-17 14:56:32
PCPU_MIN_UNIT_SIZE is an implementation detail of the percpu
allocator. Given we support __GFP_NOWARN now, lets just let
the allocation request fail naturally instead. The two call
sites from BPF mistakenly assumed __GFP_NOWARN would work, so
no changes needed to their actual __alloc_percpu_gfp() calls
which use the flag already.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/arraymap.c | 2 +-
kernel/bpf/hashtab.c | 4 ----
2 files changed, 1 insertion(+), 5 deletions(-)
@@ -317,10 +317,6 @@ static struct bpf_map *htab_map_alloc(union bpf_attr *attr)*/gotofree_htab;-if(percpu&&round_up(htab->map.value_size,8)>PCPU_MIN_UNIT_SIZE)-/* make sure the size for pcpu_alloc() is reasonable */-gotofree_htab;-htab->elem_size=sizeof(structhtab_elem)+round_up(htab->map.key_size,8);if(percpu)
From: David Laight <hidden> Date: 2017-10-17 15:03:11
From: Daniel Borkmann
Sent: 17 October 2017 15:56
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Does it make sense to allow the user program to try to allocate ever
smaller very large maps until it finds one that succeeds - thus
using up all the percpu space?
Or is this a 'root only' 'shoot self in foot' job?
David
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-17 15:11:23
On 10/17/2017 05:03 PM, David Laight wrote:
From: Daniel Borkmann
quoted
Sent: 17 October 2017 15:56
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Does it make sense to allow the user program to try to allocate ever
smaller very large maps until it finds one that succeeds - thus
using up all the percpu space?
Or is this a 'root only' 'shoot self in foot' job?
It's root only although John still has a pending fix to be flushed
out for -net first in the next days to actually enforce that cap
(devmap is not in an official kernel yet at this point, so all good),
but apart from this, all map allocs in general are accounted for
as well.
Thanks,
Daniel
On Tue, Oct 17, 2017 at 04:55:52PM +0200, Daniel Borkmann wrote:
Add an option for pcpu_alloc() to support __GFP_NOWARN flag.
Currently, we always throw a warning when size or alignment
is unsupported (and also dump stack on failed allocation
requests). The warning itself is harmless since we return
NULL anyway for any failed request, which callers are
required to handle anyway. However, it becomes harmful when
panic_on_warn is set.
The rationale for the WARN() in pcpu_alloc() is that it can
be tracked when larger than supported allocation requests are
made such that allocations limits can be tweaked if warranted.
This makes sense for in-kernel users, however, there are users
of pcpu allocator where allocation size is derived from user
space requests, e.g. when creating BPF maps. In these cases,
the requests should fail gracefully without throwing a splat.
The current work-around was to check allocation size against
the upper limit of PCPU_MIN_UNIT_SIZE from call-sites for
bailing out prior to a call to pcpu_alloc() in order to
avoid throwing the WARN(). This is bad in multiple ways since
PCPU_MIN_UNIT_SIZE is an implementation detail, and having
the checks on call-sites only complicates the code for no
good reason. Thus, lets fix it generically by supporting the
__GFP_NOWARN flag that users can then use with calling the
__alloc_percpu_gfp() helper instead.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
The approach looks great to me. We've been doing this dance around
allocator warning for long time. It's really not a job of bpf code
to guess into valid parameters of pcpu alloc.
Adding support for __GFP_NOWARN and using it in bpf is much cleaner
fix that avoids layering violations.
Acked-by: Alexei Starovoitov <ast@kernel.org>
On Tue, Oct 17, 2017 at 04:55:53PM +0200, Daniel Borkmann wrote:
It was reported that syzkaller was able to trigger a splat on
devmap percpu allocation due to illegal/unsupported allocation
request size passed to __alloc_percpu():
[ 70.094249] illegal size (32776) or align (8) for percpu allocation
[ 70.094256] ------------[ cut here ]------------
[ 70.094259] WARNING: CPU: 3 PID: 3451 at mm/percpu.c:1365 pcpu_alloc+0x96/0x630
[...]
[ 70.094325] Call Trace:
[ 70.094328] __alloc_percpu_gfp+0x12/0x20
[ 70.094330] dev_map_alloc+0x134/0x1e0
[ 70.094331] SyS_bpf+0x9bc/0x1610
[ 70.094333] ? selinux_task_setrlimit+0x5a/0x60
[ 70.094334] ? security_task_setrlimit+0x43/0x60
[ 70.094336] entry_SYSCALL_64_fastpath+0x1a/0xa5
This was due to too large max_entries for the map such that we
surpassed the upper limit of PCPU_MIN_UNIT_SIZE. It's fine to
fail naturally here, so switch to __alloc_percpu_gfp() and pass
__GFP_NOWARN instead.
Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Shankara Pailoor <redacted>
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
On Tue, Oct 17, 2017 at 04:55:54PM +0200, Daniel Borkmann wrote:
PCPU_MIN_UNIT_SIZE is an implementation detail of the percpu
allocator. Given we support __GFP_NOWARN now, lets just let
the allocation request fail naturally instead. The two call
sites from BPF mistakenly assumed __GFP_NOWARN would work, so
no changes needed to their actual __alloc_percpu_gfp() calls
which use the flag already.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-10-17 16:28:55
On 10/17/2017 07:55 AM, Daniel Borkmann wrote:
It was reported that syzkaller was able to trigger a splat on
devmap percpu allocation due to illegal/unsupported allocation
request size passed to __alloc_percpu():
[ 70.094249] illegal size (32776) or align (8) for percpu allocation
[ 70.094256] ------------[ cut here ]------------
[ 70.094259] WARNING: CPU: 3 PID: 3451 at mm/percpu.c:1365 pcpu_alloc+0x96/0x630
[...]
[ 70.094325] Call Trace:
[ 70.094328] __alloc_percpu_gfp+0x12/0x20
[ 70.094330] dev_map_alloc+0x134/0x1e0
[ 70.094331] SyS_bpf+0x9bc/0x1610
[ 70.094333] ? selinux_task_setrlimit+0x5a/0x60
[ 70.094334] ? security_task_setrlimit+0x43/0x60
[ 70.094336] entry_SYSCALL_64_fastpath+0x1a/0xa5
This was due to too large max_entries for the map such that we
surpassed the upper limit of PCPU_MIN_UNIT_SIZE. It's fine to
fail naturally here, so switch to __alloc_percpu_gfp() and pass
__GFP_NOWARN instead.
Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
Reported-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Shankara Pailoor <redacted>
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
---
Thanks!
Acked-by: John Fastabend <john.fastabend@gmail.com>
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-10-17 16:29:44
On 10/17/2017 07:55 AM, Daniel Borkmann wrote:
PCPU_MIN_UNIT_SIZE is an implementation detail of the percpu
allocator. Given we support __GFP_NOWARN now, lets just let
the allocation request fail naturally instead. The two call
sites from BPF mistakenly assumed __GFP_NOWARN would work, so
no changes needed to their actual __alloc_percpu_gfp() calls
which use the flag already.
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
Nice cleanup. Thanks!
Acked-by: John Fastabend <john.fastabend@gmail.com>
Hello, Daniel.
(cc'ing Dennis)
On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Thanks!
Daniel Borkmann (3):
mm, percpu: add support for __GFP_NOWARN flag
This looks fine.
bpf: fix splat for illegal devmap percpu allocation
bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
These look okay too but if it helps percpu allocator can expose the
maximum size / alignment supported to take out the guessing game too.
Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
nobody needed anything bigger. Increasing the size doesn't really
cost much at least on 64bit archs. Is that something we want to be
considering?
Thanks.
--
tejun
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-18 14:04:14
On 10/18/2017 03:25 PM, Tejun Heo wrote:
Hello, Daniel.
(cc'ing Dennis)
On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
quoted
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Thanks!
Daniel Borkmann (3):
mm, percpu: add support for __GFP_NOWARN flag
This looks fine.
Great, thanks!
quoted
bpf: fix splat for illegal devmap percpu allocation
bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
These look okay too but if it helps percpu allocator can expose the
maximum size / alignment supported to take out the guessing game too.
At least from BPF side there's right now no infra for exposing
max possible alloc sizes for maps to e.g. user space as indication.
There are few users left in the tree, where it would make sense for
having some helpers though:
arch/tile/kernel/setup.c:729: if (size < PCPU_MIN_UNIT_SIZE)
arch/tile/kernel/setup.c:730: size = PCPU_MIN_UNIT_SIZE;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:346: unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:352: /* make sure per cpu pool fits into PCPU_MIN_UNIT_SIZE */
drivers/scsi/libfc/fc_exch.c:2488: /* reduce range so per cpu pool fits into PCPU_MIN_UNIT_SIZE pool */
drivers/scsi/libfc/fc_exch.c:2489: pool_exch_range = (PCPU_MIN_UNIT_SIZE - sizeof(*pool)) /
Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
nobody needed anything bigger. Increasing the size doesn't really
cost much at least on 64bit archs. Is that something we want to be
considering?
For devmap (and cpumap) itself it wouldn't make sense. For per-cpu
hashtable we could indeed consider it in the future.
Thanks,
Daniel
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-18 14:22:25
On 10/18/2017 04:03 PM, Daniel Borkmann wrote:
On 10/18/2017 03:25 PM, Tejun Heo wrote:
quoted
Hello, Daniel.
(cc'ing Dennis)
On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
quoted
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Thanks!
Daniel Borkmann (3):
mm, percpu: add support for __GFP_NOWARN flag
This looks fine.
Great, thanks!
quoted
quoted
bpf: fix splat for illegal devmap percpu allocation
bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
These look okay too but if it helps percpu allocator can expose the
maximum size / alignment supported to take out the guessing game too.
At least from BPF side there's right now no infra for exposing
max possible alloc sizes for maps to e.g. user space as indication.
There are few users left in the tree, where it would make sense for
having some helpers though:
arch/tile/kernel/setup.c:729: if (size < PCPU_MIN_UNIT_SIZE)
arch/tile/kernel/setup.c:730: size = PCPU_MIN_UNIT_SIZE;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:346: unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:352: /* make sure per cpu pool fits into PCPU_MIN_UNIT_SIZE */
drivers/scsi/libfc/fc_exch.c:2488: /* reduce range so per cpu pool fits into PCPU_MIN_UNIT_SIZE pool */
drivers/scsi/libfc/fc_exch.c:2489: pool_exch_range = (PCPU_MIN_UNIT_SIZE - sizeof(*pool)) /
quoted
Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
nobody needed anything bigger. Increasing the size doesn't really
cost much at least on 64bit archs. Is that something we want to be
considering?
For devmap (and cpumap) itself it wouldn't make sense. For per-cpu
hashtable we could indeed consider it in the future.
Higher prio imo would be to make the allocation itself faster
though, I remember we talked about this back in May wrt hashtable,
but I kind of lost track whether there was an update on this in
the mean time. ;-)
Cheers,
Daniel
On Wed, Oct 18, 2017 at 7:22 AM, Daniel Borkmann [off-list ref] wrote:
Higher prio imo would be to make the allocation itself faster
though, I remember we talked about this back in May wrt hashtable,
but I kind of lost track whether there was an update on this in
the mean time. ;-)
new percpu allocator by Dennis fixed those issues. It's in 4.14
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-10-18 15:31:48
On 10/18/2017 05:28 PM, Alexei Starovoitov wrote:
On Wed, Oct 18, 2017 at 7:22 AM, Daniel Borkmann [off-list ref] wrote:
quoted
Higher prio imo would be to make the allocation itself faster
though, I remember we talked about this back in May wrt hashtable,
but I kind of lost track whether there was an update on this in
the mean time. ;-)
new percpu allocator by Dennis fixed those issues. It's in 4.14
From: Dennis Zhou <hidden> Date: 2017-10-18 21:45:16
Hi Daniel and Tejun,
On Wed, Oct 18, 2017 at 06:25:26AM -0700, Tejun Heo wrote:
quoted
Daniel Borkmann (3):
mm, percpu: add support for __GFP_NOWARN flag
This looks fine.
Looks good to me too.
quoted
bpf: fix splat for illegal devmap percpu allocation
bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
These look okay too but if it helps percpu allocator can expose the
maximum size / alignment supported to take out the guessing game too.
I can add this once we've addressed the below if we want to.
Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
nobody needed anything bigger. Increasing the size doesn't really
cost much at least on 64bit archs. Is that something we want to be
considering?
I'm not sure I see the reason we can't match the minimum allocation size
with the unit size? It seems weird to arbitrate the maximum allocation
size given a lower bound on the unit size.
Thanks,
Dennis
From: David Miller <davem@davemloft.net> Date: 2017-10-19 12:15:06
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue, 17 Oct 2017 16:55:51 +0200
The set fixes a splat in devmap percpu allocation when we alloc
the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
patch 1 is rather small, so if this could be routed via -net, for
example, with Tejun's Ack that would be good. Patch 3 gets rid of
remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
internals and should not be used.
Hello,
On Wed, Oct 18, 2017 at 04:45:08PM -0500, Dennis Zhou wrote:
I'm not sure I see the reason we can't match the minimum allocation size
with the unit size? It seems weird to arbitrate the maximum allocation
size given a lower bound on the unit size.
idk, it can be weird for the maximum allowed allocation size varying
widely depending on how the machine boots up.
Thanks.
--
tejun