lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 19 Dec 2018 19:02:47 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>
Cc:     netdev@...r.kernel.org, oss-drivers@...ronome.com,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next 6/8] tools: bpftool: add probes for eBPF helper
 functions

2018-12-18 01:42 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.net>
> On 12/17/2018 11:18 AM, Quentin Monnet wrote:
>> 2018-12-16 00:57 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.net>
>>> On 12/15/2018 04:32 AM, Quentin Monnet wrote:
>>>> 2018-12-15 01:08 UTC+0100 ~ Daniel Borkmann <daniel@...earbox.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.monnet@...ronome.com>
>>>>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