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

[PATCH v3 net-next 0/7] 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_SOCKET_FILTER
SKB_DROP_REASON_NETFILTER_DROP
SKB_DROP_REASON_OTHERHOST
SKB_DROP_REASON_IP_CSUM
SKB_DROP_REASON_IP_INHDR
SKB_DROP_REASON_IP_RPFILTER
SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST
SKB_DROP_REASON_XFRM_POLICY
SKB_DROP_REASON_IP_NOPROTO
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.

Changes since v2:
- use SKB_DROP_REASON_PKT_TOO_SMALL for a path in ip_rcv_core()

Changes since v1:
- add document for all drop reasons, as David advised
- remove unreleated cleanup
- remove EARLY_DEMUX and IP_ROUTE_INPUT drop reason
- replace {UDP, TCP}_FILTER with SOCKET_FILTER


Menglong Dong (7):
  net: skb_drop_reason: add document for drop reasons
  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     | 38 ++++++++++++++++++++++++++++++++------
 include/trace/events/skb.h | 11 +++++++++++
 net/ipv4/ip_input.c        | 31 +++++++++++++++++++++++--------
 net/ipv4/udp.c             | 22 ++++++++++++++++------
 net/netfilter/core.c       |  3 ++-
 5 files changed, 84 insertions(+), 21 deletions(-)

-- 
2.27.0

[PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Add document for following existing drop reasons:

SKB_DROP_REASON_NOT_SPECIFIED
SKB_DROP_REASON_NO_SOCKET
SKB_DROP_REASON_PKT_TOO_SMALL
SKB_DROP_REASON_TCP_CSUM
SKB_DROP_REASON_SOCKET_FILTER
SKB_DROP_REASON_UDP_CSUM

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/skbuff.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8a636e678902..5c5615a487e7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -314,12 +314,12 @@ struct sk_buff;
  * used to translate the reason to string.
  */
 enum skb_drop_reason {
-	SKB_DROP_REASON_NOT_SPECIFIED,
-	SKB_DROP_REASON_NO_SOCKET,
-	SKB_DROP_REASON_PKT_TOO_SMALL,
-	SKB_DROP_REASON_TCP_CSUM,
-	SKB_DROP_REASON_SOCKET_FILTER,
-	SKB_DROP_REASON_UDP_CSUM,
+	SKB_DROP_REASON_NOT_SPECIFIED,	/* drop reason is not specified */
+	SKB_DROP_REASON_NO_SOCKET,	/* socket not found */
+	SKB_DROP_REASON_PKT_TOO_SMALL,	/* packet size is too small */
+	SKB_DROP_REASON_TCP_CSUM,	/* TCP checksum error */
+	SKB_DROP_REASON_SOCKET_FILTER,	/* dropped by socket filter */
+	SKB_DROP_REASON_UDP_CSUM,	/* UDP checksum error */
 	SKB_DROP_REASON_MAX,
 };
 
-- 
2.34.1

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Add document for following existing drop reasons:
> 
> SKB_DROP_REASON_NOT_SPECIFIED
> SKB_DROP_REASON_NO_SOCKET
> SKB_DROP_REASON_PKT_TOO_SMALL
> SKB_DROP_REASON_TCP_CSUM
> SKB_DROP_REASON_SOCKET_FILTER
> SKB_DROP_REASON_UDP_CSUM
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
>  include/linux/skbuff.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Menglong Dong]

Hello!

On Tue, Feb 1, 2022 at 1:14 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Add document for following existing drop reasons:
> >
> > SKB_DROP_REASON_NOT_SPECIFIED
> > SKB_DROP_REASON_NO_SOCKET
> > SKB_DROP_REASON_PKT_TOO_SMALL
> > SKB_DROP_REASON_TCP_CSUM
> > SKB_DROP_REASON_SOCKET_FILTER
> > SKB_DROP_REASON_UDP_CSUM
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > ---
> >  include/linux/skbuff.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
>
> Reviewed-by: David Ahern <dsahern@kernel.org>
>
>

I'm doing the job of using kfree_skb_reason() for the TCP layer,
and I have some puzzles.

When collecting drop reason for tcp_v4_inbound_md5_hash() in
tcp_v4_rcv(), I come up with 2 ways:

First way: pass the address of reason to tcp_v4_inbound_md5_hash()
like this:

 static bool tcp_v4_inbound_md5_hash(const struct sock *sk,
                      const struct sk_buff *skb,
-                    int dif, int sdif)
+                    int dif, int sdif,
+                    enum skb_drop_reason *reason)

This can work, but many functions like tcp_v4_inbound_md5_hash()
need to do such a change.

Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore,
drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX'
anywhere.

For TCP, there are many cases where you can't get a drop reason in
the place where skb is freed, so I think there needs to be a way to
deeply collect drop reasons. The second can resolve this problem
easily, but extra fields may have performance problems.

