Re: [PATCH net] bpf: don't select potentially stale ri->map from buggy xdp progs

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

On Fri, 08 Sep 2017 12:34:28 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 09/08/2017 07:06 AM, Jesper Dangaard Brouer wrote:
> > On Fri,  8 Sep 2017 00:14:51 +0200
> > Daniel Borkmann <daniel@iogearbox.net> wrote:
> >  
> >> +	/* This is really only caused by a deliberately crappy
> >> +	 * BPF program, normally we would never hit that case,
> >> +	 * so no need to inform someone via tracepoints either,
> >> +	 * just bail out.
> >> +	 */
> >> +	if (unlikely(map_owner != xdp_prog))
> >> +		return -EINVAL;  
> >
> > IMHO we do need to call the tracepoint here.  It is not just crappy
> > BPF-progs that cause this situation, it is also drivers not implementing
> > XDP_REDIRECT yet (which is all but ixgbe).  Due to the level XDP
> > operates at, tracepoints are the only way users can runtime troubleshoot
> > their XDP programs.  
> 
> Drivers not implementing XDP_REDIRECT don't even get there in
> the first place. What they will do is to hit the 'default' case
> when they check for the action code from the BPF program. Then
> call into bpf_warn_invalid_xdp_action(act), and fall-through
> to hit the tracepoint at trace_xdp_exception() which is also
> triggered by XDP_ABORTED usually. So when that happens we do
> complain loudly and call a tracepoint already. We should probably
> tweak the bpf_warn_invalid_xdp_action() message a little to make
> it clear that the action could also just be unsupported by the
> driver instead of being illegal.

Yes. drivers not implementing XDP_REDIRECT will cause a tracepoint
trace_xdp_exception() to be called for its _own_ packets.

But it will still setup and leave map and map_owner pointer dangling.
Another NIC can load an xdp_prog that return XDP_REDIRECT, which will hit
above if-statement, and its packets will disappear, without getting
recorded by a tracepoint (thus hard to debug!).

The fundamental point is that tracepoints is the way we choose to
handle debugging XDP programs.  Thus, we must trigger a tracepoint when
a packet gets dropped.  Even in this unlikely case.

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