On Mon, 21 Sep 2020 11:37:18 +0100 Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote: > On Sat, 19 Sep 2020 at 00:06, Maciej Żenczykowski <maze@xxxxxxxxxx> wrote: > > > > > This is a good point. As bpf_skb_adjust_room() can just be run after > > > bpf_redirect() call, then a MTU check in bpf_redirect() actually > > > doesn't make much sense. As clever/bad BPF program can then avoid the > > > MTU check anyhow. This basically means that we have to do the MTU > > > check (again) on kernel side anyhow to catch such clever/bad BPF > > > programs. (And I don't like wasting cycles on doing the same check two > > > times). > > > > If you get rid of the check in bpf_redirect() you might as well get > > rid of *all* the checks for excessive mtu in all the helpers that > > adjust packet size one way or another way. They *all* then become > > useless overhead. > > > > I don't like that. There may be something the bpf program could do to > > react to the error condition (for example in my case, not modify > > things and just let the core stack deal with things - which will > > probably just generate packet too big icmp error). > > > > btw. right now our forwarding programs first adjust the packet size > > then call bpf_redirect() and almost immediately return what it > > returned. > > > > but this could I think easily be changed to reverse the ordering, so > > we wouldn't increase packet size before the core stack was informed we > > would be forwarding via a different interface. > > We do the same, except that we also use XDP_TX when appropriate. This > complicates the matter, because there is no helper call we could > return an error from. Do notice that my MTU work is focused on TC-BPF. For XDP-redirect the MTU check is done in xdp_ok_fwd_dev() via __xdp_enqueue(), which also happens too late to give BPF-prog knowledge/feedback. For XDP_TX I audited the drivers when I implemented xdp_buff.frame_sz, and they handled (or I added) handling against max HW MTU. E.g. mlx5 [1]. [1] https://elixir.bootlin.com/linux/v5.9-rc6/source/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c#L267 > My preference would be to have three helpers: get MTU for a device, > redirect ctx to a device (with MTU check), resize ctx (without MTU > check) but that doesn't work with XDP_TX. Your idea of doing checks > in redirect and adjust_room is pragmatic and seems easier to > implement. I do like this plan/proposal (with 3 helpers), but it is not possible with current API. The main problem is the current bpf_redirect API doesn't provide the ctx, so we cannot do the check in the BPF-helper. Are you saying we should create a new bpf_redirect API (that incl packet ctx)? -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
