On 10/7/19 9:32 AM, Alan Maguire wrote:
> This is an incredible leap forward! One question I have relates to
> another aspect of checking. As we move from bpf_probe_read() to "direct
> struct access", should we have the verifier insist on the same sort of
> checking we have for direct packet access? Specifically I'm thinking of
> the case where a typed pointer argument might be NULL and we attempt to
> dereference it. This might be as simple as adding
> PTR_TO_BTF_ID to the reg_type_may_be_null() check:
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0717aac..6559b4d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -342,7 +342,8 @@ static bool reg_type_may_be_null(enum bpf_reg_type
> type)
> return type == PTR_TO_MAP_VALUE_OR_NULL ||
> type == PTR_TO_SOCKET_OR_NULL ||
> type == PTR_TO_SOCK_COMMON_OR_NULL ||
> - type == PTR_TO_TCP_SOCK_OR_NULL;
> + type == PTR_TO_TCP_SOCK_OR_NULL ||
> + type == PTR_TO_BTF_ID;
> }
>
> ...in order to ensure we don't dereference the pointer before checking for
> NULL. Possibly I'm missing something that will do that NULL checking
> already?
well, it's not as simple as above ;) but the point is valid.
Yes. It's definitely possible to enforce NULL check for every step
of btf pointer walking.
The thing is that in bpf tracing all scripts are using bpf_probe_read
and walk the pointers without checking error code.
In most cases the people who write those scripts know what they're
walking and know that the pointers will be valid.
Take execsnoop.py from bcc/tools. It's doing:
task->real_parent->tgid;
Every arrow bcc is magically replacing with bpf_probe_read.
I believe 'real_parent' is always valid, so above should
return expected data all the time.
But even if the pointer is not valid the cost of checking it
in the program is not worth it. All accesses are probe_read-ed.
If we make verifier forcing users to check every pointer for NULL
the bpf C code will look very ugly.
And not only ugly, but slow. Code size will increase, etc.
Long term we're thinking to add try/catch-like builtins.
So instead of doing
__builtin_preserve_access_index(({
dev = skb->dev;
ifindex = dev->ifindex;
}));
like the test from patch 10 is doing.
We will be able to write BPF program like:
__builtin_try(({
dev = skb->dev;
ifindex = dev->ifindex;
}), ({
// handle page fault
bpf_printk("skb is NULL or skb->dev is NULL! sucks");
}));
On the kernel side it will be supported through extable mechanism.
Once we realized that C language can be improved we started doing so :)
