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

[PATCH net-next v2 00/12] 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, and following
new skb drop reasons are introduced to vxlan:

  VXLAN_DROP_INVALID_SMAC
  VXLAN_DROP_ENTRY_EXISTS
  VXLAN_DROP_INVALID_HDR
  VXLAN_DROP_VNI_NOT_FOUND
  VXLAN_DROP_NO_REMOTE

And we add the following drop reasons to enum skb_drop_reason:

  IP_TUNNEL_ECN
  TUNNEL_TXINFO

Changes since v1:
- document all the drop reaons that we introduce
- rename the drop reasons to make them more descriptive, as Ido advised
- remove the 2nd patch, which introduce the SKB_DR_RESET
- add the 4th patch, which adds skb_vlan_inet_prepare_reason() helper
- introduce the 6th patch, which make vxlan_set_mac return drop reasons
- introduce the 10th patch, which uses VXLAN_DROP_NO_REMOTE as the drop
  reasons, as Ido advised

Menglong Dong (12):
  net: vxlan: add vxlan to the drop reason subsystem
  net: skb: add pskb_network_may_pull_reason() helper
  net: tunnel: add pskb_inet_may_pull_reason() helper
  net: tunnel: add skb_vlan_inet_prepare_reason() helper
  net: vxlan: make vxlan_remcsum() return drop reasons
  net: vxlan: make vxlan_set_mac() return 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_mdb_xmit
  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          |  47 ++++++++++++++
 drivers/net/vxlan/vxlan_core.c    | 104 +++++++++++++++++++++---------
 drivers/net/vxlan/vxlan_mdb.c     |   2 +-
 drivers/net/vxlan/vxlan_private.h |   1 +
 include/linux/skbuff.h            |   8 ++-
 include/net/dropreason-core.h     |   9 +++
 include/net/dropreason.h          |   6 ++
 include/net/ip_tunnels.h          |  31 ++++++---
 8 files changed, 168 insertions(+), 40 deletions(-)
 create mode 100644 drivers/net/vxlan/drop.h

-- 
2.39.2

[PATCH net-next v2 01/12] 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.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 drivers/net/vxlan/drop.h          | 25 +++++++++++++++++++++++++
 drivers/net/vxlan/vxlan_core.c    | 15 +++++++++++++++
 drivers/net/vxlan/vxlan_private.h |  1 +
 include/net/dropreason.h          |  6 ++++++
 4 files changed, 47 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..6bcc6894fbbd
--- /dev/null
+++ b/drivers/net/vxlan/drop.h
@@ -0,0 +1,25 @@
+/* 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,
+};
+
+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 34391c18bba7..fcd224a1d0c0 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -4833,6 +4833,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);
@@ -4914,6 +4925,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);
@@ -4928,6 +4942,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 v2 02/12] net: skb: add pskb_network_may_pull_reason() helper

[Menglong Dong]

Introduce the function pskb_network_may_pull_reason() and make
pskb_network_may_pull() a simple inline call to it. The drop reasons of
it just come from pskb_may_pull_reason.

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 v2 03/12] net: tunnel: add pskb_inet_may_pull_reason() helper

[Menglong Dong]

Introduce the function pskb_inet_may_pull_reason() and make
pskb_inet_may_pull a simple inline call to it. The drop reasons of it just
come from pskb_may_pull_reason().

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 6194fbb564c6..7fc2f7bf837a 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 v2 04/12] net: tunnel: add skb_vlan_inet_prepare_reason() helper

[Menglong Dong]

Introduce the function skb_vlan_inet_prepare_reason() and make
skb_vlan_inet_prepare an inline call to it. The drop reasons of it just
come from pskb_may_pull_reason.

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

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 7fc2f7bf837a..90f8d1510a76 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -465,13 +465,14 @@ 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().
+/* Variant of pskb_inet_may_pull_reason().
  */
-static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
-					 bool inner_proto_inherit)
+static inline enum skb_drop_reason
+skb_vlan_inet_prepare_reason(struct sk_buff *skb, bool inner_proto_inherit)
 {
 	int nhlen = 0, maclen = inner_proto_inherit ? 0 : ETH_HLEN;
 	__be16 type = skb->protocol;
+	enum skb_drop_reason reason;
 
 	/* Essentially this is skb_protocol(skb, true)
 	 * And we get MAC len.
@@ -492,11 +493,19 @@ static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
 	/* For ETH_P_IPV6/ETH_P_IP we make sure to pull
 	 * a base network header in skb->head.
 	 */
-	if (!pskb_may_pull(skb, maclen + nhlen))
-		return false;
+	reason = pskb_may_pull_reason(skb, maclen + nhlen);
+	if (reason)
+		return reason;
 
 	skb_set_network_header(skb, maclen);
-	return true;
+	return SKB_NOT_DROPPED_YET;
+}
+
+static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
+					 bool inner_proto_inherit)
+{
+	return skb_vlan_inet_prepare_reason(skb, inner_proto_inherit) ==
+		SKB_NOT_DROPPED_YET;
 }
 
 static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
-- 
2.39.2

[PATCH net-next v2 05/12] net: vxlan: make vxlan_remcsum() return 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 fcd224a1d0c0..76b217d166ef 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1551,9 +1551,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)
@@ -1562,15 +1564,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)
+		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 v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Menglong Dong]

Change the return type of vxlan_set_mac() from bool to enum
skb_drop_reason. In this commit, two drop reasons are introduced:

  VXLAN_DROP_INVALID_SMAC
  VXLAN_DROP_ENTRY_EXISTS

To make it easier to document the reasons in drivers/net/vxlan/drop.h,
we don't define the enum vxlan_drop_reason with the macro
VXLAN_DROP_REASONS(), but hand by hand.

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

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index 6bcc6894fbbd..876b4a9de92f 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -9,11 +9,20 @@
 #include <net/dropreason.h>
 
 #define VXLAN_DROP_REASONS(R)			\
+	R(VXLAN_DROP_INVALID_SMAC)		\
+	R(VXLAN_DROP_ENTRY_EXISTS)		\
 	/* deliberate comment for trailing \ */
 
 enum vxlan_drop_reason {
 	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
 				SKB_DROP_REASON_SUBSYS_SHIFT,
+	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
+	VXLAN_DROP_INVALID_SMAC,
+	/**
+	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
+	 * one pointing to a nexthop
+	 */
+	VXLAN_DROP_ENTRY_EXISTS,
 };
 
 static inline void
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 76b217d166ef..58c175432a15 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
 	unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
 }
 
