netdev - Re: [PATCH v4 bpf-next 6/9] bpf: cpumap: implement XDP_REDIRECT for eBPF programs attached to map entries

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: <20200626111850.3ccfa8ac@carbon>
Date:   Fri, 26 Jun 2020 11:18:50 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, davem@...emloft.net, ast@...nel.org,
        toke@...hat.com, lorenzo.bianconi@...hat.com, dsahern@...nel.org,
        andrii.nakryiko@...il.com, brouer@...hat.com
Subject: Re: [PATCH v4 bpf-next 6/9] bpf: cpumap: implement XDP_REDIRECT for
 eBPF programs attached to map entries

On Thu, 25 Jun 2020 23:28:59 +0200
Daniel Borkmann <daniel@...earbox.net> wrote:

> > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> > index 4e4cd240f07b..c0b2f265ccb2 100644
> > --- a/kernel/bpf/cpumap.c
> > +++ b/kernel/bpf/cpumap.c
> > @@ -240,7 +240,7 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
> >   	xdp_set_return_frame_no_direct();
> >   	xdp.rxq = &rxq;
> >   
> > -	rcu_read_lock();
> > +	rcu_read_lock_bh();
> >   
> >   	prog = READ_ONCE(rcpu->prog);
> >   	for (i = 0; i < n; i++) {
> > @@ -266,6 +266,16 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
> >   				stats->pass++;
> >   			}
> >   			break;
> > +		case XDP_REDIRECT:
> > +			err = xdp_do_redirect(xdpf->dev_rx, &xdp,
> > +					      prog);
> > +			if (unlikely(err)) {
> > +				xdp_return_frame(xdpf);
> > +				stats->drop++;

I consider if this should be a redir_err counter.

> > +			} else {
> > +				stats->redirect++;
> > +			}  
> 
> Could we do better with all the accounting and do this from /inside/ BPF tracing prog
> instead (otherwise too bad we need to have it here even if the tracepoint is disabled)?

I'm on-the-fence with this one...

First of all the BPF-prog cannot see the return code of xdp_do_redirect.
So, it cannot give the correct/needed stats without this counter. It
would basically report the redirects as successful redirects. (This is
actually a re-occuring support issue, when end-users misconfigure
xdp_redirect sample and think they get good performance, even-though
packets are dropped).

Specifically for XDP_REDIRECT we need to update some state anyhow, such
that we know to call xdp_do_flush_map(). Thus removing the counter
would not gain much performance wise.


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