Re: [PATCH v5 bpf-next 5/9] bpf: cpumap: add the possibility to attach an eBPF program to cpumap

[Daniel Borkmann <daniel@xxxxxxxxxxxxx>]

On 6/30/20 2:49 PM, Lorenzo Bianconi wrote:
[...]
> +static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
> +				    void **frames, int n,
> +				    struct xdp_cpumap_stats *stats)
> +{
> +	struct xdp_rxq_info rxq;
> +	struct bpf_prog *prog;
> +	struct xdp_buff xdp;
> +	int i, nframes = 0;
> +
> +	if (!rcpu->prog)
> +		return n;
> +
> +	rcu_read_lock();
> +
> +	xdp_set_return_frame_no_direct();
> +	xdp.rxq = &rxq;
> +
> +	prog = READ_ONCE(rcpu->prog);
> +	for (i = 0; i < n; i++) {
> +		struct xdp_frame *xdpf = frames[i];
> +		u32 act;
> +		int err;
> +
> +		rxq.dev = xdpf->dev_rx;
> +		rxq.mem = xdpf->mem;
> +		/* TODO: report queue_index to xdp_rxq_info */
> +
> +		xdp_convert_frame_to_buff(xdpf, &xdp);
> +
> +		act = bpf_prog_run_xdp(prog, &xdp);
> +		switch (act) {
> +		case XDP_PASS:
> +			err = xdp_update_frame_from_buff(&xdp, xdpf);
> +			if (err < 0) {
> +				xdp_return_frame(xdpf);
> +				stats->drop++;
> +			} else {
> +				frames[nframes++] = xdpf;
> +				stats->pass++;
> +			}
> +			break;
> +		default:
> +			bpf_warn_invalid_xdp_action(act);
> +			/* fallthrough */
> +		case XDP_DROP:
> +			xdp_return_frame(xdpf);
> +			stats->drop++;
> +			break;
> +		}
> +	}
> +
> +	xdp_clear_return_frame_no_direct();
> +
> +	rcu_read_unlock();
> +
> +	return nframes;
> +}
> +
>   #define CPUMAP_BATCH 8
>   
>   static int cpu_map_kthread_run(void *data)
> @@ -235,11 +297,12 @@ static int cpu_map_kthread_run(void *data)
>   	 * kthread_stop signal until queue is empty.
>   	 */
>   	while (!kthread_should_stop() || !__ptr_ring_empty(rcpu->queue)) {
> +		struct xdp_cpumap_stats stats = {}; /* zero stats */
> +		gfp_t gfp = __GFP_ZERO | GFP_ATOMIC;
>   		unsigned int drops = 0, sched = 0;
>   		void *frames[CPUMAP_BATCH];
>   		void *skbs[CPUMAP_BATCH];
> -		gfp_t gfp = __GFP_ZERO | GFP_ATOMIC;
> -		int i, n, m;
> +		int i, n, m, nframes;
>   
>   		/* Release CPU reschedule checks */
>   		if (__ptr_ring_empty(rcpu->queue)) {
> @@ -260,8 +323,8 @@ static int cpu_map_kthread_run(void *data)
>   		 * kthread CPU pinned. Lockless access to ptr_ring
>   		 * consume side valid as no-resize allowed of queue.
>   		 */
> -		n = __ptr_ring_consume_batched(rcpu->queue, frames, CPUMAP_BATCH);
> -
> +		n = __ptr_ring_consume_batched(rcpu->queue, frames,
> +					       CPUMAP_BATCH);
>   		for (i = 0; i < n; i++) {
>   			void *f = frames[i];
>   			struct page *page = virt_to_page(f);
> @@ -273,15 +336,19 @@ static int cpu_map_kthread_run(void *data)
>   			prefetchw(page);
>   		}
>   
> -		m = kmem_cache_alloc_bulk(skbuff_head_cache, gfp, n, skbs);
> -		if (unlikely(m == 0)) {
> -			for (i = 0; i < n; i++)
> -				skbs[i] = NULL; /* effect: xdp_return_frame */
> -			drops = n;
> +		/* Support running another XDP prog on this CPU */
> +		nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, n, &stats);
> +		if (nframes) {
> +			m = kmem_cache_alloc_bulk(skbuff_head_cache, gfp, nframes, skbs);
> +			if (unlikely(m == 0)) {
> +				for (i = 0; i < nframes; i++)
> +					skbs[i] = NULL; /* effect: xdp_return_frame */
> +				drops += nframes;
> +			}
>   		}
>   
>   		local_bh_disable();
> -		for (i = 0; i < n; i++) {
> +		for (i = 0; i < nframes; i++) {
>   			struct xdp_frame *xdpf = frames[i];
>   			struct sk_buff *skb = skbs[i];
>   			int ret;
> @@ -298,7 +365,7 @@ static int cpu_map_kthread_run(void *data)
>   				drops++;
>   		}
>   		/* Feedback loop via tracepoint */
> -		trace_xdp_cpumap_kthread(rcpu->map_id, n, drops, sched);
> +		trace_xdp_cpumap_kthread(rcpu->map_id, n, drops, sched, &stats);
>   
>   		local_bh_enable(); /* resched point, may call do_softirq() */
>   	}
> @@ -308,13 +375,38 @@ static int cpu_map_kthread_run(void *data)
>   	return 0;
>   }
[...]
> @@ -415,6 +510,8 @@ static void __cpu_map_entry_replace(struct bpf_cpu_map *cmap,
>   
>   	old_rcpu = xchg(&cmap->cpu_map[key_cpu], rcpu);
>   	if (old_rcpu) {
> +		if (old_rcpu->prog)
> +			bpf_prog_put(old_rcpu->prog);
>   		call_rcu(&old_rcpu->rcu, __cpu_map_entry_free);
>   		INIT_WORK(&old_rcpu->kthread_stop_wq, cpu_map_kthread_stop);
>   		schedule_work(&old_rcpu->kthread_stop_wq);

Hm, not quite sure I follow the logic here. Why is the bpf_prog_put() not placed inside
__cpu_map_entry_free(), for example? Wouldn't this at least leave a potential small race
window of UAF given the rest is still live? If we already piggy-back from RCU side on
rcpu entry, why not having it in __cpu_map_entry_free()?

Thanks,
Daniel