diff mbox series

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

Message ID 20220124131538.1453657-6-imagedong@tencent.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: use kfree_skb_reason() for ip/udp packet receive | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5978 this patch: 5978
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 871 this patch: 871
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6132 this patch: 6132
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 56 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Menglong Dong Jan. 24, 2022, 1:15 p.m. UTC 
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(-)

Comments

David Ahern Jan. 26, 2022, 2:25 a.m. UTC | #1 
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,
>  };
>
Menglong Dong Jan. 26, 2022, 2:43 a.m. UTC | #2 
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,
> >  };
> >
>
David Ahern Jan. 26, 2022, 3:04 a.m. UTC | #3 
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.
Jakub Kicinski Jan. 26, 2022, 3:25 a.m. UTC | #4 
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?
David Ahern Jan. 26, 2022, 3:28 p.m. UTC | #5 
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.
diff mbox series

Patch

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