| Message ID | 20240815124302.982711-7-dongml2@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: vxlan: add skb drop reasons support | expand |
On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote: > #define VXLAN_DROP_REASONS(R) \ > + R(VXLAN_DROP_FLAGS) \ > + R(VXLAN_DROP_VNI) \ > + R(VXLAN_DROP_MAC) \ Drop reasons should be documented. I don't think name of a header field is a great fit for a reason. > /* deliberate comment for trailing \ */ > > enum vxlan_drop_reason { > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > index e971c4785962..9a61f04bb95d 100644 > --- a/drivers/net/vxlan/vxlan_core.c > +++ b/drivers/net/vxlan/vxlan_core.c > @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph, > /* Callback from net/ipv4/udp.c to receive packets */ > static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > { > + enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN); Do not call complex functions inline as variable init.. > struct vxlan_vni_node *vninode = NULL; > struct vxlan_dev *vxlan; > struct vxlan_sock *vs; > @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > int nh; > > /* Need UDP and VXLAN header to be present */ > - if (!pskb_may_pull(skb, VXLAN_HLEN)) > + if (reason != SKB_NOT_DROPPED_YET) please don't compare against "not dropped yet", just: if (reason) > @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > return 0; > > drop: > + SKB_DR_RESET(reason); the name of this macro is very confusing, I don't think it should exist in the first place. nothing should goto drop without initialing reason > /* Consume bad packet */ > - kfree_skb(skb); > + kfree_skb_reason(skb, reason); > return 0; > }
On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote: > > #define VXLAN_DROP_REASONS(R) \ > > + R(VXLAN_DROP_FLAGS) \ > > + R(VXLAN_DROP_VNI) \ > > + R(VXLAN_DROP_MAC) \ > > Drop reasons should be documented. Yeah, I wrote the code here just like what we did in net/openvswitch/drop.h, which makes the definition of enum ovs_drop_reason a call of VXLAN_DROP_REASONS(). I think that we can define the enum ovs_drop_reason just like what we do in include/net/dropreason-core.h, which can make it easier to document the reasons. > I don't think name of a header field is a great fit for a reason. > Enn...Do you mean the "VXLAN_DROP_" prefix? > > /* deliberate comment for trailing \ */ > > > > enum vxlan_drop_reason { > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > > index e971c4785962..9a61f04bb95d 100644 > > --- a/drivers/net/vxlan/vxlan_core.c > > +++ b/drivers/net/vxlan/vxlan_core.c > > @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph, > > /* Callback from net/ipv4/udp.c to receive packets */ > > static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > { > > + enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN); > > Do not call complex functions inline as variable init.. Okay! > > > struct vxlan_vni_node *vninode = NULL; > > struct vxlan_dev *vxlan; > > struct vxlan_sock *vs; > > @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > int nh; > > > > /* Need UDP and VXLAN header to be present */ > > - if (!pskb_may_pull(skb, VXLAN_HLEN)) > > + if (reason != SKB_NOT_DROPPED_YET) > > please don't compare against "not dropped yet", just: > Okay! > if (reason) > > > @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > return 0; > > > > drop: > > + SKB_DR_RESET(reason); > > the name of this macro is very confusing, I don't think it should exist > in the first place. nothing should goto drop without initialing reason > It's for the case that we call a function which returns drop reasons. For example, the reason now is assigned from: reason = pskb_may_pull_reason(skb, VXLAN_HLEN); if (reason) goto drop; xxxxxx if (xx) goto drop; The reason now is SKB_NOT_DROPPED_YET when we "goto drop", as we don't set a drop reason here, which is unnecessary in some cases. And, we can't set the drop reason for every "drop" code path, can we? So, we need to check if the drop reason is SKB_NOT_DROPPED_YET when we call kfree_skb_reason(). We use "SKB_DR_OR(drop_reason, NOT_SPECIFIED);" in tcp_v4_rcv() for this purpose. And we can remove SKB_DR_RESET and replace it with "SKB_DR_OR(drop_reason, NOT_SPECIFIED);" here if you think it's ok. Thanks! Menglong Dong > > /* Consume bad packet */ > > - kfree_skb(skb); > > + kfree_skb_reason(skb, reason); > > return 0; > > }
On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote: > On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote: > > > #define VXLAN_DROP_REASONS(R) \ > > > + R(VXLAN_DROP_FLAGS) \ > > > + R(VXLAN_DROP_VNI) \ > > > + R(VXLAN_DROP_MAC) \ > > > > Drop reasons should be documented. > > Yeah, I wrote the code here just like what we did in > net/openvswitch/drop.h, which makes the definition of > enum ovs_drop_reason a call of VXLAN_DROP_REASONS(). > > I think that we can define the enum ovs_drop_reason just like > what we do in include/net/dropreason-core.h, which can make > it easier to document the reasons. > > > I don't think name of a header field is a great fit for a reason. > > > > Enn...Do you mean the "VXLAN_DROP_" prefix? No, I mean the thing after VXLAN_DROP_, it's FLAGS, VNI, MAC, those are names of header fields. > > > @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > > return 0; > > > > > > drop: > > > + SKB_DR_RESET(reason); > > > > the name of this macro is very confusing, I don't think it should exist > > in the first place. nothing should goto drop without initialing reason > > > > It's for the case that we call a function which returns drop reasons. > For example, the reason now is assigned from: > > reason = pskb_may_pull_reason(skb, VXLAN_HLEN); > if (reason) goto drop; > > xxxxxx > if (xx) goto drop; > > The reason now is SKB_NOT_DROPPED_YET when we "goto drop", > as we don't set a drop reason here, which is unnecessary in some cases. > And, we can't set the drop reason for every "drop" code path, can we? Why? It's like saying "we can't set return code before jumping to an error label". In my mind drop reasons and function return codes are very similar. So IDK why we need all the SK_DR_ macros when we are just fine without them for function return codes.
On Thu, Aug 15, 2024 at 08:42:58PM +0800, Menglong Dong wrote: > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > index e971c4785962..9a61f04bb95d 100644 > --- a/drivers/net/vxlan/vxlan_core.c > +++ b/drivers/net/vxlan/vxlan_core.c > @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph, > /* Callback from net/ipv4/udp.c to receive packets */ > static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > { > + enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN); > struct vxlan_vni_node *vninode = NULL; > struct vxlan_dev *vxlan; > struct vxlan_sock *vs; > @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > int nh; > > /* Need UDP and VXLAN header to be present */ > - if (!pskb_may_pull(skb, VXLAN_HLEN)) > + if (reason != SKB_NOT_DROPPED_YET) > goto drop; > > unparsed = *vxlan_hdr(skb); > @@ -1690,6 +1691,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n", > ntohl(vxlan_hdr(skb)->vx_flags), > ntohl(vxlan_hdr(skb)->vx_vni)); > + reason = (u32)VXLAN_DROP_FLAGS; I don't find "FLAGS" very descriptive. AFAICT the reason is used in these two cases: 1. "I flag" is not set 2. The reserved fields are not zero Maybe call it VXLAN_DROP_INVALID_HDR ? And I agree with the comment about documenting these drop reasons like in include/net/dropreason-core.h > /* Return non vxlan pkt */ > goto drop; > } > @@ -1703,8 +1705,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > vni = vxlan_vni(vxlan_hdr(skb)->vx_vni); > > vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode); > - if (!vxlan) > + if (!vxlan) { > + reason = (u32)VXLAN_DROP_VNI; Same comment here. Maybe VXLAN_DROP_VNI_NOT_FOUND ? > goto drop; > + } > > /* For backwards compatibility, only allow reserved fields to be > * used by VXLAN extensions if explicitly requested. > @@ -1717,12 +1721,16 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > } > > if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto, > - !net_eq(vxlan->net, dev_net(vxlan->dev)))) > + !net_eq(vxlan->net, dev_net(vxlan->dev)))) { > + reason = SKB_DROP_REASON_NOMEM; > goto drop; > + } > > - if (vs->flags & VXLAN_F_REMCSUM_RX) > - if (unlikely(!vxlan_remcsum(&unparsed, skb, vs->flags))) > + if (vs->flags & VXLAN_F_REMCSUM_RX) { > + reason = vxlan_remcsum(&unparsed, skb, vs->flags); > + if (unlikely(reason != SKB_NOT_DROPPED_YET)) > goto drop; > + } > > if (vxlan_collect_metadata(vs)) { > IP_TUNNEL_DECLARE_FLAGS(flags) = { }; > @@ -1732,8 +1740,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), flags, > key32_to_tunnel_id(vni), sizeof(*md)); > > - if (!tun_dst) > + if (!tun_dst) { > + reason = SKB_DROP_REASON_NOMEM; > goto drop; > + } > > md = ip_tunnel_info_opts(&tun_dst->u.tun_info); > > @@ -1757,12 +1767,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > * is more robust and provides a little more security in > * adding extensions to VXLAN. > */ > + reason = (u32)VXLAN_DROP_FLAGS; > goto drop; > } > > if (!raw_proto) { > - if (!vxlan_set_mac(vxlan, vs, skb, vni)) > + if (!vxlan_set_mac(vxlan, vs, skb, vni)) { > + reason = (u32)VXLAN_DROP_MAC; The function drops the packet for various reasons: 1. Source MAC is equal to the VXLAN device's MAC 2. Source MAC is invalid (all zeroes or multicast) 3. Trying to migrate a static entry or one pointing to a nexthop They are all quite obscure so it might be fine to fold them under the same reason, but I can't find a descriptive name. If you split 1-2 to one reason and 3 to another, then they can become VXLAN_DROP_INVALID_SMAC and VXLAN_DROP_ENTRY_EXISTS > goto drop; > + }
On Tue, Aug 20, 2024 at 6:59 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 17 Aug 2024 19:33:23 +0800 Menglong Dong wrote: > > On Sat, Aug 17, 2024 at 10:22 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Thu, 15 Aug 2024 20:42:58 +0800 Menglong Dong wrote: > > > > #define VXLAN_DROP_REASONS(R) \ > > > > + R(VXLAN_DROP_FLAGS) \ > > > > + R(VXLAN_DROP_VNI) \ > > > > + R(VXLAN_DROP_MAC) \ > > > > > > Drop reasons should be documented. > > > > Yeah, I wrote the code here just like what we did in > > net/openvswitch/drop.h, which makes the definition of > > enum ovs_drop_reason a call of VXLAN_DROP_REASONS(). > > > > I think that we can define the enum ovs_drop_reason just like > > what we do in include/net/dropreason-core.h, which can make > > it easier to document the reasons. > > > > > I don't think name of a header field is a great fit for a reason. > > > > > > > Enn...Do you mean the "VXLAN_DROP_" prefix? > > No, I mean the thing after VXLAN_DROP_, it's FLAGS, VNI, MAC, > those are names of header fields. > Yeah, the reason here seems too simple. I use VXLAN_DROP_FLAGS for any dropping out of vxlan flags. Just like what Ido advised, we can use more descriptive reasons here, such as VXLAN_DROP_INVALID_HDR for FLAGS, VXLAN_DROP_NO_VNI for vni not found, etc. > > > > @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > > > return 0; > > > > > > > > drop: > > > > + SKB_DR_RESET(reason); > > > > > > the name of this macro is very confusing, I don't think it should exist > > > in the first place. nothing should goto drop without initialing reason > > > > > > > It's for the case that we call a function which returns drop reasons. > > For example, the reason now is assigned from: > > > > reason = pskb_may_pull_reason(skb, VXLAN_HLEN); > > if (reason) goto drop; > > > > xxxxxx > > if (xx) goto drop; > > > > The reason now is SKB_NOT_DROPPED_YET when we "goto drop", > > as we don't set a drop reason here, which is unnecessary in some cases. > > And, we can't set the drop reason for every "drop" code path, can we? > > Why? It's like saying "we can't set return code before jumping to > an error label". In my mind drop reasons and function return codes > are very similar. So IDK why we need all the SK_DR_ macros when > we are just fine without them for function return codes. Of course we can. In my example above, we need to set reason to SKB_DROP_REASON_NOT_SPECIFIED before we jump to an error label: reason = pskb_may_pull_reason(skb, VXLAN_HLEN); if (reason) goto drop; xxxxxx // we need to set reason here, or a WARN will be printed in // kfree_skb_reason(), as reason now is SKB_NOT_DROPPED_YET. reason = SKB_DROP_REASON_NOT_SPECIFIED; if (xx) goto drop; Should it be better to do it this way? Thanks! Menglong Dong
On Tue, Aug 20, 2024 at 8:27 PM Ido Schimmel <idosch@nvidia.com> wrote: > > On Thu, Aug 15, 2024 at 08:42:58PM +0800, Menglong Dong wrote: > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c > > index e971c4785962..9a61f04bb95d 100644 > > --- a/drivers/net/vxlan/vxlan_core.c > > +++ b/drivers/net/vxlan/vxlan_core.c > > @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph, > > /* Callback from net/ipv4/udp.c to receive packets */ > > static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > { > > + enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN); > > struct vxlan_vni_node *vninode = NULL; > > struct vxlan_dev *vxlan; > > struct vxlan_sock *vs; > > @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > int nh; > > > > /* Need UDP and VXLAN header to be present */ > > - if (!pskb_may_pull(skb, VXLAN_HLEN)) > > + if (reason != SKB_NOT_DROPPED_YET) > > goto drop; > > > > unparsed = *vxlan_hdr(skb); > > @@ -1690,6 +1691,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n", > > ntohl(vxlan_hdr(skb)->vx_flags), > > ntohl(vxlan_hdr(skb)->vx_vni)); > > + reason = (u32)VXLAN_DROP_FLAGS; > > I don't find "FLAGS" very descriptive. AFAICT the reason is used in > these two cases: > > 1. "I flag" is not set > 2. The reserved fields are not zero > > Maybe call it VXLAN_DROP_INVALID_HDR ? > Yeah, that makes sense. > And I agree with the comment about documenting these drop reasons like > in include/net/dropreason-core.h > Okay, I'm planning to do it this way. > > /* Return non vxlan pkt */ > > goto drop; > > } > > @@ -1703,8 +1705,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > vni = vxlan_vni(vxlan_hdr(skb)->vx_vni); > > > > vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode); > > - if (!vxlan) > > + if (!vxlan) { > > + reason = (u32)VXLAN_DROP_VNI; > > Same comment here. Maybe VXLAN_DROP_VNI_NOT_FOUND ? > Yeah, sounds nice. > > goto drop; > > + } > > > > /* For backwards compatibility, only allow reserved fields to be > > * used by VXLAN extensions if explicitly requested. > > @@ -1717,12 +1721,16 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > } > > > > if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto, > > - !net_eq(vxlan->net, dev_net(vxlan->dev)))) > > + !net_eq(vxlan->net, dev_net(vxlan->dev)))) { > > + reason = SKB_DROP_REASON_NOMEM; > > goto drop; > > + } > > > > - if (vs->flags & VXLAN_F_REMCSUM_RX) > > - if (unlikely(!vxlan_remcsum(&unparsed, skb, vs->flags))) > > + if (vs->flags & VXLAN_F_REMCSUM_RX) { > > + reason = vxlan_remcsum(&unparsed, skb, vs->flags); > > + if (unlikely(reason != SKB_NOT_DROPPED_YET)) > > goto drop; > > + } > > > > if (vxlan_collect_metadata(vs)) { > > IP_TUNNEL_DECLARE_FLAGS(flags) = { }; > > @@ -1732,8 +1740,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), flags, > > key32_to_tunnel_id(vni), sizeof(*md)); > > > > - if (!tun_dst) > > + if (!tun_dst) { > > + reason = SKB_DROP_REASON_NOMEM; > > goto drop; > > + } > > > > md = ip_tunnel_info_opts(&tun_dst->u.tun_info); > > > > @@ -1757,12 +1767,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > > * is more robust and provides a little more security in > > * adding extensions to VXLAN. > > */ > > + reason = (u32)VXLAN_DROP_FLAGS; > > goto drop; > > } > > > > if (!raw_proto) { > > - if (!vxlan_set_mac(vxlan, vs, skb, vni)) > > + if (!vxlan_set_mac(vxlan, vs, skb, vni)) { > > + reason = (u32)VXLAN_DROP_MAC; > > The function drops the packet for various reasons: > > 1. Source MAC is equal to the VXLAN device's MAC > 2. Source MAC is invalid (all zeroes or multicast) > 3. Trying to migrate a static entry or one pointing to a nexthop > > They are all quite obscure so it might be fine to fold them under the > same reason, but I can't find a descriptive name. > > If you split 1-2 to one reason and 3 to another, then they can become > VXLAN_DROP_INVALID_SMAC and VXLAN_DROP_ENTRY_EXISTS > Sounds great! Thanks for creating these names for me, I'm really not good at naming :/ > > goto drop; > > + }
diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h index 83e10550dd6a..cae1e0ea8c56 100644 --- a/drivers/net/vxlan/drop.h +++ b/drivers/net/vxlan/drop.h @@ -9,6 +9,9 @@ #include <net/dropreason.h> #define VXLAN_DROP_REASONS(R) \ + R(VXLAN_DROP_FLAGS) \ + R(VXLAN_DROP_VNI) \ + R(VXLAN_DROP_MAC) \ /* deliberate comment for trailing \ */ enum vxlan_drop_reason { diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c index e971c4785962..9a61f04bb95d 100644 --- a/drivers/net/vxlan/vxlan_core.c +++ b/drivers/net/vxlan/vxlan_core.c @@ -1668,6 +1668,7 @@ static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph, /* Callback from net/ipv4/udp.c to receive packets */ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) { + enum skb_drop_reason reason = pskb_may_pull_reason(skb, VXLAN_HLEN); struct vxlan_vni_node *vninode = NULL; struct vxlan_dev *vxlan; struct vxlan_sock *vs; @@ -1681,7 +1682,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) int nh; /* Need UDP and VXLAN header to be present */ - if (!pskb_may_pull(skb, VXLAN_HLEN)) + if (reason != SKB_NOT_DROPPED_YET) goto drop; unparsed = *vxlan_hdr(skb); @@ -1690,6 +1691,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n", ntohl(vxlan_hdr(skb)->vx_flags), ntohl(vxlan_hdr(skb)->vx_vni)); + reason = (u32)VXLAN_DROP_FLAGS; /* Return non vxlan pkt */ goto drop; } @@ -1703,8 +1705,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) vni = vxlan_vni(vxlan_hdr(skb)->vx_vni); vxlan = vxlan_vs_find_vni(vs, skb->dev->ifindex, vni, &vninode); - if (!vxlan) + if (!vxlan) { + reason = (u32)VXLAN_DROP_VNI; goto drop; + } /* For backwards compatibility, only allow reserved fields to be * used by VXLAN extensions if explicitly requested. @@ -1717,12 +1721,16 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) } if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto, - !net_eq(vxlan->net, dev_net(vxlan->dev)))) + !net_eq(vxlan->net, dev_net(vxlan->dev)))) { + reason = SKB_DROP_REASON_NOMEM; goto drop; + } - if (vs->flags & VXLAN_F_REMCSUM_RX) - if (unlikely(!vxlan_remcsum(&unparsed, skb, vs->flags))) + if (vs->flags & VXLAN_F_REMCSUM_RX) { + reason = vxlan_remcsum(&unparsed, skb, vs->flags); + if (unlikely(reason != SKB_NOT_DROPPED_YET)) goto drop; + } if (vxlan_collect_metadata(vs)) { IP_TUNNEL_DECLARE_FLAGS(flags) = { }; @@ -1732,8 +1740,10 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) tun_dst = udp_tun_rx_dst(skb, vxlan_get_sk_family(vs), flags, key32_to_tunnel_id(vni), sizeof(*md)); - if (!tun_dst) + if (!tun_dst) { + reason = SKB_DROP_REASON_NOMEM; goto drop; + } md = ip_tunnel_info_opts(&tun_dst->u.tun_info); @@ -1757,12 +1767,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) * is more robust and provides a little more security in * adding extensions to VXLAN. */ + reason = (u32)VXLAN_DROP_FLAGS; goto drop; } if (!raw_proto) { - if (!vxlan_set_mac(vxlan, vs, skb, vni)) + if (!vxlan_set_mac(vxlan, vs, skb, vni)) { + reason = (u32)VXLAN_DROP_MAC; goto drop; + } } else { skb_reset_mac_header(skb); skb->dev = vxlan->dev; @@ -1777,7 +1790,8 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) skb_reset_network_header(skb); - if (!pskb_inet_may_pull(skb)) { + reason = pskb_inet_may_pull_reason(skb); + if (reason != SKB_NOT_DROPPED_YET) { DEV_STATS_INC(vxlan->dev, rx_length_errors); DEV_STATS_INC(vxlan->dev, rx_errors); vxlan_vnifilter_count(vxlan, vni, vninode, @@ -1789,6 +1803,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) oiph = skb->head + nh; if (!vxlan_ecn_decapsulate(vs, oiph, skb)) { + reason = SKB_DROP_REASON_IP_TUNNEL_ECN; DEV_STATS_INC(vxlan->dev, rx_frame_errors); DEV_STATS_INC(vxlan->dev, rx_errors); vxlan_vnifilter_count(vxlan, vni, vninode, @@ -1803,6 +1818,7 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) dev_core_stats_rx_dropped_inc(vxlan->dev); vxlan_vnifilter_count(vxlan, vni, vninode, VXLAN_VNI_STATS_RX_DROPS, 0); + reason = SKB_DROP_REASON_DEV_READY; goto drop; } @@ -1815,8 +1831,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) return 0; drop: + SKB_DR_RESET(reason); /* Consume bad packet */ - kfree_skb(skb); + kfree_skb_reason(skb, reason); return 0; } diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h index 8da0129d1ed6..8388c0ae893d 100644 --- a/include/net/dropreason-core.h +++ b/include/net/dropreason-core.h @@ -92,6 +92,7 @@ FN(PACKET_SOCK_ERROR) \ FN(TC_CHAIN_NOTFOUND) \ FN(TC_RECLASSIFY_LOOP) \ + FN(IP_TUNNEL_ECN) \ FNe(MAX) /** @@ -418,6 +419,11 @@ enum skb_drop_reason { * iterations. */ SKB_DROP_REASON_TC_RECLASSIFY_LOOP, + /** + * @SKB_DROP_REASON_IP_TUNNEL_ECN: skb is dropped according to + * RFC 6040 4.2, see __INET_ECN_decapsulate() for detail. + */ + SKB_DROP_REASON_IP_TUNNEL_ECN, /** * @SKB_DROP_REASON_MAX: the maximum of core drop reasons, which * shouldn't be used as a real 'reason' - only for tracing code gen
Introduce skb drop reasons to the function vxlan_rcv(). Following new vxlan drop reasons are added: VXLAN_DROP_FLAGS VXLAN_DROP_VNI VXLAN_DROP_MAC And Following core skb drop reason is added: SKB_DROP_REASON_IP_TUNNEL_ECN As ip tunnel is a public module, I'm not sure how to deal with it. So I simply add it to the core drop reasons. Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> --- drivers/net/vxlan/drop.h | 3 +++ drivers/net/vxlan/vxlan_core.c | 35 +++++++++++++++++++++++++--------- include/net/dropreason-core.h | 6 ++++++ 3 files changed, 35 insertions(+), 9 deletions(-)