-static bool vxlan_set_mac(struct vxlan_dev *vxlan,
-			  struct vxlan_sock *vs,
-			  struct sk_buff *skb, __be32 vni)
+static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
+					  struct vxlan_sock *vs,
+					  struct sk_buff *skb, __be32 vni)
 {
 	union vxlan_addr saddr;
 	u32 ifindex = skb->dev->ifindex;
@@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
 
 	/* Ignore packet loops (and multicast echo) */
 	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
-		return false;
+		return (u32)VXLAN_DROP_INVALID_SMAC;
 
 	/* Get address from the outer IP header */
 	if (vxlan_get_sk_family(vs) == AF_INET) {
@@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
 
 	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
 	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
-		return false;
+		return (u32)VXLAN_DROP_ENTRY_EXISTS;
 
-	return true;
+	return (u32)SKB_NOT_DROPPED_YET;
 }
 
 static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
-- 
2.39.2

[PATCH net-next v2 07/12] 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_INVALID_HDR
  VXLAN_DROP_VNI_NOT_FOUND

And Following core skb drop reason is added:

  SKB_DROP_REASON_IP_TUNNEL_ECN

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
v2:
- rename the drop reasons, as Ido advised.
- document the drop reasons
---
 drivers/net/vxlan/drop.h       | 10 ++++++++++
 drivers/net/vxlan/vxlan_core.c | 35 +++++++++++++++++++++++++---------
 include/net/dropreason-core.h  |  6 ++++++
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index 876b4a9de92f..416532633881 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -11,6 +11,8 @@
 #define VXLAN_DROP_REASONS(R)			\
 	R(VXLAN_DROP_INVALID_SMAC)		\
 	R(VXLAN_DROP_ENTRY_EXISTS)		\
+	R(VXLAN_DROP_INVALID_HDR)		\
+	R(VXLAN_DROP_VNI_NOT_FOUND)		\
 	/* deliberate comment for trailing \ */
 
 enum vxlan_drop_reason {
@@ -23,6 +25,14 @@ enum vxlan_drop_reason {
 	 * one pointing to a nexthop
 	 */
 	VXLAN_DROP_ENTRY_EXISTS,
+	/**
+	 * @VXLAN_DROP_INVALID_HDR: the vxlan header is invalid, such as:
+	 * 1) the reserved fields are not zero
+	 * 2) the "I" flag is not set
+	 */
+	VXLAN_DROP_INVALID_HDR,
+	/** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
+	VXLAN_DROP_VNI_NOT_FOUND,
 };
 
 static inline void
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 58c175432a15..ab1c14a807f2 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1674,13 +1674,15 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 	struct vxlan_metadata _md;
 	struct vxlan_metadata *md = &_md;
 	__be16 protocol = htons(ETH_P_TEB);
+	enum skb_drop_reason reason;
 	bool raw_proto = false;
 	void *oiph;
 	__be32 vni = 0;
 	int nh;
 
 	/* Need UDP and VXLAN header to be present */
-	if (!pskb_may_pull(skb, VXLAN_HLEN))
+	reason = pskb_may_pull_reason(skb, VXLAN_HLEN);
+	if (reason)
 		goto drop;
 
 	unparsed = *vxlan_hdr(skb);
@@ -1689,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_INVALID_HDR;
 		/* Return non vxlan pkt */
 		goto drop;
 	}
@@ -1702,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_NOT_FOUND;
 		goto drop;
+	}
 
 	/* For backwards compatibility, only allow reserved fields to be
 	 * used by VXLAN extensions if explicitly requested.
@@ -1716,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))
 			goto drop;
+	}
 
 	if (vxlan_collect_metadata(vs)) {
 		IP_TUNNEL_DECLARE_FLAGS(flags) = { };
@@ -1731,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);
 
@@ -1756,11 +1767,13 @@ 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_INVALID_HDR;
 		goto drop;
 	}
 
 	if (!raw_proto) {
-		if (!vxlan_set_mac(vxlan, vs, skb, vni))
+		reason = vxlan_set_mac(vxlan, vs, skb, vni);
+		if (reason)
 			goto drop;
 	} else {
 		skb_reset_mac_header(skb);
@@ -1776,7 +1789,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) {
 		DEV_STATS_INC(vxlan->dev, rx_length_errors);
 		DEV_STATS_INC(vxlan->dev, rx_errors);
 		vxlan_vnifilter_count(vxlan, vni, vninode,
@@ -1788,6 +1802,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,
@@ -1802,6 +1817,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;
 	}
 
@@ -1814,8 +1830,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
 	return 0;
 
 drop:
+	reason = reason ?: SKB_DROP_REASON_NOT_SPECIFIED;
 	/* 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 4748680e8c88..d38371f33e2b 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

[PATCH net-next v2 08/12] 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 for vxlan:

/* no remote found */
VXLAN_DROP_NO_REMOTE

And following drop reason is introduced to dropreason-core:

/* txinfo is missed in "external" mode */
SKB_DROP_REASON_TUNNEL_TXINFO

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
v2:
- move the drop reason "TXINFO" from vxlan to core
- rename VXLAN_DROP_REMOTE to VXLAN_DROP_NO_REMOTE
---
 drivers/net/vxlan/drop.h       | 3 +++
 drivers/net/vxlan/vxlan_core.c | 6 +++---
 include/net/dropreason-core.h  | 3 +++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
index 416532633881..a8ad96e0a502 100644
--- a/drivers/net/vxlan/drop.h
+++ b/drivers/net/vxlan/drop.h
@@ -13,6 +13,7 @@
 	R(VXLAN_DROP_ENTRY_EXISTS)		\
 	R(VXLAN_DROP_INVALID_HDR)		\
 	R(VXLAN_DROP_VNI_NOT_FOUND)		\
+	R(VXLAN_DROP_NO_REMOTE)			\
 	/* deliberate comment for trailing \ */
 
 enum vxlan_drop_reason {
@@ -33,6 +34,8 @@ enum vxlan_drop_reason {
 	VXLAN_DROP_INVALID_HDR,
 	/** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
 	VXLAN_DROP_VNI_NOT_FOUND,
+	/** @VXLAN_DROP_NO_REMOTE: no remote found to transmit the packet */
+	VXLAN_DROP_NO_REMOTE,
 };
 
 static inline void
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index ab1c14a807f2..c3bdac6834d4 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2728,7 +2728,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);
+				kfree_skb_reason(skb, SKB_DROP_REASON_TUNNEL_TXINFO);
 			return NETDEV_TX_OK;
 		}
 	}
@@ -2791,7 +2791,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_NO_REMOTE);
 			return NETDEV_TX_OK;
 		}
 	}
@@ -2814,7 +2814,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_NO_REMOTE);
 	}
 
 	return NETDEV_TX_OK;
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index d38371f33e2b..77bd92f507d8 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -93,6 +93,7 @@
 	FN(TC_CHAIN_NOTFOUND)		\
 	FN(TC_RECLASSIFY_LOOP)		\
 	FN(IP_TUNNEL_ECN)		\
+	FN(TUNNEL_TXINFO)		\
 	FNe(MAX)
 
 /**
@@ -424,6 +425,8 @@ enum skb_drop_reason {
 	 * RFC 6040 4.2, see __INET_ECN_decapsulate() for detail.
 	 */
 	SKB_DROP_REASON_IP_TUNNEL_ECN,
