On Tue, Aug 20, 2024 at 8:27 PM Ido Schimmel [off-list ref] wrote:

On Thu, Aug 15, 2024 at 08:42:58PM +0800, Menglong Dong wrote:

quoted

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.

quoted

              /* 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.

quoted

              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 :/

quoted

                      goto drop;
+             }