| Message ID | 160407666238.1525159.9197344855524540198.stgit@firesoul |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Subj: bpf: New approach for BPF MTU handling | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 15999 this patch: 15999 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 16031 this patch: 16031 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
Jesper Dangaard Brouer wrote: > This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs. > > The API is designed to help the BPF-programmer, that want to do packet > context size changes, which involves other helpers. These other helpers > usually does a delta size adjustment. This helper also support a delta > size (len_diff), which allow BPF-programmer to reuse arguments needed by > these other helpers, and perform the MTU check prior to doing any actual > size adjustment of the packet context. > > It is on purpose, that we allow the len adjustment to become a negative > result, that will pass the MTU check. This might seem weird, but it's not > this helpers responsibility to "catch" wrong len_diff adjustments. Other > helpers will take care of these checks, if BPF-programmer chooses to do > actual size adjustment. > > V4: Lot of changes > - ifindex 0 now use current netdev for MTU lookup > - rename helper from bpf_mtu_check to bpf_check_mtu > - fix bug for GSO pkt length (as skb->len is total len) > - remove __bpf_len_adj_positive, simply allow negative len adj > > V3: Take L2/ETH_HLEN header size into account and document it. > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- Sorry for the late feedback here. This seems like a lot of baked in functionality into the helper? Can you say something about why the simpler and, at least to me, more intuitive helper to simply return the ifindex mtu is not ideal? Rough pseudo code being, my_sender(struct __sk_buff *skb, int fwd_ifindex) { mtu = bpf_get_ifindex_mtu(fwd_ifindex, 0); if (skb->len + HDR_SIZE < mtu) return send_with_hdrs(skb); return -EMSGSIZE } > include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > net/core/filter.c | 120 ++++++++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > 3 files changed, 260 insertions(+) > [...] > + * **BPF_MTU_CHK_RELAX** > + * This flag relax or increase the MTU with room for one > + * VLAN header (4 bytes). This relaxation is also used by > + * the kernels own forwarding MTU checks. I noted below as well, but not sure why this is needed. Seems if user knows to add a flag because they want a vlan header we can just as easily expect BPF program to do it. Also it only works for VLAN headers any other header data wont be accounted for so it seems only useful in one specific case. > + * > + * **BPF_MTU_CHK_SEGS** > + * This flag will only works for *ctx* **struct sk_buff**. > + * If packet context contains extra packet segment buffers > + * (often knows as GSO skb), then MTU check is partly > + * skipped, because in transmit path it is possible for the > + * skb packet to get re-segmented (depending on net device > + * features). This could still be a MTU violation, so this > + * flag enables performing MTU check against segments, with > + * a different violation return code to tell it apart. > + * > + * The *mtu_result* pointer contains the MTU value of the net > + * device including the L2 header size (usually 14 bytes Ethernet > + * header). The net device configured MTU is the L3 size, but as > + * XDP and TX length operate at L2 this helper include L2 header > + * size in reported MTU. > + * > + * Return > + * * 0 on success, and populate MTU value in *mtu_result* pointer. > + * > + * * < 0 if any input argument is invalid (*mtu_result* not updated) > + * > + * MTU violations return positive values, but also populate MTU > + * value in *mtu_result* pointer, as this can be needed for > + * implementing PMTU handing: > + * > + * * **BPF_MTU_CHK_RET_FRAG_NEEDED** > + * * **BPF_MTU_CHK_RET_SEGS_TOOBIG** > + * > */ [...] > +static int __bpf_lookup_mtu(struct net_device *dev_curr, u32 ifindex, u64 flags) > +{ > + struct net *netns = dev_net(dev_curr); > + struct net_device *dev; > + int mtu; > + > + /* Non-redirect use-cases can use ifindex=0 and save ifindex lookup */ > + if (ifindex == 0) > + dev = dev_curr; > + else > + dev = dev_get_by_index_rcu(netns, ifindex); > + > + if (!dev) > + return -ENODEV; > + > + /* XDP+TC len is L2: Add L2-header as dev MTU is L3 size */ > + mtu = dev->mtu + dev->hard_header_len; READ_ONCE() on dev->mtu and hard_header_len as well? We don't have any locks. > + > + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ > + if (flags & BPF_MTU_CHK_RELAX) > + mtu += VLAN_HLEN; I'm trying to think about the use case where this might be used? Compared to just adjusting MTU in BPF program side as needed for packet encapsulation/headers/etc. > + > + return mtu; > +} > +
On Fri, 30 Oct 2020 13:23:43 -0700 John Fastabend <john.fastabend@gmail.com> wrote: > Jesper Dangaard Brouer wrote: > > This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs. > > > > The API is designed to help the BPF-programmer, that want to do packet > > context size changes, which involves other helpers. These other helpers > > usually does a delta size adjustment. This helper also support a delta > > size (len_diff), which allow BPF-programmer to reuse arguments needed by > > these other helpers, and perform the MTU check prior to doing any actual > > size adjustment of the packet context. > > > > It is on purpose, that we allow the len adjustment to become a negative > > result, that will pass the MTU check. This might seem weird, but it's not > > this helpers responsibility to "catch" wrong len_diff adjustments. Other > > helpers will take care of these checks, if BPF-programmer chooses to do > > actual size adjustment. > > > > V4: Lot of changes > > - ifindex 0 now use current netdev for MTU lookup > > - rename helper from bpf_mtu_check to bpf_check_mtu > > - fix bug for GSO pkt length (as skb->len is total len) > > - remove __bpf_len_adj_positive, simply allow negative len adj > > > > V3: Take L2/ETH_HLEN header size into account and document it. > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > Sorry for the late feedback here. > > This seems like a lot of baked in functionality into the helper? Can you > say something about why the simpler and, at least to me, more intuitive > helper to simply return the ifindex mtu is not ideal? I tried to explain this in the patch description. This is for easier collaboration with other helpers, that also have the len_diff parameter. This API allow to check the MTU *prior* to doing the size adjustment. Let me explain what is not in the patch desc: In the first patchset, I started with the simply implementation of returning the MTU. Then I realized that this puts more work into the BPF program (thus increasing BPF code instructions). As we in BPF-prog need to extract the packet length to compare against the returned MTU size. Looking at other programs that does the ctx/packet size adjust, we don't extract the packet length as ctx is about to change, and we don't need the MTU variable in the BPF prog (unless it fails). > Rough pseudo code being, > > my_sender(struct __sk_buff *skb, int fwd_ifindex) > { > mtu = bpf_get_ifindex_mtu(fwd_ifindex, 0); > if (skb->len + HDR_SIZE < mtu) > return send_with_hdrs(skb); > return -EMSGSIZE > } > > > > include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > > net/core/filter.c | 120 ++++++++++++++++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > > 3 files changed, 260 insertions(+) > > > > [...] > > > + * **BPF_MTU_CHK_RELAX** > > + * This flag relax or increase the MTU with room for one > > + * VLAN header (4 bytes). This relaxation is also used by > > + * the kernels own forwarding MTU checks. > > I noted below as well, but not sure why this is needed. Seems if user > knows to add a flag because they want a vlan header we can just as > easily expect BPF program to do it. Also it only works for VLAN headers > any other header data wont be accounted for so it seems only useful > in one specific case. This was added because the kernels own forwarding have this relaxation build in. Thus, I though that I should add flag to compatible with the kernels forwarding checks. > > + * > > + * **BPF_MTU_CHK_SEGS** > > + * This flag will only works for *ctx* **struct sk_buff**. > > + * If packet context contains extra packet segment buffers > > + * (often knows as GSO skb), then MTU check is partly > > + * skipped, because in transmit path it is possible for the > > + * skb packet to get re-segmented (depending on net device > > + * features). This could still be a MTU violation, so this > > + * flag enables performing MTU check against segments, with > > + * a different violation return code to tell it apart. > > + * > > + * The *mtu_result* pointer contains the MTU value of the net > > + * device including the L2 header size (usually 14 bytes Ethernet > > + * header). The net device configured MTU is the L3 size, but as > > + * XDP and TX length operate at L2 this helper include L2 header > > + * size in reported MTU. > > + * > > + * Return > > + * * 0 on success, and populate MTU value in *mtu_result* pointer. > > + * > > + * * < 0 if any input argument is invalid (*mtu_result* not updated) > > + * > > + * MTU violations return positive values, but also populate MTU > > + * value in *mtu_result* pointer, as this can be needed for > > + * implementing PMTU handing: > > + * > > + * * **BPF_MTU_CHK_RET_FRAG_NEEDED** > > + * * **BPF_MTU_CHK_RET_SEGS_TOOBIG** > > + * > > */ > > [...] > > > +static int __bpf_lookup_mtu(struct net_device *dev_curr, u32 ifindex, u64 flags) > > +{ > > + struct net *netns = dev_net(dev_curr); > > + struct net_device *dev; > > + int mtu; > > + > > + /* Non-redirect use-cases can use ifindex=0 and save ifindex lookup */ > > + if (ifindex == 0) > > + dev = dev_curr; > > + else > > + dev = dev_get_by_index_rcu(netns, ifindex); > > + > > + if (!dev) > > + return -ENODEV; > > + > > + /* XDP+TC len is L2: Add L2-header as dev MTU is L3 size */ > > + mtu = dev->mtu + dev->hard_header_len; > > READ_ONCE() on dev->mtu and hard_header_len as well? We don't have > any locks. This is based on similar checks done in the same execution context, which don't have these READ_ONCE() macros. I'm not introducing reading these, I'm simply moving when they are read. If this is really needed, then I think we need separate fixes patches, for stable backporting. While doing this work, I've realized that mtu + hard_header_len is located on two different cache-lines, which is unfortunate, but I will look at fixing this in followup patches. > > + > > + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ > > + if (flags & BPF_MTU_CHK_RELAX) > > + mtu += VLAN_HLEN; > > I'm trying to think about the use case where this might be used? > Compared to just adjusting MTU in BPF program side as needed for > packet encapsulation/headers/etc. As I wrote above, this were added because the kernels own forwarding have this relaxation in it's checks (in is_skb_forwardable()). I even tried to dig through the history, introduced in [1] and copy-pasted in[2]. And this seems to be a workaround, that have become standard, that still have practical implications. My practical experiments showed, that e.g. ixgbe driver with MTU=1500 (L3-size) will allow and fully send packets with 1504 (L3-size). But i40e will not, and drops the packet in hardware/firmware step. So, what is the correct action, strict or relaxed? My own conclusion is that we should inverse the flag. Meaning to default add this VLAN_HLEN (4 bytes) relaxation, and have a flag to do more strict check, e.g. BPF_MTU_CHK_STRICT. As for historical reasons we must act like kernels version of MTU check. Unless you object, I will do this in V6.
Jesper Dangaard Brouer wrote: > On Fri, 30 Oct 2020 13:23:43 -0700 > John Fastabend <john.fastabend@gmail.com> wrote: > > > Jesper Dangaard Brouer wrote: > > > This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs. > > > > > > The API is designed to help the BPF-programmer, that want to do packet > > > context size changes, which involves other helpers. These other helpers > > > usually does a delta size adjustment. This helper also support a delta > > > size (len_diff), which allow BPF-programmer to reuse arguments needed by > > > these other helpers, and perform the MTU check prior to doing any actual > > > size adjustment of the packet context. > > > > > > It is on purpose, that we allow the len adjustment to become a negative > > > result, that will pass the MTU check. This might seem weird, but it's not > > > this helpers responsibility to "catch" wrong len_diff adjustments. Other > > > helpers will take care of these checks, if BPF-programmer chooses to do > > > actual size adjustment. > > > > > > V4: Lot of changes > > > - ifindex 0 now use current netdev for MTU lookup > > > - rename helper from bpf_mtu_check to bpf_check_mtu > > > - fix bug for GSO pkt length (as skb->len is total len) > > > - remove __bpf_len_adj_positive, simply allow negative len adj > > > > > > V3: Take L2/ETH_HLEN header size into account and document it. > > > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > > --- > > > > Sorry for the late feedback here. > > > > This seems like a lot of baked in functionality into the helper? Can you > > say something about why the simpler and, at least to me, more intuitive > > helper to simply return the ifindex mtu is not ideal? > > I tried to explain this in the patch description. This is for easier > collaboration with other helpers, that also have the len_diff parameter. > This API allow to check the MTU *prior* to doing the size adjustment. > > Let me explain what is not in the patch desc: OK extra details helps. > > In the first patchset, I started with the simply implementation of > returning the MTU. Then I realized that this puts more work into the > BPF program (thus increasing BPF code instructions). As we in BPF-prog > need to extract the packet length to compare against the returned MTU > size. Looking at other programs that does the ctx/packet size adjust, we > don't extract the packet length as ctx is about to change, and we don't > need the MTU variable in the BPF prog (unless it fails). On recent kernels instruction counts should not be a problem. So, looks like the argument is to push the skb->len lookup from BPF program into helper. I'm not sure it matters much if the insn is run in helper or in the BPF program. I have a preference for the simpler "give me the MTU and I'll figure out what to do with it". Real programs will have to handle the failing case and will need to deal with MTU anyways. We could do it as a BPF implemented helper in one of the BPF headers so users could just call the BPF "helper" and not know parts of it are implemented in BPF. > > > > Rough pseudo code being, > > > > my_sender(struct __sk_buff *skb, int fwd_ifindex) > > { > > mtu = bpf_get_ifindex_mtu(fwd_ifindex, 0); > > if (skb->len + HDR_SIZE < mtu) > > return send_with_hdrs(skb); > > return -EMSGSIZE > > } > > > > > > > include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > > > net/core/filter.c | 120 ++++++++++++++++++++++++++++++++++++++++ > > > tools/include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ > > > 3 files changed, 260 insertions(+) > > > > > > > [...] > > > > > + * **BPF_MTU_CHK_RELAX** > > > + * This flag relax or increase the MTU with room for one > > > + * VLAN header (4 bytes). This relaxation is also used by > > > + * the kernels own forwarding MTU checks. > > > > I noted below as well, but not sure why this is needed. Seems if user > > knows to add a flag because they want a vlan header we can just as > > easily expect BPF program to do it. Alsoer it only works for VLAN headers > > any other header data wont be accounted for so it seems only useful > > in one specific case. > > This was added because the kernels own forwarding have this relaxation > build in. Thus, I though that I should add flag to compatible with the > kernels forwarding checks. OK, I guess it doesn't hurt. > > > > + * > > > + * **BPF_MTU_CHK_SEGS** > > > + * This flag will only works for *ctx* **struct sk_buff**. > > > + * If packet context contains extra packet segment buffers > > > + * (often knows as GSO skb), then MTU check is partly > > > + * skipped, because in transmit path it is possible for the > > > + * skb packet to get re-segmented (depending on net device > > > + * features). This could still be a MTU violation, so this > > > + * flag enables performing MTU check against segments, with > > > + * a different violation return code to tell it apart. > > > + * > > > + * The *mtu_result* pointer contains the MTU value of the net > > > + * device including the L2 header size (usually 14 bytes Ethernet > > > + * header). The net device configured MTU is the L3 size, but as > > > + * XDP and TX length operate at L2 this helper include L2 header > > > + * size in reported MTU. > > > + * > > > + * Return > > > + * * 0 on success, and populate MTU value in *mtu_result* pointer. > > > + * > > > + * * < 0 if any input argument is invalid (*mtu_result* not updated) > > > + * > > > + * MTU violations return positive values, but also populate MTU > > > + * value in *mtu_result* pointer, as this can be needed for > > > + * implementing PMTU handing: > > > + * > > > + * * **BPF_MTU_CHK_RET_FRAG_NEEDED** > > > + * * **BPF_MTU_CHK_RET_SEGS_TOOBIG** > > > + * > > > */ > > > > [...] > > > > > +static int __bpf_lookup_mtu(struct net_device *dev_curr, u32 ifindex, u64 flags) > > > +{ > > > + struct net *netns = dev_net(dev_curr); > > > + struct net_device *dev; > > > + int mtu; > > > + > > > + /* Non-redirect use-cases can use ifindex=0 and save ifindex lookup */ > > > + if (ifindex == 0) > > > + dev = dev_curr; > > > + else > > > + dev = dev_get_by_index_rcu(netns, ifindex); > > > + > > > + if (!dev) > > > + return -ENODEV; > > > + > > > + /* XDP+TC len is L2: Add L2-header as dev MTU is L3 size */ > > > + mtu = dev->mtu + dev->hard_header_len; > > > > READ_ONCE() on dev->mtu and hard_header_len as well? We don't have > > any locks. > > This is based on similar checks done in the same execution context, > which don't have these READ_ONCE() macros. I'm not introducing reading > these, I'm simply moving when they are read. If this is really needed, > then I think we need separate fixes patches, for stable backporting. > > While doing this work, I've realized that mtu + hard_header_len is > located on two different cache-lines, which is unfortunate, but I will > look at fixing this in followup patches. Looks like a follow up then. But, would be best to add the READ_ONCE here. The netdevice.h header has this comment, /* Note : dev->mtu is often read without holding a lock. * Writers usually hold RTNL. * It is recommended to use READ_ONCE() to annotate the reads, * and to use WRITE_ONCE() to annotate the writes. */ > > > > > + > > > + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ > > > + if (flags & BPF_MTU_CHK_RELAX) > > > + mtu += VLAN_HLEN; > > > > I'm trying to think about the use case where this might be used? > > Compared to just adjusting MTU in BPF program side as needed for > > packet encapsulation/headers/etc. > > As I wrote above, this were added because the kernels own forwarding > have this relaxation in it's checks (in is_skb_forwardable()). I even > tried to dig through the history, introduced in [1] and copy-pasted > in[2]. And this seems to be a workaround, that have become standard, > that still have practical implications. > > My practical experiments showed, that e.g. ixgbe driver with MTU=1500 > (L3-size) will allow and fully send packets with 1504 (L3-size). But > i40e will not, and drops the packet in hardware/firmware step. So, > what is the correct action, strict or relaxed? > > My own conclusion is that we should inverse the flag. Meaning to > default add this VLAN_HLEN (4 bytes) relaxation, and have a flag to do > more strict check, e.g. BPF_MTU_CHK_STRICT. As for historical reasons > we must act like kernels version of MTU check. Unless you object, I will > do this in V6. I'm fine with it either way as long as its documented in the helper description so I have a chance of remembering this discussion in 6 months. But, if you make it default won't this break for XDP cases? I assume the XDP use case doesn't include the VLAN 4-bytes. Would you need to prevent the flag from being used from XDP? Thanks, John
On Mon, 02 Nov 2020 10:04:44 -0800 John Fastabend <john.fastabend@gmail.com> wrote: > > > > + > > > > + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ > > > > + if (flags & BPF_MTU_CHK_RELAX) > > > > + mtu += VLAN_HLEN; > > > > > > I'm trying to think about the use case where this might be used? > > > Compared to just adjusting MTU in BPF program side as needed for > > > packet encapsulation/headers/etc. > > > > As I wrote above, this were added because the kernels own forwarding > > have this relaxation in it's checks (in is_skb_forwardable()). I even > > tried to dig through the history, introduced in [1] and copy-pasted > > in[2]. And this seems to be a workaround, that have become standard, > > that still have practical implications. > > > > My practical experiments showed, that e.g. ixgbe driver with MTU=1500 > > (L3-size) will allow and fully send packets with 1504 (L3-size). But > > i40e will not, and drops the packet in hardware/firmware step. So, > > what is the correct action, strict or relaxed? > > > > My own conclusion is that we should inverse the flag. Meaning to > > default add this VLAN_HLEN (4 bytes) relaxation, and have a flag to do > > more strict check, e.g. BPF_MTU_CHK_STRICT. As for historical reasons > > we must act like kernels version of MTU check. Unless you object, I will > > do this in V6. > > I'm fine with it either way as long as its documented in the helper > description so I have a chance of remembering this discussion in 6 months. > But, if you make it default won't this break for XDP cases? I assume the > XDP use case doesn't include the VLAN 4-bytes. Would you need to prevent > the flag from being used from XDP? XDP actually do include the VLAN_HLEN 4-bytes, see xdp_ok_fwd_dev(). I was so certain that you John added this code, but looking through git blame it pointed back to myself. Going 5 levels git history deep and 3+ years, does seem like I move/reused some of Johns code containing VLAN_HLEN in the MTU check, introduced for xdp-generic (6103aa96ec077) which I acked. Thus, I guess I cannot push this away and have to take blame myself ;-) I conclude that we default need to include this VLAN_HLEN, else the XDP bpf_check_mtu could say deny, while it would have passed the check in xdp_ok_fwd_dev(). As i40e will drop 1504 this at HW/FW level, I still see a need for a BPF_MTU_CHK_STRICT flag for programs that want to catch this.
On Mon, 2 Nov 2020 21:10:34 +0100 Jesper Dangaard Brouer <brouer@redhat.com> wrote: > On Mon, 02 Nov 2020 10:04:44 -0800 > John Fastabend <john.fastabend@gmail.com> wrote: > > > > > > + > > > > > + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ > > > > > + if (flags & BPF_MTU_CHK_RELAX) > > > > > + mtu += VLAN_HLEN; > > > > > > > > I'm trying to think about the use case where this might be used? > > > > Compared to just adjusting MTU in BPF program side as needed for > > > > packet encapsulation/headers/etc. > > > > > > As I wrote above, this were added because the kernels own forwarding > > > have this relaxation in it's checks (in is_skb_forwardable()). I even > > > tried to dig through the history, introduced in [1] and copy-pasted > > > in[2]. And this seems to be a workaround, that have become standard, > > > that still have practical implications. > > > > > > My practical experiments showed, that e.g. ixgbe driver with MTU=1500 > > > (L3-size) will allow and fully send packets with 1504 (L3-size). But > > > i40e will not, and drops the packet in hardware/firmware step. So, > > > what is the correct action, strict or relaxed? > > > > > > My own conclusion is that we should inverse the flag. Meaning to > > > default add this VLAN_HLEN (4 bytes) relaxation, and have a flag to do > > > more strict check, e.g. BPF_MTU_CHK_STRICT. As for historical reasons > > > we must act like kernels version of MTU check. Unless you object, I will > > > do this in V6. > > > > I'm fine with it either way as long as its documented in the helper > > description so I have a chance of remembering this discussion in 6 months. > > But, if you make it default won't this break for XDP cases? I assume the > > XDP use case doesn't include the VLAN 4-bytes. Would you need to prevent > > the flag from being used from XDP? > > XDP actually do include the VLAN_HLEN 4-bytes, see xdp_ok_fwd_dev(). I > was so certain that you John added this code, but looking through git > blame it pointed back to myself. Going 5 levels git history deep and > 3+ years, does seem like I move/reused some of Johns code containing > VLAN_HLEN in the MTU check, introduced for xdp-generic (6103aa96ec077) > which I acked. Thus, I guess I cannot push this away and have to take > blame myself ;-) > > I conclude that we default need to include this VLAN_HLEN, else the XDP > bpf_check_mtu could say deny, while it would have passed the check in > xdp_ok_fwd_dev(). As i40e will drop 1504 this at HW/FW level, I still > see a need for a BPF_MTU_CHK_STRICT flag for programs that want to > catch this. Disagreeing with myself... I want to keep the BPF_MTU_CHK_RELAX, and let MTU check use the actual MTU value (adjusted to L2 of-cause). With the argument, that because some drivers with MTU 1500 will actually drop frame with MTU 1504 bytes (+14 eth_hdr) frames, it is wrong to "approve" this MTU size in the check. A BPF program will know it is playing with VLAN headers and can choose to violate the MTU check with 4 bytes. While BPF programs using other types of encap headers will get confused that MTU check gives them 4 bytes more, which if used will get dropped on a subset of drivers.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 01b2b17c645a..8f0fee2df3a6 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3745,6 +3745,63 @@ union bpf_attr { * Return * The helper returns **TC_ACT_REDIRECT** on success or * **TC_ACT_SHOT** on error. + * + * int bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_result, s32 len_diff, u64 flags) + * Description + * Check ctx packet size against MTU of net device (based on + * *ifindex*). This helper will likely be used in combination with + * helpers that adjust/change the packet size. The argument + * *len_diff* can be used for querying with a planned size + * change. This allows to check MTU prior to changing packet ctx. + * + * Specifying *ifindex* zero means the MTU check is performed + * against the current net device. This is practical if this isn't + * used prior to redirect. + * + * The Linux kernel route table can configure MTUs on a more + * specific per route level, which is not provided by this helper. + * For route level MTU checks use the **bpf_fib_lookup**\ () + * helper. + * + * *ctx* is either **struct xdp_md** for XDP programs or + * **struct sk_buff** for tc cls_act programs. + * + * The *flags* argument can be a combination of one or more of the + * following values: + * + * **BPF_MTU_CHK_RELAX** + * This flag relax or increase the MTU with room for one + * VLAN header (4 bytes). This relaxation is also used by + * the kernels own forwarding MTU checks. + * + * **BPF_MTU_CHK_SEGS** + * This flag will only works for *ctx* **struct sk_buff**. + * If packet context contains extra packet segment buffers + * (often knows as GSO skb), then MTU check is partly + * skipped, because in transmit path it is possible for the + * skb packet to get re-segmented (depending on net device + * features). This could still be a MTU violation, so this + * flag enables performing MTU check against segments, with + * a different violation return code to tell it apart. + * + * The *mtu_result* pointer contains the MTU value of the net + * device including the L2 header size (usually 14 bytes Ethernet + * header). The net device configured MTU is the L3 size, but as + * XDP and TX length operate at L2 this helper include L2 header + * size in reported MTU. + * + * Return + * * 0 on success, and populate MTU value in *mtu_result* pointer. + * + * * < 0 if any input argument is invalid (*mtu_result* not updated) + * + * MTU violations return positive values, but also populate MTU + * value in *mtu_result* pointer, as this can be needed for + * implementing PMTU handing: + * + * * **BPF_MTU_CHK_RET_FRAG_NEEDED** + * * **BPF_MTU_CHK_RET_SEGS_TOOBIG** + * */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3903,6 +3960,7 @@ union bpf_attr { FN(bpf_per_cpu_ptr), \ FN(bpf_this_cpu_ptr), \ FN(redirect_peer), \ + FN(check_mtu), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper @@ -4927,6 +4985,18 @@ struct bpf_redir_neigh { }; }; +/* bpf_check_mtu flags*/ +enum bpf_check_mtu_flags { + BPF_MTU_CHK_RELAX = (1U << 0), + BPF_MTU_CHK_SEGS = (1U << 1), +}; + +enum bpf_check_mtu_ret { + BPF_MTU_CHK_RET_SUCCESS, /* check and lookup successful */ + BPF_MTU_CHK_RET_FRAG_NEEDED, /* fragmentation required to fwd */ + BPF_MTU_CHK_RET_SEGS_TOOBIG, /* GSO re-segmentation needed to fwd */ +}; + enum bpf_task_fd_type { BPF_FD_TYPE_RAW_TRACEPOINT, /* tp name */ BPF_FD_TYPE_TRACEPOINT, /* tp name */ diff --git a/net/core/filter.c b/net/core/filter.c index edb543c477b6..bd4a416bd9ad 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5594,6 +5594,122 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { .arg4_type = ARG_ANYTHING, }; +static int __bpf_lookup_mtu(struct net_device *dev_curr, u32 ifindex, u64 flags) +{ + struct net *netns = dev_net(dev_curr); + struct net_device *dev; + int mtu; + + /* Non-redirect use-cases can use ifindex=0 and save ifindex lookup */ + if (ifindex == 0) + dev = dev_curr; + else + dev = dev_get_by_index_rcu(netns, ifindex); + + if (!dev) + return -ENODEV; + + /* XDP+TC len is L2: Add L2-header as dev MTU is L3 size */ + mtu = dev->mtu + dev->hard_header_len; + + /* Same relax as xdp_ok_fwd_dev() and is_skb_forwardable() */ + if (flags & BPF_MTU_CHK_RELAX) + mtu += VLAN_HLEN; + + return mtu; +} + +BPF_CALL_5(bpf_skb_check_mtu, struct sk_buff *, skb, + u32, ifindex, u32 *, mtu_result, s32, len_diff, u64, flags) +{ + int ret = BPF_MTU_CHK_RET_FRAG_NEEDED; + struct net_device *dev = skb->dev; + int len = skb->len; + int mtu; + + if (flags & ~(BPF_MTU_CHK_RELAX | BPF_MTU_CHK_SEGS)) + return -EINVAL; + + mtu = __bpf_lookup_mtu(dev, ifindex, flags); + if (unlikely(mtu < 0)) + return mtu; /* errno */ + + len += len_diff; /* len_diff can be negative, minus result pass check */ + if (len <= mtu) { + ret = BPF_MTU_CHK_RET_SUCCESS; + goto out; + } + /* At this point, skb->len exceed MTU, but as it include length of all + * segments, and SKB can get re-segmented in transmit path (see + * validate_xmit_skb), we cannot reject MTU-check for GSO packets. + */ + if (skb_is_gso(skb)) { + ret = BPF_MTU_CHK_RET_SUCCESS; + + /* SKB could get dropped later due to segs > MTU or lacking + * features, thus allow BPF-prog to validate segs length here. + */ + if (flags & BPF_MTU_CHK_SEGS && + skb_gso_validate_network_len(skb, mtu)) { + ret = BPF_MTU_CHK_RET_SEGS_TOOBIG; + goto out; + } + } +out: + if (mtu_result) + *mtu_result = mtu; + + return ret; +} + +BPF_CALL_5(bpf_xdp_check_mtu, struct xdp_buff *, xdp, + u32, ifindex, u32 *, mtu_result, s32, len_diff, u64, flags) +{ + struct net_device *dev = xdp->rxq->dev; + int len = xdp->data_end - xdp->data; + int ret = BPF_MTU_CHK_RET_SUCCESS; + int mtu; + + /* XDP variant doesn't support multi-buffer segment check (yet) */ + if (flags & ~BPF_MTU_CHK_RELAX) + return -EINVAL; + + mtu = __bpf_lookup_mtu(dev, ifindex, flags); + if (unlikely(mtu < 0)) + return mtu; /* errno */ + + len += len_diff; /* len_diff can be negative, minus result pass check */ + if (len > mtu) + ret = BPF_MTU_CHK_RET_FRAG_NEEDED; + + if (mtu_result) + *mtu_result = mtu; + + return ret; +} + +static const struct bpf_func_proto bpf_skb_check_mtu_proto = { + .func = bpf_skb_check_mtu, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_CTX, + .arg2_type = ARG_ANYTHING, + .arg3_type = ARG_PTR_TO_MEM, + .arg4_type = ARG_ANYTHING, + .arg5_type = ARG_ANYTHING, +}; + +static const struct bpf_func_proto bpf_xdp_check_mtu_proto = { + .func = bpf_xdp_check_mtu, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_CTX, + .arg2_type = ARG_ANYTHING, + .arg3_type = ARG_PTR_TO_MEM, + .arg4_type = ARG_ANYTHING, + .arg5_type = ARG_ANYTHING, +}; + #if IS_ENABLED(CONFIG_IPV6_SEG6_BPF) static int bpf_push_seg6_encap(struct sk_buff *skb, u32 type, void *hdr, u32 len) { @@ -7159,6 +7275,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_get_socket_uid_proto; case BPF_FUNC_fib_lookup: return &bpf_skb_fib_lookup_proto; + case BPF_FUNC_check_mtu: + return &bpf_skb_check_mtu_proto; case BPF_FUNC_sk_fullsock: return &bpf_sk_fullsock_proto; case BPF_FUNC_sk_storage_get: @@ -7228,6 +7346,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_xdp_adjust_tail_proto; case BPF_FUNC_fib_lookup: return &bpf_xdp_fib_lookup_proto; + case BPF_FUNC_check_mtu: + return &bpf_xdp_check_mtu_proto; #ifdef CONFIG_INET case BPF_FUNC_sk_lookup_udp: return &bpf_xdp_sk_lookup_udp_proto; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 01b2b17c645a..8f0fee2df3a6 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3745,6 +3745,63 @@ union bpf_attr { * Return * The helper returns **TC_ACT_REDIRECT** on success or * **TC_ACT_SHOT** on error. + * + * int bpf_check_mtu(void *ctx, u32 ifindex, u32 *mtu_result, s32 len_diff, u64 flags) + * Description + * Check ctx packet size against MTU of net device (based on + * *ifindex*). This helper will likely be used in combination with + * helpers that adjust/change the packet size. The argument + * *len_diff* can be used for querying with a planned size + * change. This allows to check MTU prior to changing packet ctx. + * + * Specifying *ifindex* zero means the MTU check is performed + * against the current net device. This is practical if this isn't + * used prior to redirect. + * + * The Linux kernel route table can configure MTUs on a more + * specific per route level, which is not provided by this helper. + * For route level MTU checks use the **bpf_fib_lookup**\ () + * helper. + * + * *ctx* is either **struct xdp_md** for XDP programs or + * **struct sk_buff** for tc cls_act programs. + * + * The *flags* argument can be a combination of one or more of the + * following values: + * + * **BPF_MTU_CHK_RELAX** + * This flag relax or increase the MTU with room for one + * VLAN header (4 bytes). This relaxation is also used by + * the kernels own forwarding MTU checks. + * + * **BPF_MTU_CHK_SEGS** + * This flag will only works for *ctx* **struct sk_buff**. + * If packet context contains extra packet segment buffers + * (often knows as GSO skb), then MTU check is partly + * skipped, because in transmit path it is possible for the + * skb packet to get re-segmented (depending on net device + * features). This could still be a MTU violation, so this + * flag enables performing MTU check against segments, with + * a different violation return code to tell it apart. + * + * The *mtu_result* pointer contains the MTU value of the net + * device including the L2 header size (usually 14 bytes Ethernet + * header). The net device configured MTU is the L3 size, but as + * XDP and TX length operate at L2 this helper include L2 header + * size in reported MTU. + * + * Return + * * 0 on success, and populate MTU value in *mtu_result* pointer. + * + * * < 0 if any input argument is invalid (*mtu_result* not updated) + * + * MTU violations return positive values, but also populate MTU + * value in *mtu_result* pointer, as this can be needed for + * implementing PMTU handing: + * + * * **BPF_MTU_CHK_RET_FRAG_NEEDED** + * * **BPF_MTU_CHK_RET_SEGS_TOOBIG** + * */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3903,6 +3960,7 @@ union bpf_attr { FN(bpf_per_cpu_ptr), \ FN(bpf_this_cpu_ptr), \ FN(redirect_peer), \ + FN(check_mtu), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper @@ -4927,6 +4985,18 @@ struct bpf_redir_neigh { }; }; +/* bpf_check_mtu flags*/ +enum bpf_check_mtu_flags { + BPF_MTU_CHK_RELAX = (1U << 0), + BPF_MTU_CHK_SEGS = (1U << 1), +}; + +enum bpf_check_mtu_ret { + BPF_MTU_CHK_RET_SUCCESS, /* check and lookup successful */ + BPF_MTU_CHK_RET_FRAG_NEEDED, /* fragmentation required to fwd */ + BPF_MTU_CHK_RET_SEGS_TOOBIG, /* GSO re-segmentation needed to fwd */ +}; + enum bpf_task_fd_type { BPF_FD_TYPE_RAW_TRACEPOINT, /* tp name */ BPF_FD_TYPE_TRACEPOINT, /* tp name */
This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs. The API is designed to help the BPF-programmer, that want to do packet context size changes, which involves other helpers. These other helpers usually does a delta size adjustment. This helper also support a delta size (len_diff), which allow BPF-programmer to reuse arguments needed by these other helpers, and perform the MTU check prior to doing any actual size adjustment of the packet context. It is on purpose, that we allow the len adjustment to become a negative result, that will pass the MTU check. This might seem weird, but it's not this helpers responsibility to "catch" wrong len_diff adjustments. Other helpers will take care of these checks, if BPF-programmer chooses to do actual size adjustment. V4: Lot of changes - ifindex 0 now use current netdev for MTU lookup - rename helper from bpf_mtu_check to bpf_check_mtu - fix bug for GSO pkt length (as skb->len is total len) - remove __bpf_len_adj_positive, simply allow negative len adj V3: Take L2/ETH_HLEN header size into account and document it. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ net/core/filter.c | 120 ++++++++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 70 +++++++++++++++++++++++ 3 files changed, 260 insertions(+)