+	/** @SKB_DROP_REASON_TUNNEL_TXINFO: tx info for tunnel is missed */
+	SKB_DROP_REASON_TUNNEL_TXINFO,
 	/**
 	 * @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

[PATCH net-next v2 09/12] 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.
No drop reasons are introduced in this commit.

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/vxlan_core.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index c3bdac6834d4..f013b9007d3a 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2372,13 +2372,16 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	bool use_cache;
 	bool udp_sum = false;
 	bool xnet = !net_eq(vxlan->net, dev_net(vxlan->dev));
+	enum skb_drop_reason reason;
 	bool no_eth_encap;
 	__be32 vni = 0;
 
 	no_eth_encap = flags & VXLAN_F_GPE && skb->protocol != htons(ETH_P_TEB);
-	if (!skb_vlan_inet_prepare(skb, no_eth_encap))
+	reason = skb_vlan_inet_prepare_reason(skb, no_eth_encap);
+	if (reason)
 		goto drop;
 
+	reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	old_iph = ip_hdr(skb);
 
 	info = skb_tunnel_info(skb);
@@ -2482,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;
 		}
 
@@ -2533,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,
@@ -2554,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;
 		}
 
@@ -2594,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,
@@ -2610,7 +2619,7 @@ 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);
+	kfree_skb_reason(skb, reason);
 	return;
 
 tx_error:
@@ -2622,7 +2631,7 @@ 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);
+	kfree_skb_reason(skb, reason);
 }
 
 static void vxlan_xmit_nh(struct sk_buff *skb, struct net_device *dev,
-- 
2.39.2

[PATCH net-next v2 10/12] net: vxlan: use kfree_skb_reason in vxlan_mdb_xmit

[Menglong Dong]

Replace kfree_skb() with kfree_skb_reason() in vxlan_mdb_xmit. No drop
reaons are introduced in this commit.

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

diff --git a/drivers/net/vxlan/vxlan_mdb.c b/drivers/net/vxlan/vxlan_mdb.c
index 60eb95a06d55..eae3a05588d9 100644
--- a/drivers/net/vxlan/vxlan_mdb.c
+++ b/drivers/net/vxlan/vxlan_mdb.c
@@ -1712,7 +1712,7 @@ netdev_tx_t vxlan_mdb_xmit(struct vxlan_dev *vxlan,
 		vxlan_xmit_one(skb, vxlan->dev, src_vni,
 			       rcu_dereference(fremote->rd), false);
 	else
-		kfree_skb(skb);
+		vxlan_kfree_skb(skb, VXLAN_DROP_NO_REMOTE);
 
 	return NETDEV_TX_OK;
 }
-- 
2.39.2

[PATCH net-next v2 11/12] 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 f013b9007d3a..adf89423e5fd 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2288,7 +2288,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 v2 12/12] 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 adf89423e5fd..65f532a000f0 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2339,7 +2339,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_INVALID_HDR);
 
 			return -ENOENT;
 		}
-- 
2.39.2

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

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:50 +0800

> In this commit, we introduce the SKB_DROP_REASON_SUBSYS_VXLAN to make the
> vxlan support skb drop reasons.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  drivers/net/vxlan/drop.h          | 25 +++++++++++++++++++++++++
>  drivers/net/vxlan/vxlan_core.c    | 15 +++++++++++++++
>  drivers/net/vxlan/vxlan_private.h |  1 +
>  include/net/dropreason.h          |  6 ++++++
>  4 files changed, 47 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..6bcc6894fbbd
> --- /dev/null
> +++ b/drivers/net/vxlan/drop.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * VXLAN drop reason list.
> + */
> +
> +#ifndef VXLAN_DROP_H
> +#define VXLAN_DROP_H

Empty newline here please after the guard.

> +#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,

Maybe make SHIFT start at the same position as VXLAN, i.e. align the
former to the latter?

[...]

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

Also an empty newline here.

> +#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

"VXLAN", uppercase?

> +	 * drivers/net/vxlan/drop.h
> +	 */
> +	SKB_DROP_REASON_SUBSYS_VXLAN,
> +
>  	/** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
>  	SKB_DROP_REASON_SUBSYS_NUM
>  };

Thanks,
Olek

Re: [PATCH net-next v2 02/12] net: skb: add pskb_network_may_pull_reason() helper

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:51 +0800

> Introduce the function pskb_network_may_pull_reason() and make
> pskb_network_may_pull() a simple inline call to it. The drop reasons of
> it just come from pskb_may_pull_reason.

No object code changes I guess? Have you checked it via
scripts/bloat-o-meter?

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

Thanks,
Olek

Re: [PATCH net-next v2 04/12] net: tunnel: add skb_vlan_inet_prepare_reason() helper

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:53 +0800

> Introduce the function skb_vlan_inet_prepare_reason() and make
> skb_vlan_inet_prepare an inline call to it. The drop reasons of it just
> come from pskb_may_pull_reason.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  include/net/ip_tunnels.h | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 7fc2f7bf837a..90f8d1510a76 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -465,13 +465,14 @@ 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().
> +/* Variant of pskb_inet_may_pull_reason().
>   */
> -static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
> -					 bool inner_proto_inherit)
> +static inline enum skb_drop_reason
> +skb_vlan_inet_prepare_reason(struct sk_buff *skb, bool inner_proto_inherit)
>  {
>  	int nhlen = 0, maclen = inner_proto_inherit ? 0 : ETH_HLEN;
>  	__be16 type = skb->protocol;
> +	enum skb_drop_reason reason;
>  
>  	/* Essentially this is skb_protocol(skb, true)
>  	 * And we get MAC len.
> @@ -492,11 +493,19 @@ static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
>  	/* For ETH_P_IPV6/ETH_P_IP we make sure to pull
>  	 * a base network header in skb->head.
>  	 */
> -	if (!pskb_may_pull(skb, maclen + nhlen))
> -		return false;
> +	reason = pskb_may_pull_reason(skb, maclen + nhlen);
> +	if (reason)
> +		return reason;
>  
>  	skb_set_network_header(skb, maclen);
> -	return true;
> +	return SKB_NOT_DROPPED_YET;

An empty newline before the return as we usually do?

> +}
> +
> +static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
> +					 bool inner_proto_inherit)
> +{
> +	return skb_vlan_inet_prepare_reason(skb, inner_proto_inherit) ==
> +		SKB_NOT_DROPPED_YET;

This line must be aligned to skb_vlan_blah, IOW you need 7 spaces, not 1
tab here.

>  }
>  
>  static inline int ip_encap_hlen(struct ip_tunnel_encap *e)

Thanks,
Olek

Re: [PATCH net-next v2 05/12] net: vxlan: make vxlan_remcsum() return drop reasons

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:54 +0800

> 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 fcd224a1d0c0..76b217d166ef 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1551,9 +1551,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)
> @@ -1562,15 +1564,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)
> +		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;

Also an empty newline before return.

> +	return SKB_NOT_DROPPED_YET;
>  }
>  
>  static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,

Thanks,
Olek

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:55 +0800

> Change the return type of vxlan_set_mac() from bool to enum
> skb_drop_reason. In this commit, two drop reasons are introduced:
> 
>   VXLAN_DROP_INVALID_SMAC
>   VXLAN_DROP_ENTRY_EXISTS
> 
> To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> we don't define the enum vxlan_drop_reason with the macro
> VXLAN_DROP_REASONS(), but hand by hand.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  drivers/net/vxlan/drop.h       |  9 +++++++++
>  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 6bcc6894fbbd..876b4a9de92f 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -9,11 +9,20 @@
>  #include <net/dropreason.h>
>  
>  #define VXLAN_DROP_REASONS(R)			\
> +	R(VXLAN_DROP_INVALID_SMAC)		\
> +	R(VXLAN_DROP_ENTRY_EXISTS)		\

