[PATCH net-next 0/6] net: use kfree_skb_reason() for ip/udp packet receive

[PATCH net-next 0/6] net: use kfree_skb_reason() for ip/udp packet receive

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In this series patches, kfree_skb() is replaced with kfree_skb_reason()
during ipv4 and udp4 packet receiving path, and following drop reasons
are introduced:

SKB_DROP_REASON_NETFILTER_DROP
SKB_DROP_REASON_OTHERHOST
SKB_DROP_REASON_IP_CSUM
SKB_DROP_REASON_IP_INHDR
SKB_DROP_REASON_IP_ROUTE_INPUT
SKB_DROP_REASON_IP_RPFILTER
SKB_DROP_REASON_EARLY_DEMUX
SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST
SKB_DROP_REASON_XFRM_POLICY
SKB_DROP_REASON_IP_NOPROTO
SKB_DROP_REASON_UDP_FILTER
SKB_DROP_REASON_SOCKET_RCVBUFF
SKB_DROP_REASON_PROTO_MEM

TCP is more complex, so I left it in the next series.

I just figure out how __print_symbolic() works. It doesn't base on the
array index, but searching for symbols by loop. So I'm a little afraid
it's performance.


Menglong Dong (6):
  net: netfilter: use kfree_drop_reason() for NF_DROP
  net: ipv4: use kfree_skb_reason() in ip_rcv_core()
  net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()
  net: ipv4: use kfree_skb_reason() in ip_protocol_deliver_rcu()
  net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()
  net: udp: use kfree_skb_reason() in __udp_queue_rcv_skb()

 include/linux/skbuff.h     | 16 +++++++++++++++
 include/trace/events/skb.h | 14 +++++++++++++
 net/ipv4/ip_input.c        | 42 +++++++++++++++++++++++++++-----------
 net/ipv4/udp.c             | 22 ++++++++++++++------
 net/netfilter/core.c       |  3 ++-
 5 files changed, 78 insertions(+), 19 deletions(-)

-- 
2.27.0

[PATCH net-next 1/6] net: netfilter: use kfree_drop_reason() for NF_DROP

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in nf_hook_slow() when
skb is dropped by reason of NF_DROP.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 1 +
 include/trace/events/skb.h | 1 +
 net/netfilter/core.c       | 3 ++-
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bf11e1fbd69b..1bcd690b8ae1 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -320,6 +320,7 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_CSUM,
 	SKB_DROP_REASON_TCP_FILTER,
 	SKB_DROP_REASON_UDP_CSUM,
+	SKB_DROP_REASON_NETFILTER_DROP,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 3e042ca2cedb..beed7bb2bc0e 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -16,6 +16,7 @@
 	EM(SKB_DROP_REASON_TCP_CSUM, TCP_CSUM)			\
 	EM(SKB_DROP_REASON_TCP_FILTER, TCP_FILTER)		\
 	EM(SKB_DROP_REASON_UDP_CSUM, UDP_CSUM)			\