Do you have some better ideas?

Thanks!
Menglong Dong

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Jakub Kicinski]

On Thu, 10 Feb 2022 11:19:49 +0800 Menglong Dong wrote:
> I'm doing the job of using kfree_skb_reason() for the TCP layer,
> and I have some puzzles.
> 
> When collecting drop reason for tcp_v4_inbound_md5_hash() in
> tcp_v4_rcv(), I come up with 2 ways:
> 
> First way: pass the address of reason to tcp_v4_inbound_md5_hash()
> like this:
> 
>  static bool tcp_v4_inbound_md5_hash(const struct sock *sk,
>                       const struct sk_buff *skb,
> -                    int dif, int sdif)
> +                    int dif, int sdif,
> +                    enum skb_drop_reason *reason)
> 
> This can work, but many functions like tcp_v4_inbound_md5_hash()
> need to do such a change.
> 
> Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore,
> drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX'
> anywhere.
> 
> For TCP, there are many cases where you can't get a drop reason in
> the place where skb is freed, so I think there needs to be a way to
> deeply collect drop reasons. The second can resolve this problem
> easily, but extra fields may have performance problems.
> 
> Do you have some better ideas?

On a quick look tcp_v4_inbound_md5_hash() returns a drop / no drop
decision, so you could just change the return type to enum
skb_drop_reason. SKB_DROP_REASON_NOT_SPECIFIED is 0 is false, 
so if (reason) goto drop; logic will hold up.

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Menglong Dong]

On Thu, Feb 10, 2022 at 1:12 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 10 Feb 2022 11:19:49 +0800 Menglong Dong wrote:
> > I'm doing the job of using kfree_skb_reason() for the TCP layer,
> > and I have some puzzles.
> >
> > When collecting drop reason for tcp_v4_inbound_md5_hash() in
> > tcp_v4_rcv(), I come up with 2 ways:
> >
> > First way: pass the address of reason to tcp_v4_inbound_md5_hash()
> > like this:
> >
> >  static bool tcp_v4_inbound_md5_hash(const struct sock *sk,
> >                       const struct sk_buff *skb,
> > -                    int dif, int sdif)
> > +                    int dif, int sdif,
> > +                    enum skb_drop_reason *reason)
> >
> > This can work, but many functions like tcp_v4_inbound_md5_hash()
> > need to do such a change.
> >
> > Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore,
> > drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX'
> > anywhere.
> >
> > For TCP, there are many cases where you can't get a drop reason in
> > the place where skb is freed, so I think there needs to be a way to
> > deeply collect drop reasons. The second can resolve this problem
> > easily, but extra fields may have performance problems.
> >
> > Do you have some better ideas?
>
> On a quick look tcp_v4_inbound_md5_hash() returns a drop / no drop
> decision, so you could just change the return type to enum
> skb_drop_reason. SKB_DROP_REASON_NOT_SPECIFIED is 0 is false,
> so if (reason) goto drop; logic will hold up.

Yeah, that's an idea. But some functions are more complex, such as
tcp_rcv_state_process() and tcp_rcv_state_process()->tcp_v4_conn_request().
The return value of tcp_rcv_state_process() can't be reused, and it's hard
to add a function param of type 'enum skb_drop_reason *' to
tcp_v4_conn_request().

There are some nice drop reasons in tcp_v4_conn_request(), it's a pity to
give up them.

How about introducing a field to 'struct sock' for drop reasons? As sk is
locked during the packet process in tcp_v4_do_rcv(), this seems to work.

Thanks!
Menglong Dong

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Jakub Kicinski]

On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote:
> How about introducing a field to 'struct sock' for drop reasons? As sk is
> locked during the packet process in tcp_v4_do_rcv(), this seems to work.

I find adding temporary storage to persistent data structures awkward.
You can put a structure on the stack and pass it thru the call chain,
that's just my subjective preference, tho, others may have better ideas.

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Eric Dumazet]

On Thu, Feb 10, 2022 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote:
> > How about introducing a field to 'struct sock' for drop reasons? As sk is
> > locked during the packet process in tcp_v4_do_rcv(), this seems to work.
>
> I find adding temporary storage to persistent data structures awkward.
> You can put a structure on the stack and pass it thru the call chain,
> that's just my subjective preference, tho, others may have better ideas.

I had a similar TODO item, because stuff like 'waking up task' or free
one skb (or list of skb) could be performed outside of socket lock.

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Menglong Dong]

On Fri, Feb 11, 2022 at 12:29 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Feb 10, 2022 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote:
> > > How about introducing a field to 'struct sock' for drop reasons? As sk is
> > > locked during the packet process in tcp_v4_do_rcv(), this seems to work.
> >
> > I find adding temporary storage to persistent data structures awkward.
> > You can put a structure on the stack and pass it thru the call chain,
> > that's just my subjective preference, tho, others may have better ideas.
>
> I had a similar TODO item, because stuff like 'waking up task' or free
> one skb (or list of skb) could be performed outside of socket lock.