To Jakub:

In our recent conversation, you said you dislike templates much. What
about this one? :>

>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
>  	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
>  				SKB_DROP_REASON_SUBSYS_SHIFT,
> +	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> +	VXLAN_DROP_INVALID_SMAC,
> +	/**
> +	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> +	 * one pointing to a nexthop
> +	 */

Maybe you'd do a proper kdoc for this enum at the top?

> +	VXLAN_DROP_ENTRY_EXISTS,
>  };
>  
>  static inline void
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 76b217d166ef..58c175432a15 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
>  	unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
>  }
>  
> -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> -			  struct vxlan_sock *vs,
> -			  struct sk_buff *skb, __be32 vni)
> +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> +					  struct vxlan_sock *vs,
> +					  struct sk_buff *skb, __be32 vni)
>  {
>  	union vxlan_addr saddr;
>  	u32 ifindex = skb->dev->ifindex;
> @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	/* Ignore packet loops (and multicast echo) */
>  	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> -		return false;
> +		return (u32)VXLAN_DROP_INVALID_SMAC;
>  
>  	/* Get address from the outer IP header */
>  	if (vxlan_get_sk_family(vs) == AF_INET) {
> @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
>  	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> -		return false;
> +		return (u32)VXLAN_DROP_ENTRY_EXISTS;
>  
> -	return true;
> +	return (u32)SKB_NOT_DROPPED_YET;

Redundant cast.

>  }

Don't you need to adjust the call site accordingly? Previously, this
function returned false in case of error and true otherwise, now it
returns a non-zero in case of error and 0 (NOT_DROPPED_YET == 0) if
everything went fine.
IOW the call site now needs to check whether the return value !=
NOT_DROPPED_YET instead of checking whether it returned false. You
inverted the return code logic, but didn't touch the call site.

>  
>  static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,

Thanks,
Olek

Re: [PATCH net-next v2 07/12] net: vxlan: add skb drop reasons to vxlan_rcv()

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 30 Aug 2024 09:59:56 +0800

> Introduce skb drop reasons to the function vxlan_rcv(). Following new
> vxlan drop reasons are added:
> 
>   VXLAN_DROP_INVALID_HDR
>   VXLAN_DROP_VNI_NOT_FOUND
> 
> And Following core skb drop reason is added:

"the following", lowercase + "the".

> 
>   SKB_DROP_REASON_IP_TUNNEL_ECN
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>

[...]

> @@ -23,6 +25,14 @@ enum vxlan_drop_reason {
>  	 * one pointing to a nexthop
>  	 */
>  	VXLAN_DROP_ENTRY_EXISTS,
> +	/**
> +	 * @VXLAN_DROP_INVALID_HDR: the vxlan header is invalid, such as:

Same as before, "VXLAN" in uppercase I'd say.

> +	 * 1) the reserved fields are not zero
> +	 * 2) the "I" flag is not set
> +	 */
> +	VXLAN_DROP_INVALID_HDR,
> +	/** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */

^

> +	VXLAN_DROP_VNI_NOT_FOUND,
>  };

[...]

>  	if (!raw_proto) {
> -		if (!vxlan_set_mac(vxlan, vs, skb, vni))
> +		reason = vxlan_set_mac(vxlan, vs, skb, vni);
> +		if (reason)
>  			goto drop;

This piece must go in the previous patch, see my comment there.

[...]

> @@ -1814,8 +1830,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>  	return 0;
>  
>  drop:
> +	reason = reason ?: SKB_DROP_REASON_NOT_SPECIFIED;

Is this possible that @reason will be 0 (NOT_DROPPED_YET) here? At the
beginning of the function, it's not initialized, then each error path
sets it to a specific value. In most paths, you check for it being != 0
as a sign of error, so I doubt it can be 0 here.

>  	/* Consume bad packet */
> -	kfree_skb(skb);
> +	kfree_skb_reason(skb, reason);
>  	return 0;
>  }

Thanks,
Olek

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Simon Horman]

On Fri, Aug 30, 2024 at 09:59:55AM +0800, Menglong Dong wrote:
> Change the return type of vxlan_set_mac() from bool to enum
> skb_drop_reason. In this commit, two drop reasons are introduced:
> 
>   VXLAN_DROP_INVALID_SMAC
>   VXLAN_DROP_ENTRY_EXISTS
> 
> To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> we don't define the enum vxlan_drop_reason with the macro
> VXLAN_DROP_REASONS(), but hand by hand.
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
>  drivers/net/vxlan/drop.h       |  9 +++++++++
>  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 6bcc6894fbbd..876b4a9de92f 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -9,11 +9,20 @@
>  #include <net/dropreason.h>
>  
>  #define VXLAN_DROP_REASONS(R)			\
> +	R(VXLAN_DROP_INVALID_SMAC)		\
> +	R(VXLAN_DROP_ENTRY_EXISTS)		\
>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
>  	__VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
>  				SKB_DROP_REASON_SUBSYS_SHIFT,
> +	/** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> +	VXLAN_DROP_INVALID_SMAC,
> +	/**
> +	 * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> +	 * one pointing to a nexthop

Maybe it is clearer to write: one -> an entry

> +	 */
> +	VXLAN_DROP_ENTRY_EXISTS,
>  };
>  
>  static inline void

...

Re: [PATCH net-next v2 08/12] net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()

[Simon Horman]

On Fri, Aug 30, 2024 at 09:59:57AM +0800, Menglong Dong wrote:
> Replace kfree_skb() with vxlan_kfree_skb() in vxlan_xmit(). Following
> new skb drop reasons are introduced for vxlan:
> 
> /* no remote found */
> VXLAN_DROP_NO_REMOTE
> 
> And following drop reason is introduced to dropreason-core:
> 
> /* txinfo is missed in "external" mode */
> SKB_DROP_REASON_TUNNEL_TXINFO
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
> v2:
> - move the drop reason "TXINFO" from vxlan to core
> - rename VXLAN_DROP_REMOTE to VXLAN_DROP_NO_REMOTE
> ---
>  drivers/net/vxlan/drop.h       | 3 +++
>  drivers/net/vxlan/vxlan_core.c | 6 +++---
>  include/net/dropreason-core.h  | 3 +++
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 416532633881..a8ad96e0a502 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -13,6 +13,7 @@
>  	R(VXLAN_DROP_ENTRY_EXISTS)		\
>  	R(VXLAN_DROP_INVALID_HDR)		\
>  	R(VXLAN_DROP_VNI_NOT_FOUND)		\
> +	R(VXLAN_DROP_NO_REMOTE)			\
>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
> @@ -33,6 +34,8 @@ enum vxlan_drop_reason {
>  	VXLAN_DROP_INVALID_HDR,
>  	/** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
>  	VXLAN_DROP_VNI_NOT_FOUND,
> +	/** @VXLAN_DROP_NO_REMOTE: no remote found to transmit the packet */

Maybe: no remote found for transmit
   or: no remote found for xmit

> +	VXLAN_DROP_NO_REMOTE,
>  };
>  
>  static inline void