+	EM(SKB_DROP_REASON_NETFILTER_DROP, NETFILTER_DROP)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 354cb472f386..d1c9dfbb11fa 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -621,7 +621,8 @@ int nf_hook_slow(struct sk_buff *skb, struct nf_hook_state *state,
 		case NF_ACCEPT:
 			break;
 		case NF_DROP:
-			kfree_skb(skb);
+			kfree_skb_reason(skb,
+					 SKB_DROP_REASON_NETFILTER_DROP);
 			ret = NF_DROP_GETERR(verdict);
 			if (ret == 0)
 				ret = -EPERM;
-- 
2.34.1

Re: [PATCH net-next 1/6] net: netfilter: use kfree_drop_reason() for NF_DROP

[Jakub Kicinski]

On Mon, 24 Jan 2022 21:15:33 +0800 menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Replace kfree_skb() with kfree_skb_reason() in nf_hook_slow() when
> skb is dropped by reason of NF_DROP.

Netfilter folks, does this look good enough to you?

Do you prefer to take the netfilter changes via your tree? I'm asking
because enum skb_drop_reason is probably going to be pretty hot so if
the patch is simple enough maybe no point dealing with merge conflicts.

Re: [PATCH net-next 1/6] net: netfilter: use kfree_drop_reason() for NF_DROP

[Pablo Neira Ayuso]

On Tue, Jan 25, 2022 at 03:32:14PM -0800, Jakub Kicinski wrote:
> On Mon, 24 Jan 2022 21:15:33 +0800 menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> > 
> > Replace kfree_skb() with kfree_skb_reason() in nf_hook_slow() when
> > skb is dropped by reason of NF_DROP.
> 
> Netfilter folks, does this look good enough to you?
> 
> Do you prefer to take the netfilter changes via your tree? I'm asking
> because enum skb_drop_reason is probably going to be pretty hot so if
> the patch is simple enough maybe no point dealing with merge conflicts.

I also think it's easier if you take it through net.git.

Thanks for asking.

[PATCH net-next 2/6] net: ipv4: use kfree_skb_reason() in ip_rcv_core()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in ip_rcv_core(). Three new
drop reasons are introduced:

SKB_DROP_REASON_OTHERHOST
SKB_DROP_REASON_IP_CSUM
SKB_DROP_REASON_IP_INHDR

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     |  3 +++
 include/trace/events/skb.h |  3 +++
 net/ipv4/ip_input.c        | 15 +++++++++++----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1bcd690b8ae1..f3028028b83e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -321,6 +321,9 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_FILTER,
 	SKB_DROP_REASON_UDP_CSUM,
 	SKB_DROP_REASON_NETFILTER_DROP,
+	SKB_DROP_REASON_OTHERHOST,
+	SKB_DROP_REASON_IP_CSUM,
+	SKB_DROP_REASON_IP_INHDR,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index beed7bb2bc0e..d1b0d9690e62 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -17,6 +17,9 @@
 	EM(SKB_DROP_REASON_TCP_FILTER, TCP_FILTER)		\
 	EM(SKB_DROP_REASON_UDP_CSUM, UDP_CSUM)			\
 	EM(SKB_DROP_REASON_NETFILTER_DROP, NETFILTER_DROP)	\
+	EM(SKB_DROP_REASON_OTHERHOST, OTHERHOST)		\
+	EM(SKB_DROP_REASON_IP_CSUM, IP_CSUM)			\
+	EM(SKB_DROP_REASON_IP_INHDR, IP_INHDR)			\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 3a025c011971..ab9bee4bbf0a 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -436,13 +436,18 @@ static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 {
 	const struct iphdr *iph;
+	int drop_reason;
 	u32 len;
 
+	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
+
 	/* When the interface is in promisc. mode, drop all the crap
 	 * that it receives, do not try to analyse it.
 	 */
-	if (skb->pkt_type == PACKET_OTHERHOST)
+	if (skb->pkt_type == PACKET_OTHERHOST) {
+		drop_reason = SKB_DROP_REASON_OTHERHOST;
 		goto drop;
+	}
 
 	__IP_UPD_PO_STATS(net, IPSTATS_MIB_IN, skb->len);
 
@@ -478,7 +483,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
 		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
 
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, iph->ihl * 4))
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
@@ -490,7 +495,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	if (skb->len < len) {
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
-	} else if (len < (iph->ihl*4))
+	} else if (len < (iph->ihl * 4))
 		goto inhdr_error;
 
 	/* Our transport medium may have padded the buffer out. Now we know it
@@ -516,11 +521,13 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	return skb;
 
 csum_error:
+	drop_reason = SKB_DROP_REASON_IP_CSUM;
 	__IP_INC_STATS(net, IPSTATS_MIB_CSUMERRORS);
 inhdr_error:
+	drop_reason = drop_reason ?: SKB_DROP_REASON_IP_INHDR;
 	__IP_INC_STATS(net, IPSTATS_MIB_INHDRERRORS);
 drop:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 out:
 	return NULL;
 }
-- 
2.34.1

Re: [PATCH net-next 2/6] net: ipv4: use kfree_skb_reason() in ip_rcv_core()

[David Ahern]

On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> @@ -478,7 +483,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
>  		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
>  
> -	if (!pskb_may_pull(skb, iph->ihl*4))
> +	if (!pskb_may_pull(skb, iph->ihl * 4))

unrelated cleanup

>  		goto inhdr_error;
>  
>  	iph = ip_hdr(skb);
> @@ -490,7 +495,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  	if (skb->len < len) {
>  		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
>  		goto drop;
> -	} else if (len < (iph->ihl*4))
> +	} else if (len < (iph->ihl * 4))

ditto

>  		goto inhdr_error;
>  
>  	/* Our transport medium may have padded the buffer out. Now we know it

Re: [PATCH net-next 2/6] net: ipv4: use kfree_skb_reason() in ip_rcv_core()

[Menglong Dong]

On Wed, Jan 26, 2022 at 10:10 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> > @@ -478,7 +483,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
> >                      IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
> >                      max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
> >
> > -     if (!pskb_may_pull(skb, iph->ihl*4))
> > +     if (!pskb_may_pull(skb, iph->ihl * 4))
>
> unrelated cleanup
>

Haha, you got me :/

> >               goto inhdr_error;
> >
> >       iph = ip_hdr(skb);
> > @@ -490,7 +495,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
> >       if (skb->len < len) {
> >               __IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
> >               goto drop;
> > -     } else if (len < (iph->ihl*4))
> > +     } else if (len < (iph->ihl * 4))
>
> ditto
>
> >               goto inhdr_error;
> >
> >       /* Our transport medium may have padded the buffer out. Now we know it
>

[PATCH net-next 3/6] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in ip_rcv_finish_core(),
following drop reasons are introduced:

SKB_DROP_REASON_IP_ROUTE_INPUT
SKB_DROP_REASON_IP_RPFILTER
SKB_DROP_REASON_EARLY_DEMUX
SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     |  4 ++++
 include/trace/events/skb.h |  5 +++++
 net/ipv4/ip_input.c        | 22 ++++++++++++++++------
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f3028028b83e..8942d32c0657 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -324,6 +324,10 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_OTHERHOST,
 	SKB_DROP_REASON_IP_CSUM,
 	SKB_DROP_REASON_IP_INHDR,
+	SKB_DROP_REASON_IP_ROUTE_INPUT,
+	SKB_DROP_REASON_IP_RPFILTER,
+	SKB_DROP_REASON_EARLY_DEMUX,
+	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index d1b0d9690e62..1dcdcc92cf08 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -20,6 +20,11 @@
 	EM(SKB_DROP_REASON_OTHERHOST, OTHERHOST)		\
 	EM(SKB_DROP_REASON_IP_CSUM, IP_CSUM)			\
 	EM(SKB_DROP_REASON_IP_INHDR, IP_INHDR)			\
+	EM(SKB_DROP_REASON_IP_ROUTE_INPUT, IP_ROUTE_INPUT)	\
+	EM(SKB_DROP_REASON_IP_RPFILTER, IP_RPFILTER)		\
+	EM(SKB_DROP_REASON_EARLY_DEMUX, EARLY_DEMUX)		\
+	EM(SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,		\
+	   UNICAST_IN_L2_MULTICAST)				\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index ab9bee4bbf0a..77bb9ddc441b 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -318,8 +318,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	int (*edemux)(struct sk_buff *skb);
+	int err, drop_reason;
 	struct rtable *rt;
-	int err;
+
+	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	if (ip_can_use_hint(skb, iph, hint)) {
 		err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos,
@@ -339,8 +341,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
 		if (ipprot && (edemux = READ_ONCE(ipprot->early_demux))) {
 			err = INDIRECT_CALL_2(edemux, tcp_v4_early_demux,
 					      udp_v4_early_demux, skb);
-			if (unlikely(err))
+			if (unlikely(err)) {
+				drop_reason = SKB_DROP_REASON_EARLY_DEMUX;
 				goto drop_error;
+			}
 			/* must reload iph, skb->head might have changed */
 			iph = ip_hdr(skb);
 		}
