On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
- if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
- return -EINVAL;
-
- if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
+ if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
+ (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
+ *mtu_len = 0;
return -EINVAL;
+ }
dev = __dev_via_ifindex(dev, ifindex);
- if (unlikely(!dev))
+ if (unlikely(!dev)) {
+ *mtu_len = 0;
return -ENODEV;
+ }
I don't understand this mtu_len clearing.
My earlier comment was that mtu is in&out argument.
The program has to set it to something. It cannot be uninit.
So zeroing it on error looks very odd.
In that sense the patch 3 looks wrong. Instead of:
@@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
bpf_skb_check_mtu_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_CTX,
.arg2_type = ARG_ANYTHING,
- .arg3_type = ARG_PTR_TO_INT,
+ .arg3_type = ARG_PTR_TO_FIXED_SIZE_MEM |
+ MEM_UNINIT | MEM_ALIGNED,
+ .arg3_size = sizeof(u32),
MEM_UNINIT should be removed, because
bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
If there is a program out there that calls this helper without
initializing mtu_len it will be rejected after we fix it,
but I think that's a good thing.
Passing random mtu_len and let helper do:
skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
is just bad.
Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
that even if its garbage MTU being passed in, mtu_len gets filled in
either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
ifindex e.g.:
__u32 mtu_len;
bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);
Will spin a v5 then.
Thanks,
Daniel
