2018-12-18 01:42 UTC+0100 ~ Daniel Borkmann <dan...@iogearbox.net>
> On 12/17/2018 11:18 AM, Quentin Monnet wrote:
>> 2018-12-16 00:57 UTC+0100 ~ Daniel Borkmann <dan...@iogearbox.net>
>>> On 12/15/2018 04:32 AM, Quentin Monnet wrote:
>>>> 2018-12-15 01:08 UTC+0100 ~ Daniel Borkmann <dan...@iogearbox.net>
>>>>> On 12/13/2018 01:19 PM, Quentin Monnet wrote:
>>>>>> Similarly to what was done for program types and map types, add a set of
>>>>>> probes to test the availability of the different eBPF helper functions
>>>>>> on the current system.
>>>>>>
>>>>>> Sample output:
>>>>>>
>>>>>> # bpftool feature probe kernel
>>>>>> ...
>>>>>> Scanning eBPF helper functions...
>>>>>> eBPF helper bpf_map_lookup_elem is available
>>>>>> eBPF helper bpf_map_update_elem is available
>>>>>> eBPF helper bpf_map_delete_elem is available
>>>>>> ...
>>>>>>
>>>>>> # bpftool --json --pretty feature probe kernel
>>>>>> {
>>>>>> ...
>>>>>> "helpers": {
>>>>>> "have_bpf_map_lookup_elem_helper": true,
>>>>>> "have_bpf_map_update_elem_helper": true,
>>>>>> "have_bpf_map_delete_elem_helper": true,
>>>>>> ...
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> # bpftool feature probe kernel macros prefix BPFTOOL_
>>>>>> ...
>>>>>> /*** eBPF helper functions ***/
>>>>>> #define BPFTOOL_BPF_MAP_LOOKUP_ELEM_HELPER
>>>>>> #define BPFTOOL_BPF_MAP_UPDATE_ELEM_HELPER
>>>>>> #define BPFTOOL_BPF_MAP_DELETE_ELEM_HELPER
>>>>>
>>>>> Small nit: instead of BPFTOOL_ prefix, would it make sense
>>>>> to generally use Have_ prefix (similar to json output). The
>>>>> former seems somewhat bpftool related though it solely probes
>>>>> the underlying kernel.
>>>>
>>>> "BPFTOOL_" is provided on the command line ("prefix BPFTOOL_"), and it's
>>>> here just for the example. I initially had a non-configurable "HAVE_"
>>>> prefix instead, but after discussing it with Jakub we thought it best to
>>>> leave the choice of the prefix to the users, so they can choose what
>>>> works best for them, or adapt it and avoid any potential name conflict.
>>>
>>> Ah, good point, makes sense then. As default you have no prefix? Perhaps
>>> there it could be "HAVE_" prefix.
>>
>> Yes, I can probably reinstate it as a default option if none was provided
>> from the command line, for prog/map types and helpers.
>>
>>>>>> Signed-off-by: Quentin Monnet <quentin.mon...@netronome.com>
>>>>>> Reviewed-by: Jakub Kicinski <jakub.kicin...@netronome.com>
>>>>>> ---
>>>>>> .../bpftool/Documentation/bpftool-feature.rst | 4 +
>>>>>> tools/bpf/bpftool/feature.c | 152 ++++++++++++++++++
>>>>>> 2 files changed, 156 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
>>>>>> b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
>>>>>> index 23920a7490e9..083d30510cce 100644
>>>>>> --- a/tools/bpf/bpftool/Documentation/bpftool-feature.rst
>>>>>> +++ b/tools/bpf/bpftool/Documentation/bpftool-feature.rst
>>>>>> @@ -39,6 +39,10 @@ DESCRIPTION
>>>>>> names when including the output of this command as a header
>>>>>> file.
>>>>>> + Note that when probed, some eBPF helpers (e.g.
>>>>>> + **bpf_trace_printk**\ () or **bpf_probe_write_user**\ ()) may
>>>>>> + print warnings to kernel logs.
>>>>>> +
>>>>>> **bpftool feature help**
>>>>>> Print short help message.
>>>>>> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
>>>>>> index 85928f172413..77221fff6ba9 100644
>>>>>> --- a/tools/bpf/bpftool/feature.c
>>>>>> +++ b/tools/bpf/bpftool/feature.c
>>>>>> @@ -24,6 +24,113 @@ enum probe_component {
>>>>>> COMPONENT_KERNEL,
>>>>>> };
>>>>>> +#define MAX_HELPER_NAME_LEN 32
>>>>>> +struct helper_param {
>>>>>> + enum bpf_prog_type progtype;
>>>>>> + const char name[MAX_HELPER_NAME_LEN];
>>>>>> +};
>>>>>> +
>>>>>> +/* helper_progtype_and_name[index] associates to the BPF helper
>>>>>> function of id
>>>>>> + * "index" a name and a program type to run this helper with. In order
>>>>>> to probe
>>>>>> + * helper availability for programs offloaded to a network device, use
>>>>>> + * offload-compatible types (e.g. XDP) everywhere we can. Caveats:
>>>>>> helper
>>>>>> + * probing may fail currently if only TC (but not XDP) is supported for
>>>>>> + * offload.
>>>>>> + */
>>>>>> +static const struct helper_param helper_progtype_and_name[] = {
>>>>>> + { BPF_PROG_TYPE_XDP, "no_helper_with_id_0" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_lookup_elem" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_update_elem" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_delete_elem" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_probe_read" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_ktime_get_ns" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_trace_printk" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_get_prandom_u32" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_get_smp_processor_id" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_store_bytes" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_l3_csum_replace" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_l4_csum_replace" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_tail_call" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_clone_redirect" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_current_pid_tgid" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_current_uid_gid" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_current_comm" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_get_cgroup_classid" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_vlan_push" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_vlan_pop" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_get_tunnel_key" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_set_tunnel_key" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_perf_event_read" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_redirect" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_get_route_realm" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_perf_event_output" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_load_bytes" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_stackid" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_csum_diff" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_get_tunnel_opt" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_set_tunnel_opt" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_change_proto" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_change_type" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_under_cgroup" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_get_hash_recalc" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_current_task" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_probe_write_user" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_current_task_under_cgroup" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_change_tail" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_pull_data" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_csum_update" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_set_hash_invalid" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_get_numa_node_id" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_skb_change_head" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_xdp_adjust_head" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_probe_read_str" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_get_socket_cookie" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_get_socket_uid" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_set_hash" },
>>>>>> + { BPF_PROG_TYPE_SOCK_OPS, "bpf_setsockopt" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_adjust_room" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_redirect_map" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_sk_redirect_map" },
>>>>>> + { BPF_PROG_TYPE_SOCK_OPS, "bpf_sock_map_update" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_xdp_adjust_meta" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_perf_event_read_value" },
>>>>>> + { BPF_PROG_TYPE_PERF_EVENT, "bpf_perf_prog_read_value" },
>>>>>> + { BPF_PROG_TYPE_SOCK_OPS, "bpf_getsockopt" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_override_return" },
>>>>>> + { BPF_PROG_TYPE_SOCK_OPS, "bpf_sock_ops_cb_flags_set" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_redirect_map" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_apply_bytes" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_cork_bytes" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_pull_data" },
>>>>>> + { BPF_PROG_TYPE_CGROUP_SOCK_ADDR, "bpf_bind" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_xdp_adjust_tail" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_get_xfrm_state" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_stack" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_load_bytes_relative" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_fib_lookup" },
>>>>>> + { BPF_PROG_TYPE_SOCK_OPS, "bpf_sock_hash_update" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_redirect_hash" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_sk_redirect_hash" },
>>>>>> + { BPF_PROG_TYPE_LWT_IN, "bpf_lwt_push_encap" },
>>>>>> + { BPF_PROG_TYPE_LWT_SEG6LOCAL, "bpf_lwt_seg6_store_bytes" },
>>>>>> + { BPF_PROG_TYPE_LWT_SEG6LOCAL, "bpf_lwt_seg6_adjust_srh" },
>>>>>> + { BPF_PROG_TYPE_LWT_SEG6LOCAL, "bpf_lwt_seg6_action" },
>>>>>> + { BPF_PROG_TYPE_LIRC_MODE2, "bpf_rc_repeat" },
>>>>>> + { BPF_PROG_TYPE_LIRC_MODE2, "bpf_rc_keydown" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_cgroup_id" },
>>>>>> + { BPF_PROG_TYPE_KPROBE, "bpf_get_current_cgroup_id" },
>>>>>> + { BPF_PROG_TYPE_CGROUP_SKB, "bpf_get_local_storage" },
>>>>>> + { BPF_PROG_TYPE_SK_REUSEPORT, "bpf_sk_select_reuseport" },
>>>>>> + { BPF_PROG_TYPE_SCHED_CLS, "bpf_skb_ancestor_cgroup_id" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_sk_lookup_tcp" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_sk_lookup_udp" },
>>>>>> + { BPF_PROG_TYPE_SK_SKB, "bpf_sk_release" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_push_elem" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_pop_elem" },
>>>>>> + { BPF_PROG_TYPE_XDP, "bpf_map_peek_elem" },
>>>>>> + { BPF_PROG_TYPE_SK_MSG, "bpf_msg_push_data" },
>>>>>
>>>>> Some of these helpers like bpf_perf_event_output are available on
>>>>> multiple types; should they all be tested in case the one probed
>>>>> prog type would not be available on the underlying kernel?
>>>>>
>>>>> Thanks,
>>>>> Daniel
>>>>
>>>> That's a good point, but it will make things more complicated. We would
>>>> need a list of compatible program types for each helper, and then try
>>>> them all until one works. I can do that, but I fear this will get huge
>>>> and hard to maintain.
>>>
>>> Hmm, true, one way to overcome this maintenance burden would be to try to
>>> brute-force all helpers over all prog types but obviously from verifier
>>> side it's more expensive. Perhaps worth testing whether this overhead would
>>> be acceptable. On the upside you'd have a listing of all supported helpers
>>> for each prog type.
>>
>> Having the listing sounds nice. My concern is that the compatibility list
>> for a couple of helpers has been modified in the past (e.g. csum_diff added
>> to XDP), and I fear such changes will be easy to miss.
>>
>> Brute-forcing all programs might be doable (programs are only 2-insn long,
>> no jumps) but seems less clean. I can have a look at it. Is there any
>> particular metric I should focus on?
>>
>> Naive question, just in case: I don't suppose it is desirable to make the
>> list of supported helpers for each program type accessible to user space? (I
>> guess it would come down to implement those probes as a new bpf() command,
>> which has been rejected before if I remember correctly?)
>
> Yep, because what this basically is testing is verifier behavior in general
> where the helper aspect is one specific case of accept / reject, but there can
> be more subtle probes on verifier behavior that would go beyond just that like
> the ones that Cilium provides where we test verifier for register's map id
> marking, etc. Such kind of things are not feasible to expose in uapi in long
> term as "capability" but should be probed instead as you do here.
>
> Thanks,
> Daniel
>
Thanks for the clarification.
I've been working on my set to add probing with all supported program
types for each helper. I'm considering printing a list of compatible
program types (as supported by the system) for each helper, like this:
/*** eBPF helper functions ***/
...
#define BPF_SKB_CHANGE_HEAD_HELPER_COMPAT_LIST "" \
"lwt_xmit " \
"sk_skb "
#define BPF_XDP_ADJUST_HEAD_HELPER_COMPAT_LIST "" \
"xdp "
#define BPF_PROBE_READ_STR_HELPER_COMPAT_LIST "" \
"kprobe " \
"tracepoint " \
"perf_event " \
"raw_tracepoint "
...
or in JSON:
{
...
"helpers": {
...
"bpf_skb_change_head_compat_list": ["lwt_xmit","sk_skb"
],
"bpf_xdp_adjust_head_compat_list": ["xdp"
],
"bpf_probe_read_str_compat_list": \
["kprobe","tracepoint","perf_event","raw_tracepoint"
],
...
}
}
Would this be acceptable?
Thanks,
Quentin