@@ -353,8 +357,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
 	if (!skb_valid_dst(skb)) {
 		err = ip_route_input_noref(skb, iph->daddr, iph->saddr,
 					   iph->tos, dev);
-		if (unlikely(err))
+		if (unlikely(err)) {
+			drop_reason = SKB_DROP_REASON_IP_ROUTE_INPUT;
 			goto drop_error;
+		}
 	}
 
 #ifdef CONFIG_IP_ROUTE_CLASSID
@@ -396,19 +402,23 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
 		 * so-called "hole-196" attack) so do it for both.
 		 */
 		if (in_dev &&
-		    IN_DEV_ORCONF(in_dev, DROP_UNICAST_IN_L2_MULTICAST))
+		    IN_DEV_ORCONF(in_dev, DROP_UNICAST_IN_L2_MULTICAST)) {
+			drop_reason = SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST;
 			goto drop;
+		}
 	}
 
 	return NET_RX_SUCCESS;
 
 drop:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 	return NET_RX_DROP;
 
 drop_error:
-	if (err == -EXDEV)
+	if (err == -EXDEV) {
+		drop_reason = SKB_DROP_REASON_IP_RPFILTER;
 		__NET_INC_STATS(net, LINUX_MIB_IPRPFILTER);
+	}
 	goto drop;
 }
 
