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.2On 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,
> };
...
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,
> > };
>
> ...
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
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
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
© 2016 - 2026 Red Hat, Inc.