[PATCH net-next 00/10] net: vxlan: add skb drop reasons support

[PATCH net-next 00/10] net: vxlan: add skb drop reasons support

[Menglong Dong]

In this series, we add skb drop reasons to the vxlan module. After the
commit 071c0fc6fb91 ("net: extend drop reasons for multiple subsystems"),
we can add the skb drop reasons as a subsystem.

So, we now add a new skb drop reason subsystem for vxlan. I'm not sure
if it is better to add them directly to enum skb_drop_reason, as there
are only 6 new drop reasons that we introduce for vxlan:

  VXLAN_DROP_FLAGS
  VXLAN_DROP_VNI
  VXLAN_DROP_MAC
  VXLAN_DROP_TXINFO
  VXLAN_DROP_REMOTE
  VXLAN_DROP_REMOTE_IP

And we add the "SKB_DROP_REASON_IP_TUNNEL_ECN" to enum skb_drop_reason,
as it not only belongs to vxlan subsystem.

In order to reset the reason to NOT_SPECIFIED if it is
SKB_NOT_DROPPED_YET, we introduce the SKB_DR_RESET() in the 2nd patch.
This is to make sure that the skb is indeed dropped.

Menglong Dong (10):
  net: vxlan: add vxlan to the drop reason subsystem
  net: skb: add SKB_DR_RESET
  net: skb: introduce pskb_network_may_pull_reason()
  net: ip: introduce pskb_inet_may_pull_reason()
  net: vxlan: make vxlan_remcsum() return skb drop reasons
  net: vxlan: add skb drop reasons to vxlan_rcv()
  net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()
  net: vxlan: add drop reasons support to vxlan_xmit_one()
  net: vxlan: use kfree_skb_reason in vxlan_encap_bypass
  net: vxlan: use vxlan_kfree_skb in encap_bypass_if_local

 drivers/net/vxlan/drop.h          | 36 ++++++++++++
 drivers/net/vxlan/vxlan_core.c    | 91 +++++++++++++++++++++++--------
 drivers/net/vxlan/vxlan_private.h |  1 +
 include/linux/skbuff.h            |  8 ++-
 include/net/dropreason-core.h     |  7 +++
 include/net/dropreason.h          |  6 ++
 include/net/ip_tunnels.h          | 10 +++-
 7 files changed, 133 insertions(+), 26 deletions(-)
 create mode 100644 drivers/net/vxlan/drop.h

-- 
2.39.2

[PATCH net-next 01/10] net: vxlan: add vxlan to the drop reason subsystem

[Menglong Dong]

In this commit, we introduce the SKB_DROP_REASON_SUBSYS_VXLAN to make the
vxlan support skb drop reasons.

