On 10/05/2017 05:34 PM, Jakub Kicinski wrote:
Verifier log buffer can be quite large (up to 16MB currently).
As Eric Dumazet points out if we allow multiple verification
requests to proceed simultaneously, malicious user may use the
verifier as a way of allocating large amounts of unswappable
memory to OOM the host.
Switch to a strategy of allocating a smaller buffer (a page)
and writing it out into the user buffer whenever it fills up.
To simplify the code assume that prints will never be longer
than 1024 bytes.
This is in preparation of the global verifier lock removal.
Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
Reviewed-by: Simon Horman <simon.hor...@netronome.com>
Set looks good in general, thanks for working on this! Just two
comments further below.
---
include/linux/bpf_verifier.h | 7 +++--
kernel/bpf/verifier.c | 64 +++++++++++++++++++++++++++++++-------------
2 files changed, 50 insertions(+), 21 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 598802dd1897..c0f0e210c3f8 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -140,10 +140,13 @@ struct bpf_verifier_env {
bool seen_direct_write;
struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
- u32 log_level;
+ char __user *log_ubuf;
+ u32 log_usize;
+ u32 log_ulen;
+ char *log_buf;
u32 log_size;
u32 log_len;
- char *log_buf;
+ u32 log_level;
Small request: given we'd now have log_{level,ubuf,usize,ulen,buf,size,len}
in struct bpf_verifier_env, could we abstract that a bit e.g. into something
like struct bpf_verifier_log, which has level and kbuf and ubuf as members
of which {k,u}buf would be something like struct bpf_verifier_buf with three
members (mem or buf, len_total, len_used) or such. I think most of patch 1
is on passing env into verbose, so likely wouldn't be too much change required
for this, but would be nice to make that a bit more structured if we need to
touch it anyway.
};
[...]
ret = -ENOMEM;
- env->log_buf = vmalloc(env->log_size);
+ env->log_buf = page_address(alloc_page(GFP_USER));
alloc_page() can return NULL, if I spot this correctly, then page_address()
cannot handle NULL and would try to deref it, no? Am I missing something?
if (!env->log_buf)
goto err_unlock;
+ env->log_size = PAGE_SIZE;
}
[...]
Thanks,
Daniel