May I ask what it's like? Is it used to solve this kind of problem?

Thanks!
Menglong Dong

Re: [PATCH v3 net-next 1/7] net: skb_drop_reason: add document for drop reasons

[Menglong Dong]

On Fri, Feb 11, 2022 at 12:13 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote:
> > How about introducing a field to 'struct sock' for drop reasons? As sk is
> > locked during the packet process in tcp_v4_do_rcv(), this seems to work.
>
> I find adding temporary storage to persistent data structures awkward.
> You can put a structure on the stack and pass it thru the call chain,
> that's just my subjective preference, tho, others may have better ideas.

Yes, I also feel it is awkward. I'll try to do this job by passing drop reasons
through the call chain. Thanks for your help :)

[PATCH v3 net-next 2/7] 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. Following new drop reasons
are introduced:

SKB_DROP_REASON_NETFILTER_DROP

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- add document for SKB_DROP_REASON_NETFILTER_DROP
---
 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 5c5615a487e7..786ea2c2334e 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -320,6 +320,7 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_CSUM,	/* TCP checksum error */
 	SKB_DROP_REASON_SOCKET_FILTER,	/* dropped by socket filter */
 	SKB_DROP_REASON_UDP_CSUM,	/* UDP checksum error */
+	SKB_DROP_REASON_NETFILTER_DROP,	/* dropped by netfilter */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index a8a64b97504d..3d89f7b09a43 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_SOCKET_FILTER, SOCKET_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

[PATCH v3 net-next 3/7] 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>
---
v3:
- add a path to SKB_DROP_REASON_PKT_TOO_SMALL

v2:
- remove unrelated cleanup
- add document for introduced drop reasons
---
 include/linux/skbuff.h     |  9 +++++++++
 include/trace/events/skb.h |  3 +++
 net/ipv4/ip_input.c        | 12 ++++++++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 786ea2c2334e..2e87da91424f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -321,6 +321,15 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_SOCKET_FILTER,	/* dropped by socket filter */
 	SKB_DROP_REASON_UDP_CSUM,	/* UDP checksum error */
 	SKB_DROP_REASON_NETFILTER_DROP,	/* dropped by netfilter */
+	SKB_DROP_REASON_OTHERHOST,	/* packet don't belong to current
+					 * host (interface is in promisc
+					 * mode)
+					 */
+	SKB_DROP_REASON_IP_CSUM,	/* IP checksum error */
+	SKB_DROP_REASON_IP_INHDR,	/* there is something wrong with
+					 * IP header (see
+					 * IPSTATS_MIB_INHDRERRORS)
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 3d89f7b09a43..f2b1778485f0 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -17,6 +17,9 @@
 	EM(SKB_DROP_REASON_SOCKET_FILTER, SOCKET_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..627fad437593 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);
 
@@ -488,6 +493,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 
 	len = ntohs(iph->tot_len);
 	if (skb->len < len) {
+		drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
 	} else if (len < (iph->ihl*4))
@@ -516,11 +522,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 v3 net-next 3/7] net: ipv4: use kfree_skb_reason() in ip_rcv_core()

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
\> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> index 3a025c011971..627fad437593 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;

move this line down, right before:

	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
		goto inhdr_error;

> +
>  	/* 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);
>  
> @@ -488,6 +493,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
>  
>  	len = ntohs(iph->tot_len);
>  	if (skb->len < len) {
> +		drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
>  		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
>  		goto drop;
>  	} else if (len < (iph->ihl*4))
> @@ -516,11 +522,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;

That makes assumptions about the value of SKB_DROP_REASON_NOT_SPECIFIED.
Make that line:
	if (drop_reason != SKB_DROP_REASON_NOT_SPECIFIED)
		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;
>  }

Re: [PATCH v3 net-next 3/7] net: ipv4: use kfree_skb_reason() in ip_rcv_core()

[Menglong Dong]

On Tue, Feb 1, 2022 at 2:06 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> \> diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> > index 3a025c011971..627fad437593 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;
>
> move this line down, right before:
>
>         if (!pskb_may_pull(skb, sizeof(struct iphdr)))
>                 goto inhdr_error;
>
> > +
> >       /* 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);
> >
> > @@ -488,6 +493,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
> >
> >       len = ntohs(iph->tot_len);
> >       if (skb->len < len) {
> > +             drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
> >               __IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
> >               goto drop;
> >       } else if (len < (iph->ihl*4))
> > @@ -516,11 +522,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;
>
> That makes assumptions about the value of SKB_DROP_REASON_NOT_SPECIFIED.
> Make that line:
>         if (drop_reason != SKB_DROP_REASON_NOT_SPECIFIED)
>                 drop_reason = SKB_DROP_REASON_IP_INHDR;
>

You are right, the assumptions here are unsuitable. But I guess it
should be this?

         if (drop_reason == SKB_DROP_REASON_NOT_SPECIFIED)
                 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;
> >  }
>

[PATCH v3 net-next 4/7] 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_RPFILTER
SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- remove SKB_DROP_REASON_EARLY_DEMUX and SKB_DROP_REASON_IP_ROUTE_INPUT
- add document for SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST and
  SKB_DROP_REASON_IP_RPFILTER
---
 include/linux/skbuff.h     |  9 +++++++++
 include/trace/events/skb.h |  3 +++
 net/ipv4/ip_input.c        | 14 ++++++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 2e87da91424f..2d712459d564 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -330,6 +330,15 @@ enum skb_drop_reason {
 					 * IP header (see
 					 * IPSTATS_MIB_INHDRERRORS)
 					 */