As the vxlan is a network protocol, maybe we can directly add it to the
enum skb_drop_reason instead of a subsystem?

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/drop.h          | 30 ++++++++++++++++++++++++++++++
 drivers/net/vxlan/vxlan_core.c    | 15 +++++++++++++++
 drivers/net/vxlan/vxlan_private.h |  1 +
 include/net/dropreason.h          |  6 ++++++
 4 files changed, 52 insertions(+)
 create mode 100644 drivers/net/vxlan/drop.h

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
new file mode 100644
index 000000000000..83e10550dd6a
--- /dev/null
+++ b/drivers/net/vxlan/drop.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * VXLAN drop reason list.
+ */
+
+#ifndef VXLAN_DROP_H
+#define VXLAN_DROP_H
+#include <linux/skbuff.h>
+#include <net/dropreason.h>
+
+#define VXLAN_DROP_REASONS(R)			\
+	/* deliberate comment for trailing \ */
+
+enum vxlan_drop_reason {
+	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
+				SKB_DROP_REASON_SUBSYS_SHIFT,
+#define ENUM(x) x,
+	VXLAN_DROP_REASONS(ENUM)
+#undef ENUM
+
+	VXLAN_DROP_MAX,
+};
+
+static inline void
+vxlan_kfree_skb(struct sk_buff *skb, enum vxlan_drop_reason reason)
+{
+	kfree_skb_reason(skb, (u32)reason);
+}
+
+#endif /* VXLAN_DROP_H */
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 8983e75e9881..5785902e20ce 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -4834,6 +4834,17 @@ static int vxlan_nexthop_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+static const char * const vxlan_drop_reasons[] = {
+#define S(x)	[(x) & ~SKB_DROP_REASON_SUBSYS_MASK] = (#x),
+	VXLAN_DROP_REASONS(S)
+#undef S
+};
+
+static struct drop_reason_list drop_reason_list_vxlan = {
+	.reasons = vxlan_drop_reasons,
+	.n_reasons = ARRAY_SIZE(vxlan_drop_reasons),
+};
+
 static __net_init int vxlan_init_net(struct net *net)
 {
 	struct vxlan_net *vn = net_generic(net, vxlan_net_id);
@@ -4915,6 +4926,9 @@ static int __init vxlan_init_module(void)
 
 	vxlan_vnifilter_init();
 
+	drop_reasons_register_subsys(SKB_DROP_REASON_SUBSYS_VXLAN,
+				     &drop_reason_list_vxlan);
+
 	return 0;
 out4:
 	unregister_switchdev_notifier(&vxlan_switchdev_notifier_block);
@@ -4929,6 +4943,7 @@ late_initcall(vxlan_init_module);
 
 static void __exit vxlan_cleanup_module(void)
 {
+	drop_reasons_unregister_subsys(SKB_DROP_REASON_SUBSYS_VXLAN);
 	vxlan_vnifilter_uninit();
 	rtnl_link_unregister(&vxlan_link_ops);
 	unregister_switchdev_notifier(&vxlan_switchdev_notifier_block);
diff --git a/drivers/net/vxlan/vxlan_private.h b/drivers/net/vxlan/vxlan_private.h
index b35d96b78843..8720d7a1206f 100644
--- a/drivers/net/vxlan/vxlan_private.h
+++ b/drivers/net/vxlan/vxlan_private.h
@@ -8,6 +8,7 @@
 #define _VXLAN_PRIVATE_H
 
 #include <linux/rhashtable.h>
+#include "drop.h"
 
 extern unsigned int vxlan_net_id;
 extern const u8 all_zeros_mac[ETH_ALEN + 2];
diff --git a/include/net/dropreason.h b/include/net/dropreason.h
index 56cb7be92244..2e5d158d670e 100644
--- a/include/net/dropreason.h
+++ b/include/net/dropreason.h
@@ -29,6 +29,12 @@ enum skb_drop_reason_subsys {
 	 */
 	SKB_DROP_REASON_SUBSYS_OPENVSWITCH,
 
+	/**
+	 * @SKB_DROP_REASON_SUBSYS_VXLAN: vxlan drop reason, see
+	 * drivers/net/vxlan/drop.h
+	 */
+	SKB_DROP_REASON_SUBSYS_VXLAN,
+
 	/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
 	SKB_DROP_REASON_SUBSYS_NUM
 };
-- 
2.39.2

[PATCH net-next 02/10] net: skb: add SKB_DR_RESET

[Menglong Dong]

For now, the skb drop reason can be SKB_NOT_DROPPED_YET, which makes the
kfree_skb_reason call consume_skb. In some case, we need to make sure that
kfree_skb is called, which means the reason can't be SKB_NOT_DROPPED_YET.
Introduce SKB_DR_RESET() to reset the reason to NOT_SPECIFIED if it is
SKB_NOT_DROPPED_YET.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 include/net/dropreason-core.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 9707ab54fdd5..8da0129d1ed6 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -445,5 +445,6 @@ enum skb_drop_reason {
 		    name == SKB_NOT_DROPPED_YET)		\
 			SKB_DR_SET(name, reason);		\
 	} while (0)
+#define SKB_DR_RESET(name) SKB_DR_OR(name, NOT_SPECIFIED)
 
 #endif
-- 
2.39.2

Re: [PATCH net-next 02/10] net: skb: add SKB_DR_RESET

[Ido Schimmel]

On Thu, Aug 15, 2024 at 08:42:54PM +0800, Menglong Dong wrote:
> For now, the skb drop reason can be SKB_NOT_DROPPED_YET, which makes the
> kfree_skb_reason call consume_skb.

Maybe I'm missing something, but kfree_skb_reason() only calls
trace_consume_skb() if reason is SKB_CONSUMED. With SKB_NOT_DROPPED_YET
you will get a warning.

> In some case, we need to make sure that
> kfree_skb is called, which means the reason can't be SKB_NOT_DROPPED_YET.
> Introduce SKB_DR_RESET() to reset the reason to NOT_SPECIFIED if it is
> SKB_NOT_DROPPED_YET.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  include/net/dropreason-core.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> index 9707ab54fdd5..8da0129d1ed6 100644
> --- a/include/net/dropreason-core.h
> +++ b/include/net/dropreason-core.h
> @@ -445,5 +445,6 @@ enum skb_drop_reason {
>  		    name == SKB_NOT_DROPPED_YET)		\
>  			SKB_DR_SET(name, reason);		\
>  	} while (0)
> +#define SKB_DR_RESET(name) SKB_DR_OR(name, NOT_SPECIFIED)
>  
>  #endif
> -- 
> 2.39.2
> 