-- 
2.34.1

Re: [PATCH net-next 3/6] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[David Ahern]

On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> index ab9bee4bbf0a..77bb9ddc441b 100644
> --- a/net/ipv4/ip_input.c
> +++ b/net/ipv4/ip_input.c
> @@ -318,8 +318,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
>  {
>  	const struct iphdr *iph = ip_hdr(skb);
>  	int (*edemux)(struct sk_buff *skb);
> +	int err, drop_reason;
>  	struct rtable *rt;
> -	int err;
> +
> +	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  
>  	if (ip_can_use_hint(skb, iph, hint)) {
>  		err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos,
> @@ -339,8 +341,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
>  		if (ipprot && (edemux = READ_ONCE(ipprot->early_demux))) {
>  			err = INDIRECT_CALL_2(edemux, tcp_v4_early_demux,
>  					      udp_v4_early_demux, skb);
> -			if (unlikely(err))
> +			if (unlikely(err)) {
> +				drop_reason = SKB_DROP_REASON_EARLY_DEMUX;

is there really value in this one? You ignore the error case from
ip_route_use_hint which is a similar, highly unlikely error path so why
care about this one? The only failure case is ip_mc_validate_source from
udp_v4_early_demux and 'early demux' drops really mean nothing to the user.


>  				goto drop_error;
> +			}
>  			/* must reload iph, skb->head might have changed */
>  			iph = ip_hdr(skb);
>  		}
> @@ -353,8 +357,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
>  	if (!skb_valid_dst(skb)) {
>  		err = ip_route_input_noref(skb, iph->daddr, iph->saddr,
>  					   iph->tos, dev);
> -		if (unlikely(err))
> +		if (unlikely(err)) {
> +			drop_reason = SKB_DROP_REASON_IP_ROUTE_INPUT;

The reason codes should be meaningful to users and not derived from a
code path. What does SKB_DROP_REASON_IP_ROUTE_INPUT mean as a failure?


>  			goto drop_error;
> +		}
>  	}
>  
>  #ifdef CONFIG_IP_ROUTE_CLASSID

Re: [PATCH net-next 3/6] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[Menglong Dong]

On Wed, Jan 26, 2022 at 10:18 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> > diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> > index ab9bee4bbf0a..77bb9ddc441b 100644
> > --- a/net/ipv4/ip_input.c
> > +++ b/net/ipv4/ip_input.c
> > @@ -318,8 +318,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
> >  {
> >       const struct iphdr *iph = ip_hdr(skb);
> >       int (*edemux)(struct sk_buff *skb);
> > +     int err, drop_reason;
> >       struct rtable *rt;
> > -     int err;
> > +
> > +     drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> >
> >       if (ip_can_use_hint(skb, iph, hint)) {
> >               err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos,
> > @@ -339,8 +341,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
> >               if (ipprot && (edemux = READ_ONCE(ipprot->early_demux))) {
> >                       err = INDIRECT_CALL_2(edemux, tcp_v4_early_demux,
> >                                             udp_v4_early_demux, skb);
> > -                     if (unlikely(err))
> > +                     if (unlikely(err)) {
> > +                             drop_reason = SKB_DROP_REASON_EARLY_DEMUX;
>
> is there really value in this one? You ignore the error case from
> ip_route_use_hint which is a similar, highly unlikely error path so why
> care about this one? The only failure case is ip_mc_validate_source from
> udp_v4_early_demux and 'early demux' drops really mean nothing to the user.
>

Ok, let's just ignore it ( In fact, it's because that I don't know
what 'early demux'
do :/ )

>
> >                               goto drop_error;
> > +                     }
> >                       /* must reload iph, skb->head might have changed */
> >                       iph = ip_hdr(skb);
> >               }
> > @@ -353,8 +357,10 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
> >       if (!skb_valid_dst(skb)) {
> >               err = ip_route_input_noref(skb, iph->daddr, iph->saddr,
> >                                          iph->tos, dev);
> > -             if (unlikely(err))
> > +             if (unlikely(err)) {
> > +                     drop_reason = SKB_DROP_REASON_IP_ROUTE_INPUT;
>
> The reason codes should be meaningful to users and not derived from a
> code path. What does SKB_DROP_REASON_IP_ROUTE_INPUT mean as a failure?
>

Is't it meaningful? I name it from the meaning of 'ip route lookup or validate
failed in input path', can't it express this information?

>
> >                       goto drop_error;
> > +             }
> >       }
> >
> >  #ifdef CONFIG_IP_ROUTE_CLASSID

Re: [PATCH net-next 3/6] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[David Ahern]

On 1/25/22 7:36 PM, Menglong Dong wrote:
> Is't it meaningful? I name it from the meaning of 'ip route lookup or validate
> failed in input path', can't it express this information?


ip_route_input_noref has many failures and not all of them are FIB
lookups. ip_route_input_slow has a bunch of EINVAL cases for example.

Returning a 'reason' as the code function name has no meaning to a user
and could actually be misleading in some cases. I would skip this one
for now.

Re: [PATCH net-next 3/6] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[Menglong Dong]

On Wed, Jan 26, 2022 at 10:57 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/25/22 7:36 PM, Menglong Dong wrote:
> > Is't it meaningful? I name it from the meaning of 'ip route lookup or validate
> > failed in input path', can't it express this information?
>
>
> ip_route_input_noref has many failures and not all of them are FIB
> lookups. ip_route_input_slow has a bunch of EINVAL cases for example.
>
> Returning a 'reason' as the code function name has no meaning to a user
> and could actually be misleading in some cases. I would skip this one
> for now.

Yeah, the real reason can be complex. I'll skip this case for now.

[PATCH net-next 4/6] net: ipv4: use kfree_skb_reason() in ip_protocol_deliver_rcu()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in ip_protocol_deliver_rcu().
Following new drop reasons are introduced:

SKB_DROP_REASON_XFRM_POLICY
SKB_DROP_REASON_IP_NOPROTO

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     | 2 ++
 include/trace/events/skb.h | 2 ++
 net/ipv4/ip_input.c        | 5 +++--
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8942d32c0657..603f77ef2170 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -328,6 +328,8 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_IP_RPFILTER,
 	SKB_DROP_REASON_EARLY_DEMUX,
 	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
+	SKB_DROP_REASON_XFRM_POLICY,
+	SKB_DROP_REASON_IP_NOPROTO,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 1dcdcc92cf08..e8369b8e8430 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -25,6 +25,8 @@
 	EM(SKB_DROP_REASON_EARLY_DEMUX, EARLY_DEMUX)		\
 	EM(SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,		\
 	   UNICAST_IN_L2_MULTICAST)				\
+	EM(SKB_DROP_REASON_XFRM_POLICY, XFRM_POLICY)		\
+	EM(SKB_DROP_REASON_IP_NOPROTO, IP_NOPROTO)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index a4afb367cf02..376c96cfd5d1 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -196,7 +196,8 @@ void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int protocol)
 	if (ipprot) {
 		if (!ipprot->no_policy) {
 			if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
-				kfree_skb(skb);
+				kfree_skb_reason(skb,
+						 SKB_DROP_REASON_XFRM_POLICY);
 				return;
 			}
 			nf_reset_ct(skb);
@@ -215,7 +216,7 @@ void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int protocol)
 				icmp_send(skb, ICMP_DEST_UNREACH,
 					  ICMP_PROT_UNREACH, 0);
 			}
-			kfree_skb(skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_IP_NOPROTO);
 		} else {
 			__IP_INC_STATS(net, IPSTATS_MIB_INDELIVERS);
 			consume_skb(skb);
-- 
2.34.1

Re: [PATCH net-next 4/6] net: ipv4: use kfree_skb_reason() in ip_protocol_deliver_rcu()

[David Ahern]

On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 8942d32c0657..603f77ef2170 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -328,6 +328,8 @@ enum skb_drop_reason {

It would be worthwhile to document the meaning of these as you add them
-- long description of the enum.

>  	SKB_DROP_REASON_IP_RPFILTER,
>  	SKB_DROP_REASON_EARLY_DEMUX,
>  	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,

	/* xfrm policy check failed */
> +	SKB_DROP_REASON_XFRM_POLICY,

	/* no support for IP protocol */
> +	SKB_DROP_REASON_IP_NOPROTO,
>  	SKB_DROP_REASON_MAX,
>  };
>  


If the enum is 1:1 with an SNMP counter, just state that.

Re: [PATCH net-next 4/6] net: ipv4: use kfree_skb_reason() in ip_protocol_deliver_rcu()

[Menglong Dong]

On Wed, Jan 26, 2022 at 10:21 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 8942d32c0657..603f77ef2170 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -328,6 +328,8 @@ enum skb_drop_reason {
>
> It would be worthwhile to document the meaning of these as you add them
> -- long description of the enum.

Yeah, I realize it later. I'll complete these documents.

>
> >       SKB_DROP_REASON_IP_RPFILTER,
> >       SKB_DROP_REASON_EARLY_DEMUX,
> >       SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
>
>         /* xfrm policy check failed */
> > +     SKB_DROP_REASON_XFRM_POLICY,
>
>         /* no support for IP protocol */
> > +     SKB_DROP_REASON_IP_NOPROTO,
> >       SKB_DROP_REASON_MAX,
> >  };
> >
>
>
> If the enum is 1:1 with an SNMP counter, just state that.

[PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in udp_queue_rcv_one_skb().
Following new drop reasons are introduced:

SKB_DROP_REASON_UDP_FILTER

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     |  1 +
 include/trace/events/skb.h |  1 +
 net/ipv4/udp.c             | 12 +++++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 603f77ef2170..dd64a4f2ff1d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -330,6 +330,7 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
 	SKB_DROP_REASON_XFRM_POLICY,
 	SKB_DROP_REASON_IP_NOPROTO,
+	SKB_DROP_REASON_UDP_FILTER,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index e8369b8e8430..6db61ce4d6f5 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -27,6 +27,7 @@
 	   UNICAST_IN_L2_MULTICAST)				\
 	EM(SKB_DROP_REASON_XFRM_POLICY, XFRM_POLICY)		\
 	EM(SKB_DROP_REASON_IP_NOPROTO, IP_NOPROTO)		\
+	EM(SKB_DROP_REASON_UDP_FILTER, UDP_FILTER)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 464590ea922e..57681e98e6bf 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2120,14 +2120,17 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
  */
 static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 {
+	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	struct udp_sock *up = udp_sk(sk);
 	int is_udplite = IS_UDPLITE(sk);
 
 	/*
 	 *	Charge it to the socket, dropping if the queue is full.
 	 */
-	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
+	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb)) {
+		drop_reason = SKB_DROP_REASON_XFRM_POLICY;
 		goto drop;
+	}
 	nf_reset_ct(skb);
 
 	if (static_branch_unlikely(&udp_encap_needed_key) && up->encap_type) {
@@ -2204,8 +2207,10 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 	    udp_lib_checksum_complete(skb))
 			goto csum_error;
 
