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

On 09/08/2017 01:52 PM, Jesper Dangaard Brouer wrote:

On Fri, 08 Sep 2017 12:34:28 +0200 Daniel Borkmann [off-list ref] wrote:

quoted

On 09/08/2017 07:06 AM, Jesper Dangaard Brouer wrote:

quoted

On Fri,  8 Sep 2017 00:14:51 +0200
Daniel Borkmann [off-list ref] wrote:

quoted

+	/* 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.

Yep, plus a big one time warning for the case a user doesn't
look at tracepoints initially.

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!).

If a user wants to reproduce this exact error, he would need
to go and reload the program on the driver not supporting the
XDP_REDIRECT in the first place, and then reload his buggy program
on the other driver supporting XDP_REDIRECT but w/o having called
bpf_xdp_redirect_map(), so exactly once on the switch from one
driver to another with this misuse, any subsequent packets will
trigger _trace_xdp_redirect_err(), same way as if the buggy
program was loaded to the 2nd driver from the beginning since
the map and ifindex etc will be zero, hence my comment on this.