Re: [PATCH net-next 02/10] net: skb: add SKB_DR_RESET

[Menglong Dong]

On Tue, Aug 20, 2024 at 8:24 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Thu, Aug 15, 2024 at 08:42:54PM +0800, Menglong Dong wrote:
> > For now, the skb drop reason can be SKB_NOT_DROPPED_YET, which makes the
> > kfree_skb_reason call consume_skb.
>
> Maybe I'm missing something, but kfree_skb_reason() only calls
> trace_consume_skb() if reason is SKB_CONSUMED. With SKB_NOT_DROPPED_YET
> you will get a warning.
>

Yeah, I use this macro to avoid such WARNING. And I'm not sure
if we need this patch now :/

> > In some case, we need to make sure that
> > kfree_skb is called, which means the reason can't be SKB_NOT_DROPPED_YET.
> > Introduce SKB_DR_RESET() to reset the reason to NOT_SPECIFIED if it is
> > SKB_NOT_DROPPED_YET.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  include/net/dropreason-core.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
> > index 9707ab54fdd5..8da0129d1ed6 100644
> > --- a/include/net/dropreason-core.h
> > +++ b/include/net/dropreason-core.h
> > @@ -445,5 +445,6 @@ enum skb_drop_reason {
> >                   name == SKB_NOT_DROPPED_YET)                \
> >                       SKB_DR_SET(name, reason);               \
> >       } while (0)
> > +#define SKB_DR_RESET(name) SKB_DR_OR(name, NOT_SPECIFIED)
> >
> >  #endif
> > --
> > 2.39.2
> >

[PATCH net-next 03/10] net: skb: introduce pskb_network_may_pull_reason()

[Menglong Dong]

Introduce the function pskb_network_may_pull_reason() and make
pskb_network_may_pull() a simple inline call to it.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 include/linux/skbuff.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cf8f6ce06742..fe6f97b550fc 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3114,9 +3114,15 @@ static inline int skb_inner_network_offset(const struct sk_buff *skb)
 	return skb_inner_network_header(skb) - skb->data;
 }
 