-	if (sk_filter_trim_cap(sk, skb, sizeof(struct udphdr)))
+	if (sk_filter_trim_cap(sk, skb, sizeof(struct udphdr))) {
+		drop_reason = SKB_DROP_REASON_UDP_FILTER;
 		goto drop;
+	}
 
 	udp_csum_pull_header(skb);
 
@@ -2213,11 +2218,12 @@ static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
 	return __udp_queue_rcv_skb(sk, skb);
 
 csum_error:
+	drop_reason = SKB_DROP_REASON_UDP_CSUM;
 	__UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
 drop:
 	__UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
 	atomic_inc(&sk->sk_drops);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 	return -1;
 }
 
-- 
2.34.1

Re: [PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[David Ahern]

On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 603f77ef2170..dd64a4f2ff1d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -330,6 +330,7 @@ enum skb_drop_reason {
>  	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
>  	SKB_DROP_REASON_XFRM_POLICY,
>  	SKB_DROP_REASON_IP_NOPROTO,
> +	SKB_DROP_REASON_UDP_FILTER,

Is there really a need for a UDP and TCP version? why not just:

	/* dropped due to bpf filter on socket */
	SKB_DROP_REASON_SOCKET_FILTER

>  	SKB_DROP_REASON_MAX,
>  };
>  

Re: [PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[Menglong Dong]

On Wed, Jan 26, 2022 at 10:25 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 603f77ef2170..dd64a4f2ff1d 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -330,6 +330,7 @@ enum skb_drop_reason {
> >       SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
> >       SKB_DROP_REASON_XFRM_POLICY,
> >       SKB_DROP_REASON_IP_NOPROTO,
> > +     SKB_DROP_REASON_UDP_FILTER,
>
> Is there really a need for a UDP and TCP version? why not just:
>
>         /* dropped due to bpf filter on socket */
>         SKB_DROP_REASON_SOCKET_FILTER
>

I realized it, but SKB_DROP_REASON_TCP_FILTER was already
introduced before. Besides, I think maybe
a SKB_DROP_REASON_L4_CSUM is enough for UDP/TCP/ICMP
checksum error?

> >       SKB_DROP_REASON_MAX,
> >  };
> >
>

Re: [PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[David Ahern]

On 1/25/22 7:43 PM, Menglong Dong wrote:
> On Wed, Jan 26, 2022 at 10:25 AM David Ahern <dsahern@gmail.com> wrote:
>>
>> On 1/24/22 6:15 AM, menglong8.dong@gmail.com wrote:
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 603f77ef2170..dd64a4f2ff1d 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -330,6 +330,7 @@ enum skb_drop_reason {
>>>       SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,
>>>       SKB_DROP_REASON_XFRM_POLICY,
>>>       SKB_DROP_REASON_IP_NOPROTO,
>>> +     SKB_DROP_REASON_UDP_FILTER,
>>
>> Is there really a need for a UDP and TCP version? why not just:
>>
>>         /* dropped due to bpf filter on socket */
>>         SKB_DROP_REASON_SOCKET_FILTER
>>
> 
> I realized it, but SKB_DROP_REASON_TCP_FILTER was already
> introduced before. Besides, I think maybe

SKB_DROP_REASON_TCP_FILTER is not in a released kernel yet. If
Dave/Jakub are ok you can change SKB_DROP_REASON_TCP_FILTER to
SKB_DROP_REASON_SOCKET_FILTER in 'net' repository to make it usable in
both code paths.


> a SKB_DROP_REASON_L4_CSUM is enough for UDP/TCP/ICMP
> checksum error?

Separating this one has value to me since they are separate protocols.

Re: [PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[Jakub Kicinski]

On Tue, 25 Jan 2022 20:04:39 -0700 David Ahern wrote:
> > I realized it, but SKB_DROP_REASON_TCP_FILTER was already
> > introduced before. Besides, I think maybe  
> 
> SKB_DROP_REASON_TCP_FILTER is not in a released kernel yet. If
> Dave/Jakub are ok you can change SKB_DROP_REASON_TCP_FILTER to
> SKB_DROP_REASON_SOCKET_FILTER in 'net' repository to make it usable in
> both code paths.

SGTM, FWIW. 

What was the reason we went with separate CSUM values for TCP and UDP?
Should we coalesce those as well?

Re: [PATCH net-next 5/6] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[David Ahern]

On 1/25/22 8:25 PM, Jakub Kicinski wrote:
> On Tue, 25 Jan 2022 20:04:39 -0700 David Ahern wrote:
>>> I realized it, but SKB_DROP_REASON_TCP_FILTER was already
>>> introduced before. Besides, I think maybe  
>>
>> SKB_DROP_REASON_TCP_FILTER is not in a released kernel yet. If
>> Dave/Jakub are ok you can change SKB_DROP_REASON_TCP_FILTER to
>> SKB_DROP_REASON_SOCKET_FILTER in 'net' repository to make it usable in
>> both code paths.
> 
> SGTM, FWIW. 
> 
> What was the reason we went with separate CSUM values for TCP and UDP?
> Should we coalesce those as well?


To me those are good as independent reasons because the checksum is part
of the protocol and visible in packets.

[PATCH net-next 6/6] net: udp: use kfree_skb_reason() in __udp_queue_rcv_skb()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() with kfree_skb_reason() in __udp_queue_rcv_skb().
Following new drop reasons are introduced:

SKB_DROP_REASON_SOCKET_RCVBUFF
SKB_DROP_REASON_PROTO_MEM

For example, following log will be printed by ftrace when udp socket
receive buff overflow:

> 3345.537796: kfree_skb: skbaddr=00000000f013dfb6 protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3345.638805: kfree_skb: skbaddr=00000000cbc73bc7 protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3345.739975: kfree_skb: skbaddr=00000000717f24bb protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3345.841172: kfree_skb: skbaddr=00000000c62d20e9 protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3345.941353: kfree_skb: skbaddr=000000007eea9d4d protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3346.042610: kfree_skb: skbaddr=00000000c62d20e9 protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3346.142723: kfree_skb: skbaddr=00000000717f24bb protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF
> 3346.242785: kfree_skb: skbaddr=00000000cbc73bc7 protocol=2048 location=00000000c74d2ddd reason: SOCKET_RCVBUFF

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h     |  5 +++++
 include/trace/events/skb.h |  2 ++
 net/ipv4/udp.c             | 10 +++++++---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index dd64a4f2ff1d..723fc1cbbe3c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -331,6 +331,11 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_XFRM_POLICY,
 	SKB_DROP_REASON_IP_NOPROTO,
 	SKB_DROP_REASON_UDP_FILTER,
+	SKB_DROP_REASON_SOCKET_RCVBUFF,	/* socket receive buff is full */
+	SKB_DROP_REASON_PROTO_MEM,	/* proto memory limition, such as
+					 * udp packet drop out of
+					 * udp_memory_allocated.
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 6db61ce4d6f5..0496bbb0fe08 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -28,6 +28,8 @@
 	EM(SKB_DROP_REASON_XFRM_POLICY, XFRM_POLICY)		\
 	EM(SKB_DROP_REASON_IP_NOPROTO, IP_NOPROTO)		\
 	EM(SKB_DROP_REASON_UDP_FILTER, UDP_FILTER)		\
+	EM(SKB_DROP_REASON_SOCKET_RCVBUFF, SOCKET_RCVBUFF)	\
+	EM(SKB_DROP_REASON_PROTO_MEM, PROTO_MEM)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 57681e98e6bf..ca9ed24704ec 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2093,16 +2093,20 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	rc = __udp_enqueue_schedule_skb(sk, skb);
 	if (rc < 0) {
 		int is_udplite = IS_UDPLITE(sk);
+		int drop_reason;
 
 		/* Note that an ENOMEM error is charged twice */
-		if (rc == -ENOMEM)
+		if (rc == -ENOMEM) {
 			UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
 					is_udplite);
-		else
+			drop_reason = SKB_DROP_REASON_SOCKET_RCVBUFF;
+		} else {
 			UDP_INC_STATS(sock_net(sk), UDP_MIB_MEMERRORS,
 				      is_udplite);
+			drop_reason = SKB_DROP_REASON_PROTO_MEM;
+		}
 		UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
-		kfree_skb(skb);
+		kfree_skb_reason(skb, drop_reason);
 		trace_udp_fail_queue_rcv_skb(rc, sk);
 		return -1;
 	}
-- 
2.34.1