netdev - Re: [bpf-next PATCH 3/3] bpf: add sample program to trace map events

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <062780b1-6a24-a7f3-175c-db3b02605850@polito.it>
Date:   Mon, 23 Apr 2018 16:08:36 +0200
From:   Sebastiano Miano <sebastiano.miano@...ito.it>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        mingo@...hat.com, rostedt@...dmis.org, fulvio.risso@...ito.it,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [bpf-next PATCH 3/3] bpf: add sample program to trace map events

On 20/04/2018 11:47, Jesper Dangaard Brouer wrote:
> On Thu, 19 Apr 2018 17:27:37 -0700
> Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
> 
>> On Wed, Apr 18, 2018 at 05:30:59PM +0200, Sebastiano Miano wrote:
>>> This patch adds a sample program, called trace_map_events,
>>> that shows how to capture map events and filter them based on
>>> the map id.
>> ...
>>> +struct bpf_map_keyval_ctx {
>>> +	u64 pad;		// First 8 bytes are not accessible by bpf code
>>> +	u32 type;		// offset:8;	size:4;	signed:0;
>>> +	u32 key_len;		// offset:12;	size:4;	signed:0;
>>> +	u32 key;		// offset:16;	size:4;	signed:0;
>>> +	bool key_trunc;		// offset:20;	size:1;	signed:0;
>>> +	u32 val_len;		// offset:24;	size:4;	signed:0;
>>> +	u32 val;		// offset:28;	size:4;	signed:0;
>>> +	bool val_trunc;		// offset:32;	size:1;	signed:0;
>>> +	int ufd;		// offset:36;	size:4;	signed:1;
>>> +	u32 id;			// offset:40;	size:4;	signed:0;
>>> +};
>>> +
>>> +SEC("tracepoint/bpf/bpf_map_lookup_elem")
>>> +int trace_bpf_map_lookup_elem(struct bpf_map_keyval_ctx *ctx)
>>> +{
>>> +	struct map_event_data data;
>>> +	int cpu = bpf_get_smp_processor_id();
>>> +	bool *filter;
>>> +	u32 key = 0, map_id = ctx->id;
>>> +
>>> +	filter = bpf_map_lookup_elem(&filter_events, &key);
>>> +	if (!filter)
>>> +		return 1;
>>> +
>>> +	if (!*filter)
>>> +		goto send_event;
>>> +
>>> +	/*
>>> +	 * If the map_id is not in the list of filtered
>>> +	 * ids we immediately return
>>> +	 */
>>> +	if (!bpf_map_lookup_elem(&filtered_ids, &map_id))
>>> +		return 0;
>>> +
>>> +send_event:
>>> +	data.map_id = map_id;
>>> +	data.evnt_type = MAP_LOOKUP;
>>> +	data.map_type = ctx->type;
>>> +
>>> +	bpf_perf_event_output(ctx, &map_event_trace, cpu, &data, sizeof(data));
>>> +	return 0;
>>> +}
>>
>> looks like the purpose of the series is to create map notify mechanism
>> so some external user space daemon can snoop all bpf map operations
>> that all other processes and bpf programs are doing.
>> I think it would be way better to create a proper mechanism for that
>> with permissions.
>>
>> tracepoints in the bpf core were introduced as introspection mechanism
>> for debugging. Right now we have better ways to do introspection
>> with ids, queries, etc that bpftool is using, so original purpose of
>> those tracepoints is gone and they actually rot.
>> Let's not repurpose them into this map notify logic.
>> I don't want tracepoints in the bpf core to become a stable interface
>> it will stiffen the development.
> 
> Well, I need it exactly for introspection and debugging, and just need
> the missing ID (as it was introduced later).
> 
> Can we just drop the sample program then? I would likely not use the
> sample program, because it is missing the PID+comm-name.

I agree with Jesper. We could drop this sample program but, do you think 
it is worth spending some time to create another sample program that 
shows the correct or suggested way to do it?

I believe it would be useful to have a program for this since I found it 
difficult to understand how to do it with the current 
documentation/samples and it seems I was not the only one hitting this 
problem.

> 
> For my use, I can simply use perf record to debug what programs are
> changing the map ID:
> 
>    perf record -e bpf:bpf_map_* -a --filter 'id == 2'
> 
> This should be a fairly common troubleshooting step.  I want to
> figure-out if anybody else (another userspace program) is changing my
> map. This can easily be caused by two userspace control programs
> running at the same time. Sysadm/scripts made a mistake and started two
> instances. Without the map ID, I cannot know what map perf is talking
> about...
> 

That's in fact the real use case for the first two patches. Since bpf 
tracepoints are still a rather common (and easy to use) troubleshooting 
and monitoring tool why shouldn't we "enhance" their support with the 
newly added map/prog IDs?