+static inline enum skb_drop_reason
+pskb_network_may_pull_reason(struct sk_buff *skb, unsigned int len)
+{
+	return pskb_may_pull_reason(skb, skb_network_offset(skb) + len);
+}
+
 static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len)
 {
-	return pskb_may_pull(skb, skb_network_offset(skb) + len);
+	return pskb_network_may_pull_reason(skb, len) == SKB_NOT_DROPPED_YET;
 }
 
 /*
-- 
2.39.2

[PATCH net-next 04/10] net: ip: introduce pskb_inet_may_pull_reason()

[Menglong Dong]

Introduce the function pskb_inet_may_pull_reason() and make
pskb_inet_may_pull a simple inline call to it.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 include/net/ip_tunnels.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 1db2417b8ff5..12992aa792fc 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -439,7 +439,8 @@ int ip_tunnel_encap_del_ops(const struct ip_tunnel_encap_ops *op,
 int ip_tunnel_encap_setup(struct ip_tunnel *t,
 			  struct ip_tunnel_encap *ipencap);
 
-static inline bool pskb_inet_may_pull(struct sk_buff *skb)
+static inline enum skb_drop_reason
+pskb_inet_may_pull_reason(struct sk_buff *skb)
 {
 	int nhlen;
 
@@ -456,7 +457,12 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 		nhlen = 0;
 	}
 
-	return pskb_network_may_pull(skb, nhlen);
+	return pskb_network_may_pull_reason(skb, nhlen);
+}
+
+static inline bool pskb_inet_may_pull(struct sk_buff *skb)
+{
+	return pskb_inet_may_pull_reason(skb) == SKB_NOT_DROPPED_YET;
 }
 
 /* Variant of pskb_inet_may_pull().
-- 
2.39.2

[PATCH net-next 05/10] net: vxlan: make vxlan_remcsum() return skb drop reasons

[Menglong Dong]

Make vxlan_remcsum() support skb drop reasons by changing the return
value type of it from bool to enum skb_drop_reason.

The only drop reason in vxlan_remcsum() comes from pskb_may_pull_reason(),
so we just return it.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/vxlan_core.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 5785902e20ce..e971c4785962 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1552,9 +1552,11 @@ static void vxlan_sock_release(struct vxlan_dev *vxlan)
 #endif
 }
 
-static bool vxlan_remcsum(struct vxlanhdr *unparsed,
-			  struct sk_buff *skb, u32 vxflags)
+static enum skb_drop_reason vxlan_remcsum(struct vxlanhdr *unparsed,
+					  struct sk_buff *skb,
+					  u32 vxflags)
 {
+	enum skb_drop_reason reason;
 	size_t start, offset;
 
 	if (!(unparsed->vx_flags & VXLAN_HF_RCO) || skb->remcsum_offload)
@@ -1563,15 +1565,16 @@ static bool vxlan_remcsum(struct vxlanhdr *unparsed,
 	start = vxlan_rco_start(unparsed->vx_vni);
 	offset = start + vxlan_rco_offset(unparsed->vx_vni);
 
-	if (!pskb_may_pull(skb, offset + sizeof(u16)))
-		return false;
+	reason = pskb_may_pull_reason(skb, offset + sizeof(u16));
+	if (reason != SKB_NOT_DROPPED_YET)
+		return reason;
 
 	skb_remcsum_process(skb, (void *)(vxlan_hdr(skb) + 1), start, offset,
 			    !!(vxflags & VXLAN_F_REMCSUM_NOPARTIAL));
 out:
 	unparsed->vx_flags &= ~VXLAN_HF_RCO;
 	unparsed->vx_vni &= VXLAN_VNI_MASK;
-	return true;
+	return SKB_NOT_DROPPED_YET;
 }
 
 static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
-- 
2.39.2

[PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Menglong Dong]

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(-)

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
-- 
2.39.2

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Jakub Kicinski]

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;
>  }

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Menglong Dong]

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;
> >  }

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Jakub Kicinski]

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.

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Menglong Dong]

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

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[Ido Schimmel]

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;
> +		}

Re: [PATCH net-next 06/10] net: vxlan: add skb drop reasons to vxlan_rcv()

[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;
> > +             }

[PATCH net-next 07/10] net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()

[Menglong Dong]

Replace kfree_skb() with vxlan_kfree_skb() in vxlan_xmit(). Following
new skb drop reasons are introduced:

/* txinfo is missed in "external" mode */
VXLAN_DROP_TXINFO
/* no remote found */
VXLAN_DROP_REMOTE

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/drop.h       | 2 ++
 drivers/net/vxlan/vxlan_core.c | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index cae1e0ea8c56..da30cb4a9ed9 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -12,6 +12,8 @@
 	R(VXLAN_DROP_FLAGS)			\
 	R(VXLAN_DROP_VNI)			\
 	R(VXLAN_DROP_MAC)			\
