diff mbox series

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

Message ID 20240830020001.79377-8-dongml2@chinatelecom.cn (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: vxlan: add skb drop reasons support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 58 this patch: 58
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 116 this patch: 116
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4960 this patch: 4960
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Menglong Dong <menglong8.dong@gmail.com>' != 'Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>'
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-30--21-00 (tests: 713)

Commit Message

Menglong Dong Aug. 30, 2024, 1:59 a.m. UTC 
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(-)

Comments

Alexander Lobakin Aug. 30, 2024, 3:04 p.m. UTC | #1 
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
Simon Horman Aug. 30, 2024, 3:36 p.m. UTC | #2 
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,
>  };

...
Menglong Dong Sept. 1, 2024, 12:48 p.m. UTC | #3 
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,
> >  };
>
> ...
Menglong Dong Sept. 1, 2024, 1:02 p.m. UTC | #4 
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
Alexander Lobakin Sept. 3, 2024, 12:32 p.m. UTC | #5 
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
diff mbox series

Patch

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