...

Re: [PATCH net-next v2 07/12] net: vxlan: add skb drop reasons to vxlan_rcv()

[Simon Horman]

On Fri, Aug 30, 2024 at 09:59:56AM +0800, Menglong Dong wrote:
> Introduce skb drop reasons to the function vxlan_rcv(). Following new
> vxlan drop reasons are added:
> 
>   VXLAN_DROP_INVALID_HDR
>   VXLAN_DROP_VNI_NOT_FOUND
> 
> And Following core skb drop reason is added:
> 
>   SKB_DROP_REASON_IP_TUNNEL_ECN
> 
> Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> ---
> v2:
> - rename the drop reasons, as Ido advised.
> - document the drop reasons
> ---
>  drivers/net/vxlan/drop.h       | 10 ++++++++++
>  drivers/net/vxlan/vxlan_core.c | 35 +++++++++++++++++++++++++---------
>  include/net/dropreason-core.h  |  6 ++++++
>  3 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> index 876b4a9de92f..416532633881 100644
> --- a/drivers/net/vxlan/drop.h
> +++ b/drivers/net/vxlan/drop.h
> @@ -11,6 +11,8 @@
>  #define VXLAN_DROP_REASONS(R)			\
>  	R(VXLAN_DROP_INVALID_SMAC)		\
>  	R(VXLAN_DROP_ENTRY_EXISTS)		\
> +	R(VXLAN_DROP_INVALID_HDR)		\
> +	R(VXLAN_DROP_VNI_NOT_FOUND)		\
>  	/* deliberate comment for trailing \ */
>  
>  enum vxlan_drop_reason {
> @@ -23,6 +25,14 @@ enum vxlan_drop_reason {
>  	 * one pointing to a nexthop
>  	 */
>  	VXLAN_DROP_ENTRY_EXISTS,
> +	/**
> +	 * @VXLAN_DROP_INVALID_HDR: the vxlan header is invalid, such as:

> +	 * 1) the reserved fields are not zero
> +	 * 2) the "I" flag is not set

Maybe:
	 * ...: VXLAN header is invalid. E.g.:
	 * 1) reserved fields are not zero
	 * 2) "I" flag is not set

> +	 */
> +	VXLAN_DROP_INVALID_HDR,
> +	/** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */

Maybe: no VXLAN device found for VNI

> +	VXLAN_DROP_VNI_NOT_FOUND,
>  };

...

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Jakub Kicinski]

On Fri, 30 Aug 2024 09:59:55 +0800 Menglong Dong wrote:
> -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> -			  struct vxlan_sock *vs,
> -			  struct sk_buff *skb, __be32 vni)
> +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> +					  struct vxlan_sock *vs,
> +					  struct sk_buff *skb, __be32 vni)
>  {
>  	union vxlan_addr saddr;
>  	u32 ifindex = skb->dev->ifindex;
> @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	/* Ignore packet loops (and multicast echo) */
>  	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> -		return false;
> +		return (u32)VXLAN_DROP_INVALID_SMAC;

It's the MAC address of the local interface, not just invalid...

>  	/* Get address from the outer IP header */
>  	if (vxlan_get_sk_family(vs) == AF_INET) {
> @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  
>  	if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
>  	    vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> -		return false;
> +		return (u32)VXLAN_DROP_ENTRY_EXISTS;

... because it's vxlan_snoop() that checks:

        if (!is_valid_ether_addr(src_mac))
-- 
pw-bot: cr

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

[Menglong Dong]

On Fri, Aug 30, 2024 at 10:43 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:50 +0800
>
> > In this commit, we introduce the SKB_DROP_REASON_SUBSYS_VXLAN to make the
> > vxlan support skb drop reasons.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  drivers/net/vxlan/drop.h          | 25 +++++++++++++++++++++++++
> >  drivers/net/vxlan/vxlan_core.c    | 15 +++++++++++++++
> >  drivers/net/vxlan/vxlan_private.h |  1 +
> >  include/net/dropreason.h          |  6 ++++++
> >  4 files changed, 47 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..6bcc6894fbbd
> > --- /dev/null
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * VXLAN drop reason list.
> > + */
> > +
> > +#ifndef VXLAN_DROP_H
> > +#define VXLAN_DROP_H
>
> Empty newline here please after the guard.
>
> > +#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,
>
> Maybe make SHIFT start at the same position as VXLAN, i.e. align the
> former to the latter?
>

Yeah, that looks better.

> [...]
>
> > 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>
>
> Also an empty newline here.
>
> > +#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
>
> "VXLAN", uppercase?
>

Okay! I'll change this commit as you advised.

Thanks!
Menglong Dong

> > +      * drivers/net/vxlan/drop.h
> > +      */
> > +     SKB_DROP_REASON_SUBSYS_VXLAN,
> > +
> >       /** @SKB_DROP_REASON_SUBSYS_NUM: number of subsystems defined */
> >       SKB_DROP_REASON_SUBSYS_NUM
> >  };
>
> Thanks,
> Olek

Re: [PATCH net-next v2 02/12] net: skb: add pskb_network_may_pull_reason() helper

[Menglong Dong]

On Fri, Aug 30, 2024 at 10:45 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:51 +0800
>
> > Introduce the function pskb_network_may_pull_reason() and make
> > pskb_network_may_pull() a simple inline call to it. The drop reasons of
> > it just come from pskb_may_pull_reason.
>
> No object code changes I guess? Have you checked it via
> scripts/bloat-o-meter?
>

Not yet. I'll have a try on 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;
> >  }
> >
> >  /*
>
> Thanks,
> Olek

Re: [PATCH net-next v2 04/12] net: tunnel: add skb_vlan_inet_prepare_reason() helper

[Menglong Dong]

On Fri, Aug 30, 2024 at 10:47 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:53 +0800
>
> > Introduce the function skb_vlan_inet_prepare_reason() and make
> > skb_vlan_inet_prepare an inline call to it. The drop reasons of it just
> > come from pskb_may_pull_reason.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  include/net/ip_tunnels.h | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> > index 7fc2f7bf837a..90f8d1510a76 100644
> > --- a/include/net/ip_tunnels.h
> > +++ b/include/net/ip_tunnels.h
> > @@ -465,13 +465,14 @@ 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().
> > +/* Variant of pskb_inet_may_pull_reason().
> >   */
> > -static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
> > -                                      bool inner_proto_inherit)
> > +static inline enum skb_drop_reason
> > +skb_vlan_inet_prepare_reason(struct sk_buff *skb, bool inner_proto_inherit)
> >  {
> >       int nhlen = 0, maclen = inner_proto_inherit ? 0 : ETH_HLEN;
> >       __be16 type = skb->protocol;
> > +     enum skb_drop_reason reason;
> >
> >       /* Essentially this is skb_protocol(skb, true)
> >        * And we get MAC len.
> > @@ -492,11 +493,19 @@ static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
> >       /* For ETH_P_IPV6/ETH_P_IP we make sure to pull
> >        * a base network header in skb->head.
> >        */
> > -     if (!pskb_may_pull(skb, maclen + nhlen))
> > -             return false;
> > +     reason = pskb_may_pull_reason(skb, maclen + nhlen);
> > +     if (reason)
> > +             return reason;
> >
> >       skb_set_network_header(skb, maclen);
> > -     return true;
> > +     return SKB_NOT_DROPPED_YET;
>
> An empty newline before the return as we usually do?
>
> > +}
> > +
> > +static inline bool skb_vlan_inet_prepare(struct sk_buff *skb,
> > +                                      bool inner_proto_inherit)
> > +{
> > +     return skb_vlan_inet_prepare_reason(skb, inner_proto_inherit) ==
> > +             SKB_NOT_DROPPED_YET;
>
> This line must be aligned to skb_vlan_blah, IOW you need 7 spaces, not 1
> tab here.
>

Okay! I'll follow your advice of the format in this series.

Thanks!
Menglong Dong

> >  }
> >
> >  static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
>
> Thanks,
> Olek

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Menglong Dong]

