On Mon, 25 Jan 2021 23:27:22 +0100
Daniel Borkmann [off-list ref] wrote:

quoted

quoted

quoted

+	/* 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/ (local)
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer