| Message ID | 160208776033.798237.4028465222836713720.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: New approach for BPF MTU handling and enforcement | expand |
On 10/7/20 6:22 PM, Jesper Dangaard Brouer wrote: [...] > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > (imported from commit 37f8552786cf46588af52b77829b730dd14524d3) slipped in?
> static u32 __bpf_skb_max_len(const struct sk_buff *skb) > { > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > - SKB_MAX_ALLOC; > + return IP_MAX_MTU; > } Shouldn't we just delete this helper instead and replace call sites?
On Wed, 7 Oct 2020 16:46:10 -0700 Maciej Żenczykowski <maze@google.com> wrote: > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > { > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > - SKB_MAX_ALLOC; > > + return IP_MAX_MTU; > > } > > Shouldn't we just delete this helper instead and replace call sites? It does seem wrong to pass argument skb into this function, as it is no-longer used... Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU.
On Thu, Oct 8, 2020 at 7:06 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Wed, 7 Oct 2020 16:46:10 -0700 > Maciej Żenczykowski <maze@google.com> wrote: > > > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > > { > > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > > - SKB_MAX_ALLOC; > > > + return IP_MAX_MTU; > > > } > > > > Shouldn't we just delete this helper instead and replace call sites? > > It does seem wrong to pass argument skb into this function, as it is > no-longer used... > > Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU. Should that be IP6_MAX_MTU, which is larger than IP_MAX_MTU?
On Thu, 8 Oct 2020 08:33:04 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Thu, Oct 8, 2020 at 7:06 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > > > On Wed, 7 Oct 2020 16:46:10 -0700 > > Maciej Żenczykowski <maze@google.com> wrote: > > > > > > static u32 __bpf_skb_max_len(const struct sk_buff *skb) > > > > { > > > > - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : > > > > - SKB_MAX_ALLOC; > > > > + return IP_MAX_MTU; > > > > } > > > > > > Shouldn't we just delete this helper instead and replace call sites? > > > > It does seem wrong to pass argument skb into this function, as it is > > no-longer used... > > > > Guess I can simply replace __bpf_skb_max_len with IP_MAX_MTU. > > Should that be IP6_MAX_MTU, which is larger than IP_MAX_MTU? Sure I'll do that, and handle that is hides under CONFIG_IPV6.
diff --git a/net/core/filter.c b/net/core/filter.c index 05df73780dd3..fed239e77bdc 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3476,8 +3476,7 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff, static u32 __bpf_skb_max_len(const struct sk_buff *skb) { - return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len : - SKB_MAX_ALLOC; + return IP_MAX_MTU; } BPF_CALL_4(sk_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
Multiple BPF-helpers that can manipulate/increase the size of the SKB uses __bpf_skb_max_len() as the max-length. This function limit size against the current net_device MTU (skb->dev->mtu). When a BPF-prog grow the packet size, then it should not be limited to the MTU. The MTU is a transmit limitation, and software receiving this packet should be allowed to increase the size. Further more, current MTU check in __bpf_skb_max_len uses the MTU from ingress/current net_device, which in case of redirects uses the wrong net_device. Keep a sanity max limit of IP_MAX_MTU which is 64KiB. In later patches we will enforce the MTU limitation when transmitting packets. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> (imported from commit 37f8552786cf46588af52b77829b730dd14524d3) --- net/core/filter.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)