On Fri, Aug 30, 2024 at 11:31 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Aug 30, 2024 at 09:59:55AM +0800, Menglong Dong wrote:
> > Change the return type of vxlan_set_mac() from bool to enum
> > skb_drop_reason. In this commit, two drop reasons are introduced:
> >
> >   VXLAN_DROP_INVALID_SMAC
> >   VXLAN_DROP_ENTRY_EXISTS
> >
> > To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> > we don't define the enum vxlan_drop_reason with the macro
> > VXLAN_DROP_REASONS(), but hand by hand.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  drivers/net/vxlan/drop.h       |  9 +++++++++
> >  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 6bcc6894fbbd..876b4a9de92f 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -9,11 +9,20 @@
> >  #include <net/dropreason.h>
> >
> >  #define VXLAN_DROP_REASONS(R)                        \
> > +     R(VXLAN_DROP_INVALID_SMAC)              \
> > +     R(VXLAN_DROP_ENTRY_EXISTS)              \
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> >       __VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
> >                               SKB_DROP_REASON_SUBSYS_SHIFT,
> > +     /** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> > +     VXLAN_DROP_INVALID_SMAC,
> > +     /**
> > +      * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> > +      * one pointing to a nexthop
>
> Maybe it is clearer to write: one -> an entry
>

Okay!

> > +      */
> > +     VXLAN_DROP_ENTRY_EXISTS,
> >  };
> >
> >  static inline void
>
> ...

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Menglong Dong]

On Sat, Aug 31, 2024 at 7:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 30 Aug 2024 09:59:55 +0800 Menglong Dong wrote:
> > -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > -                       struct vxlan_sock *vs,
> > -                       struct sk_buff *skb, __be32 vni)
> > +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> > +                                       struct vxlan_sock *vs,
> > +                                       struct sk_buff *skb, __be32 vni)
> >  {
> >       union vxlan_addr saddr;
> >       u32 ifindex = skb->dev->ifindex;
> > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       /* Ignore packet loops (and multicast echo) */
> >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > -             return false;
> > +             return (u32)VXLAN_DROP_INVALID_SMAC;
>
> It's the MAC address of the local interface, not just invalid...
>

Yeah, my mistake. It seems that we need to add a
VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
or can we reuse VXLAN_DROP_INVALID_SMAC here too?

> >       /* Get address from the outer IP header */
> >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > -             return false;
> > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
>
> ... because it's vxlan_snoop() that checks:
>
>         if (!is_valid_ether_addr(src_mac))

It seems that we need to make vxlan_snoop() return skb drop reasons
too, and we need to add a new patch, which makes this series too many
patches. Any  advice?

I'll see if I can combine some of them into one.

Thanks!
Menglong Dong

> --
> pw-bot: cr

Re: [PATCH net-next v2 07/12] net: vxlan: add skb drop reasons to vxlan_rcv()

[Menglong Dong]

On Fri, Aug 30, 2024 at 11:36 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Aug 30, 2024 at 09:59:56AM +0800, Menglong Dong wrote:
> > Introduce skb drop reasons to the function vxlan_rcv(). Following new
> > vxlan drop reasons are added:
> >
> >   VXLAN_DROP_INVALID_HDR
> >   VXLAN_DROP_VNI_NOT_FOUND
> >
> > And Following core skb drop reason is added:
> >
> >   SKB_DROP_REASON_IP_TUNNEL_ECN
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> > v2:
> > - rename the drop reasons, as Ido advised.
> > - document the drop reasons
> > ---
> >  drivers/net/vxlan/drop.h       | 10 ++++++++++
> >  drivers/net/vxlan/vxlan_core.c | 35 +++++++++++++++++++++++++---------
> >  include/net/dropreason-core.h  |  6 ++++++
> >  3 files changed, 42 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 876b4a9de92f..416532633881 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -11,6 +11,8 @@
> >  #define VXLAN_DROP_REASONS(R)                        \
> >       R(VXLAN_DROP_INVALID_SMAC)              \
> >       R(VXLAN_DROP_ENTRY_EXISTS)              \
> > +     R(VXLAN_DROP_INVALID_HDR)               \
> > +     R(VXLAN_DROP_VNI_NOT_FOUND)             \
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> > @@ -23,6 +25,14 @@ enum vxlan_drop_reason {
> >        * one pointing to a nexthop
> >        */
> >       VXLAN_DROP_ENTRY_EXISTS,
> > +     /**
> > +      * @VXLAN_DROP_INVALID_HDR: the vxlan header is invalid, such as:
>
> > +      * 1) the reserved fields are not zero
> > +      * 2) the "I" flag is not set
>
> Maybe:
>          * ...: VXLAN header is invalid. E.g.:
>          * 1) reserved fields are not zero
>          * 2) "I" flag is not set
>

Sounds nice!

> > +      */
> > +     VXLAN_DROP_INVALID_HDR,
> > +     /** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
>
> Maybe: no VXLAN device found for VNI
>

Okay!

Thanks!
Menglong Dong

> > +     VXLAN_DROP_VNI_NOT_FOUND,
> >  };
>
> ...

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Menglong Dong]

On Fri, Aug 30, 2024 at 10:58 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:55 +0800
>
> > Change the return type of vxlan_set_mac() from bool to enum
> > skb_drop_reason. In this commit, two drop reasons are introduced:
> >
> >   VXLAN_DROP_INVALID_SMAC
> >   VXLAN_DROP_ENTRY_EXISTS
> >
> > To make it easier to document the reasons in drivers/net/vxlan/drop.h,
> > we don't define the enum vxlan_drop_reason with the macro
> > VXLAN_DROP_REASONS(), but hand by hand.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> >  drivers/net/vxlan/drop.h       |  9 +++++++++
> >  drivers/net/vxlan/vxlan_core.c | 12 ++++++------
> >  2 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 6bcc6894fbbd..876b4a9de92f 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -9,11 +9,20 @@
> >  #include <net/dropreason.h>
> >
> >  #define VXLAN_DROP_REASONS(R)                        \
> > +     R(VXLAN_DROP_INVALID_SMAC)              \
> > +     R(VXLAN_DROP_ENTRY_EXISTS)              \
>
> To Jakub:
>
> In our recent conversation, you said you dislike templates much. What
> about this one? :>
>
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> >       __VXLAN_DROP_REASON = SKB_DROP_REASON_SUBSYS_VXLAN <<
> >                               SKB_DROP_REASON_SUBSYS_SHIFT,
> > +     /** @VXLAN_DROP_INVALID_SMAC: source mac is invalid */
> > +     VXLAN_DROP_INVALID_SMAC,
> > +     /**
> > +      * @VXLAN_DROP_ENTRY_EXISTS: trying to migrate a static entry or
> > +      * one pointing to a nexthop
> > +      */
>
> Maybe you'd do a proper kdoc for this enum at the top?
>

Do you mean the enum vxlan_drop_reason? Yeah, that makes
sense, and I'll doc it.

> > +     VXLAN_DROP_ENTRY_EXISTS,
> >  };
> >
> >  static inline void
> > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > index 76b217d166ef..58c175432a15 100644
> > --- a/drivers/net/vxlan/vxlan_core.c
> > +++ b/drivers/net/vxlan/vxlan_core.c
> > @@ -1607,9 +1607,9 @@ static void vxlan_parse_gbp_hdr(struct vxlanhdr *unparsed,
> >       unparsed->vx_flags &= ~VXLAN_GBP_USED_BITS;
> >  }
> >
> > -static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > -                       struct vxlan_sock *vs,
> > -                       struct sk_buff *skb, __be32 vni)
> > +static enum skb_drop_reason vxlan_set_mac(struct vxlan_dev *vxlan,
> > +                                       struct vxlan_sock *vs,
> > +                                       struct sk_buff *skb, __be32 vni)
> >  {
> >       union vxlan_addr saddr;
> >       u32 ifindex = skb->dev->ifindex;
> > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       /* Ignore packet loops (and multicast echo) */
> >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > -             return false;
> > +             return (u32)VXLAN_DROP_INVALID_SMAC;
> >
> >       /* Get address from the outer IP header */
> >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> >
> >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > -             return false;
> > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
> >
> > -     return true;
> > +     return (u32)SKB_NOT_DROPPED_YET;
>
> Redundant cast.
>

Oops, I'll remove it.

> >  }
>
> Don't you need to adjust the call site accordingly? Previously, this
> function returned false in case of error and true otherwise, now it
> returns a non-zero in case of error and 0 (NOT_DROPPED_YET == 0) if
> everything went fine.
> IOW the call site now needs to check whether the return value !=
> NOT_DROPPED_YET instead of checking whether it returned false. You
> inverted the return code logic, but didn't touch the call site.
>