+	R(VXLAN_DROP_TXINFO)			\
+	R(VXLAN_DROP_REMOTE)			\
 	/* 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 9a61f04bb95d..22e2bf532ac3 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2729,7 +2729,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 			if (info && info->mode & IP_TUNNEL_INFO_TX)
 				vxlan_xmit_one(skb, dev, vni, NULL, false);
 			else
-				kfree_skb(skb);
+				vxlan_kfree_skb(skb, VXLAN_DROP_TXINFO);
 			return NETDEV_TX_OK;
 		}
 	}
@@ -2792,7 +2792,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 			dev_core_stats_tx_dropped_inc(dev);
 			vxlan_vnifilter_count(vxlan, vni, NULL,
 					      VXLAN_VNI_STATS_TX_DROPS, 0);
-			kfree_skb(skb);
+			vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);
 			return NETDEV_TX_OK;
 		}
 	}
@@ -2815,7 +2815,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (fdst)
 			vxlan_xmit_one(skb, dev, vni, fdst, did_rsc);
 		else
-			kfree_skb(skb);
+			vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);
 	}
 
 	return NETDEV_TX_OK;
-- 
2.39.2

Re: [PATCH net-next 07/10] net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()

[Ido Schimmel]

On Thu, Aug 15, 2024 at 08:42:59PM +0800, Menglong Dong wrote:
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 9a61f04bb95d..22e2bf532ac3 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2729,7 +2729,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  			if (info && info->mode & IP_TUNNEL_INFO_TX)
>  				vxlan_xmit_one(skb, dev, vni, NULL, false);
>  			else
> -				kfree_skb(skb);
> +				vxlan_kfree_skb(skb, VXLAN_DROP_TXINFO);

This one probably belongs in include/net/dropreason-core.h as there are
other devices that support tunnel info with similar checks.

>  			return NETDEV_TX_OK;
>  		}
>  	}
> @@ -2792,7 +2792,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  			dev_core_stats_tx_dropped_inc(dev);
>  			vxlan_vnifilter_count(vxlan, vni, NULL,
>  					      VXLAN_VNI_STATS_TX_DROPS, 0);
> -			kfree_skb(skb);
> +			vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);
>  			return NETDEV_TX_OK;
>  		}
>  	}
> @@ -2815,7 +2815,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>  		if (fdst)
>  			vxlan_xmit_one(skb, dev, vni, fdst, did_rsc);
>  		else
> -			kfree_skb(skb);
> +			vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);

Maybe VXLAN_DROP_NO_REMOTE? Please add it to vxlan_mdb_xmit() as well

>  	}
>  
>  	return NETDEV_TX_OK;
> -- 
> 2.39.2
> 

Re: [PATCH net-next 07/10] net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()

[Menglong Dong]

On Tue, Aug 20, 2024 at 8:29 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Thu, Aug 15, 2024 at 08:42:59PM +0800, Menglong Dong wrote:
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index 9a61f04bb95d..22e2bf532ac3 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -2729,7 +2729,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       if (info && info->mode & IP_TUNNEL_INFO_TX)
> >                               vxlan_xmit_one(skb, dev, vni, NULL, false);
> >                       else
> > -                             kfree_skb(skb);
> > +                             vxlan_kfree_skb(skb, VXLAN_DROP_TXINFO);
>
> This one probably belongs in include/net/dropreason-core.h as there are
> other devices that support tunnel info with similar checks.
>

OK, I'll add it to the drop reason core as SKB_DROP_REASON_TUNNEL_TXINFO.

> >                       return NETDEV_TX_OK;
> >               }
> >       }
> > @@ -2792,7 +2792,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       dev_core_stats_tx_dropped_inc(dev);
> >                       vxlan_vnifilter_count(vxlan, vni, NULL,
> >                                             VXLAN_VNI_STATS_TX_DROPS, 0);
> > -                     kfree_skb(skb);
> > +                     vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);
> >                       return NETDEV_TX_OK;
> >               }
> >       }
> > @@ -2815,7 +2815,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
> >               if (fdst)
> >                       vxlan_xmit_one(skb, dev, vni, fdst, did_rsc);
> >               else
> > -                     kfree_skb(skb);
> > +                     vxlan_kfree_skb(skb, VXLAN_DROP_REMOTE);
>
> Maybe VXLAN_DROP_NO_REMOTE? Please add it to vxlan_mdb_xmit() as well
>

Okay!

> >       }
> >
> >       return NETDEV_TX_OK;
> > --
> > 2.39.2
> >

[PATCH net-next 08/10] net: vxlan: add drop reasons support to vxlan_xmit_one()

[Menglong Dong]

Replace kfree_skb/dev_kfree_skb with kfree_skb_reason in vxlan_xmit_one.
The new skb drop reason "VXLAN_DROP_REMOTE_IP" is introduced, which is
for a invalid remote ip.

The only concern of mine is replacing dev_kfree_skb with
kfree_skb_reason. The dev_kfree_skb is equal to consume_skb, and I'm not
sure if we can change it to kfree_skb here. In my option, the skb is
"dropped" here, isn't it?

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/drop.h       |  1 +
 drivers/net/vxlan/vxlan_core.c | 18 ++++++++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index da30cb4a9ed9..542f391b1273 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -14,6 +14,7 @@
 	R(VXLAN_DROP_MAC)			\
 	R(VXLAN_DROP_TXINFO)			\
 	R(VXLAN_DROP_REMOTE)			\
+	R(VXLAN_DROP_REMOTE_IP)			\
 	/* 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 22e2bf532ac3..c1bae120727f 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2375,6 +2375,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	bool xnet = !net_eq(vxlan->net, dev_net(vxlan->dev));
 	bool no_eth_encap;
 	__be32 vni = 0;
+	SKB_DR(reason);
 
 	no_eth_encap = flags & VXLAN_F_GPE && skb->protocol != htons(ETH_P_TEB);
 	if (!skb_vlan_inet_prepare(skb, no_eth_encap))
@@ -2396,6 +2397,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 						   default_vni, true);
 				return;
 			}
+			reason = (u32)VXLAN_DROP_REMOTE_IP;
 			goto drop;
 		}
 
@@ -2483,6 +2485,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 					   tos, use_cache ? dst_cache : NULL);
 		if (IS_ERR(rt)) {
 			err = PTR_ERR(rt);
+			reason = SKB_DROP_REASON_IP_OUTNOROUTES;
 			goto tx_error;
 		}
 
@@ -2534,8 +2537,10 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
 		err = vxlan_build_skb(skb, ndst, sizeof(struct iphdr),
 				      vni, md, flags, udp_sum);
-		if (err < 0)
+		if (err < 0) {
+			reason = SKB_DROP_REASON_NOMEM;
 			goto tx_error;
+		}
 
 		udp_tunnel_xmit_skb(rt, sock4->sock->sk, skb, saddr,
 				    pkey->u.ipv4.dst, tos, ttl, df,
@@ -2555,6 +2560,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		if (IS_ERR(ndst)) {
 			err = PTR_ERR(ndst);
 			ndst = NULL;
+			reason = SKB_DROP_REASON_IP_OUTNOROUTES;
 			goto tx_error;
 		}
 
@@ -2595,8 +2601,10 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 		skb_scrub_packet(skb, xnet);
 		err = vxlan_build_skb(skb, ndst, sizeof(struct ipv6hdr),
 				      vni, md, flags, udp_sum);
-		if (err < 0)
+		if (err < 0) {
+			reason = SKB_DROP_REASON_NOMEM;
 			goto tx_error;
+		}
 
 		udp_tunnel6_xmit_skb(ndst, sock6->sock->sk, skb, dev,
 				     &saddr, &pkey->u.ipv6.dst, tos, ttl,
@@ -2611,7 +2619,8 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 drop:
 	dev_core_stats_tx_dropped_inc(dev);
 	vxlan_vnifilter_count(vxlan, vni, NULL, VXLAN_VNI_STATS_TX_DROPS, 0);
-	dev_kfree_skb(skb);
+	SKB_DR_RESET(reason);
+	kfree_skb_reason(skb, reason);
 	return;
 
 tx_error:
@@ -2623,7 +2632,8 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	dst_release(ndst);
 	DEV_STATS_INC(dev, tx_errors);
 	vxlan_vnifilter_count(vxlan, vni, NULL, VXLAN_VNI_STATS_TX_ERRORS, 0);
-	kfree_skb(skb);
+	SKB_DR_RESET(reason);
+	kfree_skb_reason(skb, reason);
 }
 
 static void vxlan_xmit_nh(struct sk_buff *skb, struct net_device *dev,
-- 
2.39.2

Re: [PATCH net-next 08/10] net: vxlan: add drop reasons support to vxlan_xmit_one()

[Ido Schimmel]

On Thu, Aug 15, 2024 at 08:43:00PM +0800, Menglong Dong wrote:
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index da30cb4a9ed9..542f391b1273 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -14,6 +14,7 @@
>  	R(VXLAN_DROP_MAC)			\
>  	R(VXLAN_DROP_TXINFO)			\
>  	R(VXLAN_DROP_REMOTE)			\
> +	R(VXLAN_DROP_REMOTE_IP)			\
>  	/* 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 22e2bf532ac3..c1bae120727f 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -2375,6 +2375,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  	bool xnet = !net_eq(vxlan->net, dev_net(vxlan->dev));
>  	bool no_eth_encap;
>  	__be32 vni = 0;
> +	SKB_DR(reason);
>  
>  	no_eth_encap = flags & VXLAN_F_GPE && skb->protocol != htons(ETH_P_TEB);
>  	if (!skb_vlan_inet_prepare(skb, no_eth_encap))
> @@ -2396,6 +2397,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
>  						   default_vni, true);
>  				return;
>  			}
> +			reason = (u32)VXLAN_DROP_REMOTE_IP;

This looks quite obscure to me. I didn't know you can add 0.0.0.0 as
remote and I'm not sure what is the use case. Personally I wouldn't
bother with this reason.

>  			goto drop;
>  		}

Re: [PATCH net-next 08/10] net: vxlan: add drop reasons support to vxlan_xmit_one()

[Menglong Dong]

On Tue, Aug 20, 2024 at 8:33 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Thu, Aug 15, 2024 at 08:43:00PM +0800, Menglong Dong wrote:
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index da30cb4a9ed9..542f391b1273 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -14,6 +14,7 @@
> >       R(VXLAN_DROP_MAC)                       \
> >       R(VXLAN_DROP_TXINFO)                    \
> >       R(VXLAN_DROP_REMOTE)                    \
> > +     R(VXLAN_DROP_REMOTE_IP)                 \
> >       /* 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 22e2bf532ac3..c1bae120727f 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -2375,6 +2375,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
> >       bool xnet = !net_eq(vxlan->net, dev_net(vxlan->dev));
> >       bool no_eth_encap;
> >       __be32 vni = 0;
> > +     SKB_DR(reason);
> >
> >       no_eth_encap = flags & VXLAN_F_GPE && skb->protocol != htons(ETH_P_TEB);
> >       if (!skb_vlan_inet_prepare(skb, no_eth_encap))
> > @@ -2396,6 +2397,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
> >                                                  default_vni, true);
> >                               return;
> >                       }
> > +                     reason = (u32)VXLAN_DROP_REMOTE_IP;
>
> This looks quite obscure to me. I didn't know you can add 0.0.0.0 as
> remote and I'm not sure what is the use case. Personally I wouldn't
> bother with this reason.
>

I know. I'm hesitant about this commit, as I don't find a case
for this dropping, and the remote seems can't be 0.0.0.0.
I add this reason as the code drops the packet here.

Enn...I will abandon this commit.

Thanks!
Menglong Dong

> >                       goto drop;
> >               }

[PATCH net-next 09/10] net: vxlan: use kfree_skb_reason in vxlan_encap_bypass

[Menglong Dong]

Replace kfree_skb with kfree_skb_reason in vxlan_encap_bypass, and no new
skb drop reason is added in this commit.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/vxlan_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index c1bae120727f..885aa956b5c2 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2289,7 +2289,7 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan,
 	rcu_read_lock();
 	dev = skb->dev;
 	if (unlikely(!(dev->flags & IFF_UP))) {
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_DEV_READY);
 		goto drop;
 	}
 
-- 
2.39.2

[PATCH net-next 10/10] net: vxlan: use vxlan_kfree_skb in encap_bypass_if_local

[Menglong Dong]

Replace kfree_skb with vxlan_kfree_skb in encap_bypass_if_local, and no
new skb drop reason is added in this commit.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/vxlan_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 885aa956b5c2..87d7eca49e20 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2340,7 +2340,7 @@ static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
 			DEV_STATS_INC(dev, tx_errors);
 			vxlan_vnifilter_count(vxlan, vni, NULL,
 					      VXLAN_VNI_STATS_TX_ERRORS, 0);
-			kfree_skb(skb);
+			vxlan_kfree_skb(skb, VXLAN_DROP_VNI);
 
 			return -ENOENT;
 		}
-- 
2.39.2