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]

Date:   Fri, 20 Apr 2018 11:47:11 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Sebastiano Miano <sebastiano.miano@...ito.it>,
        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>, brouer@...hat.com
Subject: Re: [bpf-next PATCH 3/3] bpf: add sample program to trace map
 events

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.  

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...

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer