Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Lorenzo Bianconi <lorenzo@kernel.org>,
	netdev@vger.kernel.org, bpf@vger.kernel.org
Cc: davem@davemloft.net, ast@kernel.org, brouer@redhat.com,
	toke@redhat.com, lorenzo.bianconi@redhat.com, dsahern@kernel.org,
	andrii.nakryiko@gmail.com
Subject: Re: [PATCH v5 bpf-next 5/9] bpf: cpumap: add the possibility to attach an eBPF program to cpumap
Date: Thu, 2 Jul 2020 01:39:40 +0200
Message-ID: <1f4af1f3-10cf-57ca-4171-11d3bff51c99@iogearbox.net> (raw)
In-Reply-To: <a6bb83a429f3b073e97f81ec3935b8ebe89fbd71.1593521030.git.lorenzo@kernel.org>

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

  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 12:49 [PATCH v5 bpf-next 0/9] introduce support for XDP programs in CPUMAP Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 1/9] cpumap: use non-locked version __ptr_ring_consume_batched Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 2/9] net: refactor xdp_convert_buff_to_frame Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 3/9] samples/bpf: xdp_redirect_cpu_user: do not update bpf maps in option loop Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 4/9] cpumap: formalize map value as a named struct Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 5/9] bpf: cpumap: add the possibility to attach an eBPF program to cpumap Lorenzo Bianconi
2020-07-01 23:02   ` Daniel Borkmann
2020-07-03 15:46     ` Lorenzo Bianconi
2020-07-01 23:39   ` Daniel Borkmann [this message]
2020-07-03 20:48     ` Lorenzo Bianconi
2020-07-03 22:24       ` Daniel Borkmann
2020-06-30 12:49 ` [PATCH v5 bpf-next 6/9] bpf: cpumap: implement XDP_REDIRECT for eBPF programs attached to map entries Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 7/9] libbpf: add SEC name for xdp programs attached to CPUMAP Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 8/9] samples/bpf: xdp_redirect_cpu: load a eBPF program on cpumap Lorenzo Bianconi
2020-06-30 12:49 ` [PATCH v5 bpf-next 9/9] selftest: add tests for XDP programs in CPUMAP entries Lorenzo Bianconi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1f4af1f3-10cf-57ca-4171-11d3bff51c99@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=lorenzo.bianconi@redhat.com \
    --cc=lorenzo@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=toke@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git