Yeah, you are right. I need to change the call of this function
in this patch too, rather than the next patch. Thanks for reminding
me of this point!

> >
> >  static bool vxlan_ecn_decapsulate(struct vxlan_sock *vs, void *oiph,
>
> Thanks,
> Olek

Re: [PATCH net-next v2 07/12] net: vxlan: add skb drop reasons to vxlan_rcv()

[Menglong Dong]

On Fri, Aug 30, 2024 at 11:04 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Menglong Dong <menglong8.dong@gmail.com>
> Date: Fri, 30 Aug 2024 09:59:56 +0800
>
> > Introduce skb drop reasons to the function vxlan_rcv(). Following new
> > vxlan drop reasons are added:
> >
> >   VXLAN_DROP_INVALID_HDR
> >   VXLAN_DROP_VNI_NOT_FOUND
> >
> > And Following core skb drop reason is added:
>
> "the following", lowercase + "the".
>

Okay!

> >
> >   SKB_DROP_REASON_IP_TUNNEL_ECN
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
>
> [...]
>
> > @@ -23,6 +25,14 @@ enum vxlan_drop_reason {
> >        * one pointing to a nexthop
> >        */
> >       VXLAN_DROP_ENTRY_EXISTS,
> > +     /**
> > +      * @VXLAN_DROP_INVALID_HDR: the vxlan header is invalid, such as:
>
> Same as before, "VXLAN" in uppercase I'd say.
>
> > +      * 1) the reserved fields are not zero
> > +      * 2) the "I" flag is not set
> > +      */
> > +     VXLAN_DROP_INVALID_HDR,
> > +     /** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
>
> ^
>
> > +     VXLAN_DROP_VNI_NOT_FOUND,
> >  };
>
> [...]
>
> >       if (!raw_proto) {
> > -             if (!vxlan_set_mac(vxlan, vs, skb, vni))
> > +             reason = vxlan_set_mac(vxlan, vs, skb, vni);
> > +             if (reason)
> >                       goto drop;
>
> This piece must go in the previous patch, see my comment there.
>

Yeah, I'll do it.

> [...]
>
> > @@ -1814,8 +1830,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
> >       return 0;
> >
> >  drop:
> > +     reason = reason ?: SKB_DROP_REASON_NOT_SPECIFIED;
>
> Is this possible that @reason will be 0 (NOT_DROPPED_YET) here? At the
> beginning of the function, it's not initialized, then each error path
> sets it to a specific value. In most paths, you check for it being != 0
> as a sign of error, so I doubt it can be 0 here.
>

It can be 0 here, as we don't set a reason for every "goto drop"
path. For example, in the line:

    if (!vs)
        goto drop;

we don't set a reason, and the "reason" is 0 when we "goto drop",
as I don't think that it is worth introducing a reason here.

Thanks!
Menglong Dong

> >       /* Consume bad packet */
> > -     kfree_skb(skb);
> > +     kfree_skb_reason(skb, reason);
> >       return 0;
> >  }
>
> Thanks,
> Olek

Re: [PATCH net-next v2 08/12] net: vxlan: use vxlan_kfree_skb() in vxlan_xmit()

[Menglong Dong]

On Fri, Aug 30, 2024 at 11:34 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Aug 30, 2024 at 09:59:57AM +0800, Menglong Dong wrote:
> > Replace kfree_skb() with vxlan_kfree_skb() in vxlan_xmit(). Following
> > new skb drop reasons are introduced for vxlan:
> >
> > /* no remote found */
> > VXLAN_DROP_NO_REMOTE
> >
> > And following drop reason is introduced to dropreason-core:
> >
> > /* txinfo is missed in "external" mode */
> > SKB_DROP_REASON_TUNNEL_TXINFO
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> > v2:
> > - move the drop reason "TXINFO" from vxlan to core
> > - rename VXLAN_DROP_REMOTE to VXLAN_DROP_NO_REMOTE
> > ---
> >  drivers/net/vxlan/drop.h       | 3 +++
> >  drivers/net/vxlan/vxlan_core.c | 6 +++---
> >  include/net/dropreason-core.h  | 3 +++
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/vxlan/drop.h b/drivers/net/vxlan/drop.h
> > index 416532633881..a8ad96e0a502 100644
> > --- a/drivers/net/vxlan/drop.h
> > +++ b/drivers/net/vxlan/drop.h
> > @@ -13,6 +13,7 @@
> >       R(VXLAN_DROP_ENTRY_EXISTS)              \
> >       R(VXLAN_DROP_INVALID_HDR)               \
> >       R(VXLAN_DROP_VNI_NOT_FOUND)             \
> > +     R(VXLAN_DROP_NO_REMOTE)                 \
> >       /* deliberate comment for trailing \ */
> >
> >  enum vxlan_drop_reason {
> > @@ -33,6 +34,8 @@ enum vxlan_drop_reason {
> >       VXLAN_DROP_INVALID_HDR,
> >       /** @VXLAN_DROP_VNI_NOT_FOUND: no vxlan device found for the vni */
> >       VXLAN_DROP_VNI_NOT_FOUND,
> > +     /** @VXLAN_DROP_NO_REMOTE: no remote found to transmit the packet */
>
> Maybe: no remote found for transmit
>    or: no remote found for xmit
>

Okay!

> > +     VXLAN_DROP_NO_REMOTE,
> >  };
> >
> >  static inline void
>
> ...

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Jakub Kicinski]

On Sun, 1 Sep 2024 20:47:27 +0800 Menglong Dong wrote:
> > > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > >
> > >       /* Ignore packet loops (and multicast echo) */
> > >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > > -             return false;
> > > +             return (u32)VXLAN_DROP_INVALID_SMAC;  
> >
> > It's the MAC address of the local interface, not just invalid...
> >  
> 
> Yeah, my mistake. It seems that we need to add a
> VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
> or can we reuse VXLAN_DROP_INVALID_SMAC here too?

Could you take a look at the bridge code and see if it has similar
checks? Learning the addresses and dropping frames which don't match
static FDB entries seem like fairly normal L2 switching drop reasons.
Perhaps we could add these as non-VXLAN specific?

The subsystem reason API was added for wireless, because wireless
folks had their own encoding, and they have their own development
tree (we don't merge their patches directly into net-next).
I keep thinking that we should add the VXLAN reason to the "core"
group rather than creating a subsystem..

> > >       /* Get address from the outer IP header */
> > >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > >
> > >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> > >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > > -             return false;
> > > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;  
> >
> > ... because it's vxlan_snoop() that checks:
> >
> >         if (!is_valid_ether_addr(src_mac))  
> 
> It seems that we need to make vxlan_snoop() return skb drop reasons
> too, and we need to add a new patch, which makes this series too many
> patches. Any  advice?

You could save some indentation by inverting the condition:

 	if (!(vxlan->cfg.flags & VXLAN_F_LEARN))
		return (u32)SKB_NOT_DROPPED_YET;

	return vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni);

But yes, I don't see a better way than having vxlan_snoop() return a
reason code :(

The patch limit count is 15, 12 is our preferred number but you can go
higher if it helps the clarity of the series.

Re: [PATCH net-next v2 07/12] net: vxlan: add skb drop reasons to vxlan_rcv()

[Alexander Lobakin]

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Sun, 1 Sep 2024 21:02:17 +0800

> On Fri, Aug 30, 2024 at 11:04 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:

[...]

>>> @@ -1814,8 +1830,9 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb)
>>>       return 0;
>>>
>>>  drop:
>>> +     reason = reason ?: SKB_DROP_REASON_NOT_SPECIFIED;
>>
>> Is this possible that @reason will be 0 (NOT_DROPPED_YET) here? At the
>> beginning of the function, it's not initialized, then each error path
>> sets it to a specific value. In most paths, you check for it being != 0
>> as a sign of error, so I doubt it can be 0 here.
>>
> 
> It can be 0 here, as we don't set a reason for every "goto drop"
> path. For example, in the line:
> 
>     if (!vs)
>         goto drop;
> 
> we don't set a reason, and the "reason" is 0 when we "goto drop",
> as I don't think that it is worth introducing a reason here.

Aaah okay, I didn't notice that, thanks for the explanation!

> 
> Thanks!
> Menglong Dong
> 
>>>       /* Consume bad packet */
>>> -     kfree_skb(skb);
>>> +     kfree_skb_reason(skb, reason);
>>>       return 0;
>>>  }

