Subject: Re: [PATCH bpf-next V12 4/7] bpf: add BPF-helper for MTU checking

From: Jesper Dangaard Brouer <[email-masked]>
Date: Tue, 26 Jan 2021 10:13:25 +0100

On Mon, 25 Jan 2021 23:27:22 +0100
Daniel Borkmann <[email-masked]> wrote:

> >>> +	/* At this point, skb->len exceed MTU, but as it include length of all
> >>> +	 * segments, it can still be below MTU.  The SKB can possibly get
> >>> +	 * re-segmented in transmit path (see validate_xmit_skb).  Thus, user
> >>> +	 * must choose if segs are to be MTU checked.
> >>> +	 */
> >>> +	if (skb_is_gso(skb)) {
> >>> +		ret = BPF_MTU_CHK_RET_SUCCESS;
> >>> +
> >>> +		if (flags & BPF_MTU_CHK_SEGS &&
> >>> +		    !skb_gso_validate_network_len(skb, mtu))
> >>> +			ret = BPF_MTU_CHK_RET_SEGS_TOOBIG;  
> >>
> >> I think that looks okay overall now. One thing that will easily slip through
> >> is that in the helper description you mentioned 'Check cannot use len_diff.'
> >> for BPF_MTU_CHK_SEGS flag. So right now for non-zero len_diff the user
> >> will still get BPF_MTU_CHK_RET_SUCCESS if the current length check via
> >> skb_gso_validate_network_len(skb, mtu) passes. If it cannot be checked,
> >> maybe enforce len_diff == 0 for gso skbs on BPF_MTU_CHK_SEGS?  
> > 
> > Ok. Do you want/think this can be enforced by the verifier or are you
> > simply requesting that the helper will return -EINVAL (or another errno)?  
> 
> Simple -EINVAL should be fine in this case. Generally, we can detect this from
> verifier side but I don't think the extra complexity is worth it especially given
> this is dependent on BPF_MTU_CHK_SEGS and otherwise can be non-zero.

Luckily this was also my choice in V13 that I've already send out.

https://lore.kernel.org/netdev/161159457239.321749.9067604476261493815.stgit@firesoul/
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer