Re: [net-next V4 PATCH 1/5] bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Fri, 06 Oct 2017 16:52:40 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 10/06/2017 12:50 PM, Jesper Dangaard Brouer wrote:
> > On Thu, 05 Oct 2017 11:40:15 +0200
> > Daniel Borkmann <daniel@iogearbox.net> wrote:  
> >> On 10/04/2017 02:03 PM, Jesper Dangaard Brouer wrote:
> >> [...]  
> [...]
> >>> +		/* Updating qsize cause re-allocation of bpf_cpu_map_entry */
> >>> +		rcpu = __cpu_map_entry_alloc(qsize, key_cpu, map->id);
> >>> +		if (!rcpu)
> >>> +			return -ENOMEM;
> >>> +	}
> >>> +	rcu_read_lock();
> >>> +	__cpu_map_entry_replace(cmap, key_cpu, rcpu);
> >>> +	rcu_read_unlock();
> >>> +	return 0;  
> >>
> >> You need to update verifier such that this function cannot be called
> >> out of an BPF program,  
> >
> > In the example BPF program, I do a lookup into the map, but only to
> > verify that an entry exist (I don't look at the value).  I would like
> > to support such usage.  
> 
> Ok, put comment below.
> 
> >> otherwise it would be possible under full RCU
> >> read context, which is explicitly avoided here and also it would otherwise
> >> be allowed for other maps of different type as well, which needs to
> >> be avoided.  
> >
> > Sorry, I don't follow this.  
> 
> What I meant is that check_map_func_compatibility() should check for
> BPF_MAP_TYPE_CPUMAP and only allow func_id of BPF_FUNC_redirect_map
> and BPF_FUNC_map_lookup_elem to be used, which I haven't seen the set
> restricting it to. Some of your later patches do this for the helper
> BPF_FUNC_redirect_map but the important point is that map updates
> wouldn't be done out of the BPF program itself, but rather from user
> space control path given they can't be done under full RCU read lock
> context if I read this correctly (which the programs run in though).

Okay, I choose to restrict bpf_prog side in check_map_func_compatibility()
as you describe.  And I changed the user program to keep track of valid
entries via secondary map.  We can always add/allow lookup later if users
request this.

I'll send out V5 shortly...

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