syzkaller was recently triggering an oversized kvmalloc() warning via
xdp_umem_create().
The triggered warning was added back in 7661809d493b ("mm: don't allow
oversized kvmalloc() calls"). The rationale for the warning for huge
kvmalloc sizes was as a reaction to a security bug where the size was
more than UINT_MAX but not everything was prepared to handle unsigned
long sizes.
Anyway, the AF_XDP related call trace from this syzkaller report was:
kvmalloc include/linux/mm.h:806 [inline]
kvmalloc_array include/linux/mm.h:824 [inline]
kvcalloc include/linux/mm.h:829 [inline]
xdp_umem_pin_pages net/xdp/xdp_umem.c:102 [inline]
xdp_umem_reg net/xdp/xdp_umem.c:219 [inline]
xdp_umem_create+0x6a5/0xf00 net/xdp/xdp_umem.c:252
xsk_setsockopt+0x604/0x790 net/xdp/xsk.c:1068
__sys_setsockopt+0x1fd/0x4e0 net/socket.c:2176
__do_sys_setsockopt net/socket.c:2187 [inline]
__se_sys_setsockopt net/socket.c:2184 [inline]
__x64_sys_setsockopt+0xb5/0x150 net/socket.c:2184
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
Björn mentioned that requests for >2GB allocation can still be valid:
The structure that is being allocated is the page-pinning accounting.
AF_XDP has an internal limit of U32_MAX pages, which is *a lot*, but
still fewer than what memcg allows (PAGE_COUNTER_MAX is a LONG_MAX/
PAGE_SIZE on 64 bit systems). [...]
I could just change from U32_MAX to INT_MAX, but as I stated earlier
that has a hacky feeling to it. [...] From my perspective, the code
isn't broken, with the memcg limits in consideration. [...]
Linus says:
[...] Pretty much every time this has come up, the kernel warning has
shown that yes, the code was broken and there really wasn't a reason
for doing allocations that big.
Of course, some people would be perfectly fine with the allocation
failing, they just don't want the warning. I didn't want __GFP_NOWARN
to shut it up originally because I wanted people to see all those
cases, but these days I think we can just say "yeah, people can shut
it up explicitly by saying 'go ahead and fail this allocation, don't
warn about it'".
So enough time has passed that by now I'd certainly be ok with [it].
Thus allow call-sites to silence such userspace triggered splats if the
allocation requests have __GFP_NOWARN. For xdp_umem_pin_pages()'s call
to kvcalloc() this is already the case, so nothing else needed there.
Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")
Reported-by: [email protected]
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Daniel Borkmann <[email protected]>
Tested-by: [email protected]
Cc: Björn Töpel <[email protected]>
Cc: Magnus Karlsson <[email protected]>
Cc: Willy Tarreau <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: David S. Miller <[email protected]>
Link: https://lore.kernel.org/bpf/CAJ+HfNhyfsT5cS_U9EC213ducHs9k9zNxX9+abqC0kTrPbQ0gg@mail.gmail.com
Link: https://lore.kernel.org/bpf/[email protected]
---
[ Hi Linus, just to follow-up on the discussion from here [0], I've cooked
up proper and tested patch. Feel free to take it directly to your tree if
you prefer, or we could also either route it via bpf or mm, whichever way
is best. Thanks!
[0] https://lore.kernel.org/bpf/CAHk-=wiRq+_jd_O1gz3J6-ANtXMY7iLpi8XFUcmtB3rBixvUXQ@mail.gmail.com/ ]
mm/util.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/util.c b/mm/util.c
index 7e43369064c8..d3102081add0 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -587,8 +587,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
return ret;
/* Don't even allow crazy sizes */
- if (WARN_ON_ONCE(size > INT_MAX))
+ if (unlikely(size > INT_MAX)) {
+ WARN_ON_ONCE(!(flags & __GFP_NOWARN));
return NULL;
+ }
return __vmalloc_node(size, 1, flags, node,
__builtin_return_address(0));
--
2.21.0
On Fri, Mar 04, 2022 at 03:26:32PM +0100, Daniel Borkmann wrote:
> syzkaller was recently triggering an oversized kvmalloc() warning via
> xdp_umem_create().
>
> The triggered warning was added back in 7661809d493b ("mm: don't allow
> oversized kvmalloc() calls"). The rationale for the warning for huge
> kvmalloc sizes was as a reaction to a security bug where the size was
> more than UINT_MAX but not everything was prepared to handle unsigned
> long sizes.
>
> Anyway, the AF_XDP related call trace from this syzkaller report was:
>
> kvmalloc include/linux/mm.h:806 [inline]
> kvmalloc_array include/linux/mm.h:824 [inline]
> kvcalloc include/linux/mm.h:829 [inline]
> xdp_umem_pin_pages net/xdp/xdp_umem.c:102 [inline]
> xdp_umem_reg net/xdp/xdp_umem.c:219 [inline]
> xdp_umem_create+0x6a5/0xf00 net/xdp/xdp_umem.c:252
> xsk_setsockopt+0x604/0x790 net/xdp/xsk.c:1068
> __sys_setsockopt+0x1fd/0x4e0 net/socket.c:2176
> __do_sys_setsockopt net/socket.c:2187 [inline]
> __se_sys_setsockopt net/socket.c:2184 [inline]
> __x64_sys_setsockopt+0xb5/0x150 net/socket.c:2184
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Bj?rn mentioned that requests for >2GB allocation can still be valid:
>
> The structure that is being allocated is the page-pinning accounting.
> AF_XDP has an internal limit of U32_MAX pages, which is *a lot*, but
> still fewer than what memcg allows (PAGE_COUNTER_MAX is a LONG_MAX/
> PAGE_SIZE on 64 bit systems). [...]
>
> I could just change from U32_MAX to INT_MAX, but as I stated earlier
> that has a hacky feeling to it. [...] From my perspective, the code
> isn't broken, with the memcg limits in consideration. [...]
>
> Linus says:
>
> [...] Pretty much every time this has come up, the kernel warning has
> shown that yes, the code was broken and there really wasn't a reason
> for doing allocations that big.
>
> Of course, some people would be perfectly fine with the allocation
> failing, they just don't want the warning. I didn't want __GFP_NOWARN
> to shut it up originally because I wanted people to see all those
> cases, but these days I think we can just say "yeah, people can shut
> it up explicitly by saying 'go ahead and fail this allocation, don't
> warn about it'".
>
> So enough time has passed that by now I'd certainly be ok with [it].
>
> Thus allow call-sites to silence such userspace triggered splats if the
> allocation requests have __GFP_NOWARN. For xdp_umem_pin_pages()'s call
> to kvcalloc() this is already the case, so nothing else needed there.
>
> Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")
> Reported-by: [email protected]
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Daniel Borkmann <[email protected]>
> Tested-by: [email protected]
> Cc: Bj?rn T?pel <[email protected]>
> Cc: Magnus Karlsson <[email protected]>
> Cc: Willy Tarreau <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: David S. Miller <[email protected]>
> Link: https://lore.kernel.org/bpf/CAJ+HfNhyfsT5cS_U9EC213ducHs9k9zNxX9+abqC0kTrPbQ0gg@mail.gmail.com
> Link: https://lore.kernel.org/bpf/[email protected]
> ---
> [ Hi Linus, just to follow-up on the discussion from here [0], I've cooked
> up proper and tested patch. Feel free to take it directly to your tree if
> you prefer, or we could also either route it via bpf or mm, whichever way
> is best. Thanks!
> [0] https://lore.kernel.org/bpf/CAHk-=wiRq+_jd_O1gz3J6-ANtXMY7iLpi8XFUcmtB3rBixvUXQ@mail.gmail.com/ ]
It will be great to see this patch applied directly to Linus's tree.
We (RDMA) have same false alarms [1].
[1] https://lore.kernel.org/linux-mm/YayptO82EvG3EwKA@unreal/
Thanks,
Reviewed-by: Leon Romanovsky <[email protected]>
On Fri, Mar 4, 2022 at 6:27 AM Daniel Borkmann <[email protected]> wrote:
>
> [ Hi Linus, just to follow-up on the discussion from here [0], I've cooked
> up proper and tested patch. Feel free to take it directly to your tree if
> you prefer, or we could also either route it via bpf or mm, whichever way
> is best. Thanks!
Applied.
Thanks,
Linus
On Fri 04-03-22 15:26:32, Daniel Borkmann wrote:
> syzkaller was recently triggering an oversized kvmalloc() warning via
> xdp_umem_create().
>
> The triggered warning was added back in 7661809d493b ("mm: don't allow
> oversized kvmalloc() calls"). The rationale for the warning for huge
> kvmalloc sizes was as a reaction to a security bug where the size was
> more than UINT_MAX but not everything was prepared to handle unsigned
> long sizes.
>
> Anyway, the AF_XDP related call trace from this syzkaller report was:
>
> kvmalloc include/linux/mm.h:806 [inline]
> kvmalloc_array include/linux/mm.h:824 [inline]
> kvcalloc include/linux/mm.h:829 [inline]
> xdp_umem_pin_pages net/xdp/xdp_umem.c:102 [inline]
> xdp_umem_reg net/xdp/xdp_umem.c:219 [inline]
> xdp_umem_create+0x6a5/0xf00 net/xdp/xdp_umem.c:252
> xsk_setsockopt+0x604/0x790 net/xdp/xsk.c:1068
> __sys_setsockopt+0x1fd/0x4e0 net/socket.c:2176
> __do_sys_setsockopt net/socket.c:2187 [inline]
> __se_sys_setsockopt net/socket.c:2184 [inline]
> __x64_sys_setsockopt+0xb5/0x150 net/socket.c:2184
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Bj?rn mentioned that requests for >2GB allocation can still be valid:
>
> The structure that is being allocated is the page-pinning accounting.
> AF_XDP has an internal limit of U32_MAX pages, which is *a lot*, but
> still fewer than what memcg allows (PAGE_COUNTER_MAX is a LONG_MAX/
> PAGE_SIZE on 64 bit systems). [...]
>
> I could just change from U32_MAX to INT_MAX, but as I stated earlier
> that has a hacky feeling to it. [...] From my perspective, the code
> isn't broken, with the memcg limits in consideration. [...]
>
> Linus says:
>
> [...] Pretty much every time this has come up, the kernel warning has
> shown that yes, the code was broken and there really wasn't a reason
> for doing allocations that big.
>
> Of course, some people would be perfectly fine with the allocation
> failing, they just don't want the warning. I didn't want __GFP_NOWARN
> to shut it up originally because I wanted people to see all those
> cases, but these days I think we can just say "yeah, people can shut
> it up explicitly by saying 'go ahead and fail this allocation, don't
> warn about it'".
>
> So enough time has passed that by now I'd certainly be ok with [it].
>
> Thus allow call-sites to silence such userspace triggered splats if the
> allocation requests have __GFP_NOWARN. For xdp_umem_pin_pages()'s call
> to kvcalloc() this is already the case, so nothing else needed there.
>
> Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")
> Reported-by: [email protected]
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: Daniel Borkmann <[email protected]>
> Tested-by: [email protected]
> Cc: Bj?rn T?pel <[email protected]>
> Cc: Magnus Karlsson <[email protected]>
> Cc: Willy Tarreau <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: David S. Miller <[email protected]>
> Link: https://lore.kernel.org/bpf/CAJ+HfNhyfsT5cS_U9EC213ducHs9k9zNxX9+abqC0kTrPbQ0gg@mail.gmail.com
> Link: https://lore.kernel.org/bpf/[email protected]
This makes sense to me.
Ackd-by: Michal Hocko <[email protected]>
> ---
> [ Hi Linus, just to follow-up on the discussion from here [0], I've cooked
> up proper and tested patch. Feel free to take it directly to your tree if
> you prefer, or we could also either route it via bpf or mm, whichever way
> is best. Thanks!
> [0] https://lore.kernel.org/bpf/CAHk-=wiRq+_jd_O1gz3J6-ANtXMY7iLpi8XFUcmtB3rBixvUXQ@mail.gmail.com/ ]
>
> mm/util.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 7e43369064c8..d3102081add0 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -587,8 +587,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
> return ret;
>
> /* Don't even allow crazy sizes */
> - if (WARN_ON_ONCE(size > INT_MAX))
> + if (unlikely(size > INT_MAX)) {
> + WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> return NULL;
> + }
>
> return __vmalloc_node(size, 1, flags, node,
> __builtin_return_address(0));
> --
> 2.21.0
--
Michal Hocko
SUSE Labs