+	SKB_DROP_REASON_IP_RPFILTER,	/* IP rpfilter validate failed.
+					 * see the document for rp_filter
+					 * in ip-sysctl.rst for more
+					 * information
+					 */
+	SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST, /* destination address of L2
+						  * is multicast, but L3 is
+						  * unicast.
+						  */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index f2b1778485f0..485a1d3034a4 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -20,6 +20,9 @@
 	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_RPFILTER, IP_RPFILTER)		\
+	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 7f64c5432cba..184decb1c8eb 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,
@@ -396,19 +398,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 v3 net-next 4/7] net: ipv4: use kfree_skb_reason() in ip_rcv_finish_core()

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> 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_RPFILTER
> SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v2:
> - remove SKB_DROP_REASON_EARLY_DEMUX and SKB_DROP_REASON_IP_ROUTE_INPUT
> - add document for SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST and
>   SKB_DROP_REASON_IP_RPFILTER
> ---
>  include/linux/skbuff.h     |  9 +++++++++
>  include/trace/events/skb.h |  3 +++
>  net/ipv4/ip_input.c        | 14 ++++++++++----
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 


Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH v3 net-next 5/7] 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>
---
v2:
- add document for the introduced drop reasons
---
 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 2d712459d564..4e55321e2fc2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -339,6 +339,8 @@ enum skb_drop_reason {
 						  * is multicast, but L3 is
 						  * unicast.
 						  */
+	SKB_DROP_REASON_XFRM_POLICY,	/* xfrm policy check failed */
+	SKB_DROP_REASON_IP_NOPROTO,	/* no support for IP protocol */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 485a1d3034a4..985e481c092d 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -23,6 +23,8 @@
 	EM(SKB_DROP_REASON_IP_RPFILTER, IP_RPFILTER)		\
 	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 184decb1c8eb..74c090f6eb9d 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 v3 net-next 5/7] net: ipv4: use kfree_skb_reason() in ip_protocol_deliver_rcu()

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> 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>
> ---
> v2:
> - add document for the introduced drop reasons
> ---
>  include/linux/skbuff.h     | 2 ++
>  include/trace/events/skb.h | 2 ++
>  net/ipv4/ip_input.c        | 5 +++--
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH v3 net-next 6/7] 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().

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
v2:
- use SKB_DROP_REASON_SOCKET_FILTER instead of
  SKB_DROP_REASON_UDP_FILTER
---
 net/ipv4/udp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 464590ea922e..9e28e76e95b8 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_SOCKET_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 v3 net-next 6/7] net: udp: use kfree_skb_reason() in udp_queue_rcv_one_skb()

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> Replace kfree_skb() with kfree_skb_reason() in udp_queue_rcv_one_skb().
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v2:
> - use SKB_DROP_REASON_SOCKET_FILTER instead of
>   SKB_DROP_REASON_UDP_FILTER
> ---
>  net/ipv4/udp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH v3 net-next 7/7] 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

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 4e55321e2fc2..2390f6e230fb 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -341,6 +341,11 @@ enum skb_drop_reason {
 						  */
 	SKB_DROP_REASON_XFRM_POLICY,	/* xfrm policy check failed */
 	SKB_DROP_REASON_IP_NOPROTO,	/* no support for IP protocol */
+	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 985e481c092d..cfcfd26399f7 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -25,6 +25,8 @@
 	   UNICAST_IN_L2_MULTICAST)				\
 	EM(SKB_DROP_REASON_XFRM_POLICY, XFRM_POLICY)		\
 	EM(SKB_DROP_REASON_IP_NOPROTO, IP_NOPROTO)		\
+	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 e295f7f38398..1f756bb0bb1f 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

Re: [PATCH v3 net-next 7/7] net: udp: use kfree_skb_reason() in __udp_queue_rcv_skb()

[David Ahern]

On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote:
> 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
> 
> 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(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>