Re: [PATCH bpf-next V8 4/8] bpf: add BPF-helper for MTU checking

On Thu, 3 Dec 2020 00:23:14 +0100
Daniel Borkmann [off-list ref] wrote:

On 11/27/20 7:06 PM, Jesper Dangaard Brouer wrote:
[...]

quoted

+static struct net_device *__dev_via_ifindex(struct net_device *dev_curr,
+					    u32 ifindex)
+{
+	struct net *netns = dev_net(dev_curr);
+
+	/* Non-redirect use-cases can use ifindex=0 and save ifindex lookup */
+	if (ifindex == 0)
+		return dev_curr;
+
+	return dev_get_by_index_rcu(netns, ifindex);
+}
+
+BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb,
+	   u32, ifindex, u32 *, mtu_len, s32, len_diff, u64, flags)
+{
+	int ret = BPF_MTU_CHK_RET_FRAG_NEEDED;
+	struct net_device *dev = skb->dev;
+	int len;
+	int mtu;
+
+	if (flags & ~(BPF_MTU_CHK_SEGS))  

nit: unlikely() (similar for XDP case)

ok

quoted

+		return -EINVAL;
+
+	dev = __dev_via_ifindex(dev, ifindex);
+	if (!dev)  

nit: unlikely() (ditto XDP)

ok

quoted

+		return -ENODEV;
+
+	mtu = READ_ONCE(dev->mtu);
+
+	/* TC len is L2, remove L2-header as dev MTU is L3 size */
+	len = skb->len - ETH_HLEN;  

s/ETH_HLEN/dev->hard_header_len/ ?

ok
 

quoted

+	len += len_diff; /* len_diff can be negative, minus result pass check */
+	if (len <= mtu) {
+		ret = BPF_MTU_CHK_RET_SUCCESS;  

Wouldn't it be more intuitive to do ...

    len_dev = READ_ONCE(dev->mtu) + dev->hard_header_len + VLAN_HLEN;
    len_skb = skb->len + len_diff;
    if (len_skb <= len_dev) {
       ret = BPF_MTU_CHK_RET_SUCCESS;
       got out;
    }

Yes, that is more intuitive to read.

quoted

+		goto out;
+	}
+	/* 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.  Last SKB "headlen" is
+	 * checked against MTU.
+	 */
+	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;
+			goto out;  

Maybe my lack of coffe, but looking at ip_exceeds_mtu() for example, shouldn't
the above test be on !skb_gso_validate_network_len() instead?

Yes, you are right!

skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu) would indicate that
it does /not/ exceed mtu.

quoted

+		}
+
+		len = skb_headlen(skb) - ETH_HLEN + len_diff;  

How does this work with GRO when we invoke this helper at tc ingress, e.g. when
there is still non-linear data in skb_shinfo(skb)->frags[]?

In case of skb_is_gso() then this code will check the linear part
skb_headlen(skb) against the MTU.  I though this was an improvement
from what we have today, where skb_is_gso() packets will skip all
checks, which have caused a lot of confusion by end-users.

I will put this under the BPF_MTU_CHK_SEGS flag (in V9) as I understand
from you comment, you don't think this is correct at tc ingress.

quoted

+		if (len > mtu) {
+			ret = BPF_MTU_CHK_RET_FRAG_NEEDED;
+			goto out;
+		}
+	}
+out:
+	/* BPF verifier guarantees valid pointer */
+	*mtu_len = mtu;
+
+	return ret;
+}

[...]

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