Thanks,
Olek

Re: [PATCH net-next v2 06/12] net: vxlan: make vxlan_set_mac() return drop reasons

[Menglong Dong]

On Tue, Sep 3, 2024 at 9:12 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 1 Sep 2024 20:47:27 +0800 Menglong Dong wrote:
> > > > @@ -1620,7 +1620,7 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > > >
> > > >       /* Ignore packet loops (and multicast echo) */
> > > >       if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
> > > > -             return false;
> > > > +             return (u32)VXLAN_DROP_INVALID_SMAC;
> > >
> > > It's the MAC address of the local interface, not just invalid...
> > >
> >
> > Yeah, my mistake. It seems that we need to add a
> > VXLAN_DROP_LOOP_SMAC here? I'm not sure if it is worth here,
> > or can we reuse VXLAN_DROP_INVALID_SMAC here too?
>
> Could you take a look at the bridge code and see if it has similar
> checks? Learning the addresses and dropping frames which don't match
> static FDB entries seem like fairly normal L2 switching drop reasons.
> Perhaps we could add these as non-VXLAN specific?
>

Yeah, I'll have a look at that part.

> The subsystem reason API was added for wireless, because wireless
> folks had their own encoding, and they have their own development
> tree (we don't merge their patches directly into net-next).
> I keep thinking that we should add the VXLAN reason to the "core"
> group rather than creating a subsystem..
>

I'm hesitant about this in the beginning too, as VXLAN is a standard
tunnel protocol. And I'm still hesitant now, should I add them to the "core"?
which will make things simpler.

Enn......I'll add them to the "core" in the next version, and let's see
if it is better.

> > > >       /* Get address from the outer IP header */
> > > >       if (vxlan_get_sk_family(vs) == AF_INET) {
> > > > @@ -1635,9 +1635,9 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
> > > >
> > > >       if ((vxlan->cfg.flags & VXLAN_F_LEARN) &&
> > > >           vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni))
> > > > -             return false;
> > > > +             return (u32)VXLAN_DROP_ENTRY_EXISTS;
> > >
> > > ... because it's vxlan_snoop() that checks:
> > >
> > >         if (!is_valid_ether_addr(src_mac))
> >
> > It seems that we need to make vxlan_snoop() return skb drop reasons
> > too, and we need to add a new patch, which makes this series too many
> > patches. Any  advice?
>
> You could save some indentation by inverting the condition:
>
>         if (!(vxlan->cfg.flags & VXLAN_F_LEARN))
>                 return (u32)SKB_NOT_DROPPED_YET;
>
>         return vxlan_snoop(skb->dev, &saddr, eth_hdr(skb)->h_source, ifindex, vni);
>
> But yes, I don't see a better way than having vxlan_snoop() return a
> reason code :(
>
> The patch limit count is 15, 12 is our preferred number but you can go
> higher if it helps the clarity of the series.

Okay! I feel much better with your words :/

Thanks!
Menglong Dong