[PATCH net-next v4 0/4] tun/tap: use kfree_skb_reason() to trace dropped skb

[PATCH net-next v4 0/4] tun/tap: use kfree_skb_reason() to trace dropped skb

[Dongli Zhang]

The commit c504e5c2f964 ("net: skb: introduce kfree_skb_reason()") has
introduced the kfree_skb_reason() to help track the reason.

The tun and tap are commonly used as virtio-net/vhost-net backend. This is to
use kfree_skb_reason() to trace the dropped skb for those two drivers. 

Changed since v1:
- I have renamed many of the reasons since v1. I make them as generic as
  possible so that they can be re-used by core networking and drivers.

Changed since v2:
- declare drop_reason as type "enum skb_drop_reason"
- handle the drop in skb_list_walk_safe() case for tap driver, and
  kfree_skb_list_reason() is introduced

Changed since v3 (only for PATCH 4/4):
- rename to TAP_FILTER and TAP_TXFILTER
- honor reverse xmas tree style declaration for 'drop_reason' in
  tun_net_xmit()


The following reasons are introduced.

- SKB_DROP_REASON_SKB_CSUM

This is used whenever there is checksum error with sk_buff.

- SKB_DROP_REASON_SKB_COPY_DATA

The kernel may (zero) copy the data to or from sk_buff, e.g.,
zerocopy_sg_from_iter(), skb_copy_datagram_from_iter() and
skb_orphan_frags_rx(). This reason is for the copy related error.

- SKB_DROP_REASON_SKB_GSO_SEG

Any error reported when GSO processing the sk_buff. It is frequent to process
sk_buff gso data and we introduce a new reason to handle that.
	
- SKB_DROP_REASON_SKB_PULL
- SKB_DROP_REASON_SKB_TRIM

It is frequent to pull to sk_buff data or trim the sk_buff data.

- SKB_DROP_REASON_DEV_HDR

Any driver may report error if there is any error in the metadata on the DMA
ring buffer.

- SKB_DROP_REASON_DEV_READY

The device is not ready/online or initialized to receive data.

- SKB_DROP_REASON_FULL_RING

Suggested by Eric Dumazet.

- SKB_DROP_REASON_TAP_FILTER

Suggested by Menglong Dong. For sk_buff dropped by (ebpf) filter directly
attached to tun/tap, e.g., via TUNSETFILTEREBPF.

- SKB_DROP_REASON_TAP_TXFILTER

Suggested by Menglong Dong. For sk_buff dropped by tx filter implemented at
tun/tap, e.g., check_filter()


This is the output for TUN device.

# cat /sys/kernel/debug/tracing/trace_pipe
          <idle>-0       [014] b.s3.   893.074829: kfree_skb: skbaddr=0000000013eea285 protocol=4 location=00000000036fe12c reason: FULL_RING
      vhost-4456-4469    [024] b..1.   893.230235: kfree_skb: skbaddr=0000000011eab049 protocol=2054 location=00000000036fe12c reason: FULL_RING
          arping-6811    [002] b..1.   893.235606: kfree_skb: skbaddr=000000000121f124 protocol=2054 location=00000000036fe12c reason: FULL_RING
          arping-6811    [002] b..1.   894.235682: kfree_skb: skbaddr=000000000121f124 protocol=2054 location=00000000036fe12c reason: FULL_RING
      vhost-4456-4469    [024] b..1.   894.291240: kfree_skb: skbaddr=00000000d093f0cd protocol=2054 location=00000000036fe12c reason: FULL_RING


This is the output for TAP device.

# cat /sys/kernel/debug/tracing/trace_pipe
          <idle>-0       [004] ..s1.  1584.564287: kfree_skb: skbaddr=00000000e0987862 protocol=0 location=00000000177d2c83 reason: NOT_SPECIFIED
          arping-9338    [018] ..s1.  1584.642082: kfree_skb: skbaddr=00000000856cd27d protocol=2054 location=00000000615812ac reason: FULL_RING
          arping-9338    [018] ..s1.  1585.642190: kfree_skb: skbaddr=00000000856cd27d protocol=2054 location=00000000615812ac reason: FULL_RING
          arping-9338    [018] ..s1.  1586.642271: kfree_skb: skbaddr=00000000856cd27d protocol=2054 location=00000000615812ac reason: FULL_RING
          arping-9338    [018] ..s1.  1587.642368: kfree_skb: skbaddr=00000000856cd27d protocol=2054 location=00000000615812ac reason: FULL_RING


 drivers/net/tap.c          | 35 +++++++++++++++++++++++++----------
 drivers/net/tun.c          | 38 ++++++++++++++++++++++++++++++--------
 include/linux/skbuff.h     | 21 +++++++++++++++++++++
 include/trace/events/skb.h | 10 ++++++++++
 net/core/skbuff.c          | 11 +++++++++--
 5 files changed, 95 insertions(+), 20 deletions(-)


Thank you very much!

Dongli Zhang

[PATCH net-next v4 1/4] skbuff: introduce kfree_skb_list_reason()

[Dongli Zhang]

This is to introduce kfree_skb_list_reason() to drop a list of sk_buff with
a specific reason.

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 31be38078918..bc3f7822110c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1176,6 +1176,8 @@ static inline void kfree_skb(struct sk_buff *skb)
 }
 
 void skb_release_head_state(struct sk_buff *skb);
+void kfree_skb_list_reason(struct sk_buff *segs,
+			   enum skb_drop_reason reason);
 void kfree_skb_list(struct sk_buff *segs);
 void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
 void skb_tx_error(struct sk_buff *skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b32c5d782fe1..b4193eab3083 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -777,15 +777,22 @@ void kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason)
 }
 EXPORT_SYMBOL(kfree_skb_reason);
 
-void kfree_skb_list(struct sk_buff *segs)
+void kfree_skb_list_reason(struct sk_buff *segs,
+			   enum skb_drop_reason reason)
 {
 	while (segs) {
 		struct sk_buff *next = segs->next;
 
-		kfree_skb(segs);
+		kfree_skb_reason(segs, reason);
 		segs = next;
 	}
 }
+EXPORT_SYMBOL(kfree_skb_list_reason);
+
+void kfree_skb_list(struct sk_buff *segs)
+{
+	kfree_skb_list_reason(segs, SKB_DROP_REASON_NOT_SPECIFIED);
+}
 EXPORT_SYMBOL(kfree_skb_list);
 
 /* Dump skb information and contents.
-- 
2.17.1

Re: [PATCH net-next v4 1/4] skbuff: introduce kfree_skb_list_reason()

[Jakub Kicinski]

On Sat, 26 Feb 2022 00:49:26 -0800 Dongli Zhang wrote:
> +void kfree_skb_list(struct sk_buff *segs)
> +{
> +	kfree_skb_list_reason(segs, SKB_DROP_REASON_NOT_SPECIFIED);
> +}
>  EXPORT_SYMBOL(kfree_skb_list);

Why not make it a static inline now, like we did with kfree_skb()?

Re: [PATCH net-next v4 1/4] skbuff: introduce kfree_skb_list_reason()

[Dongli Zhang]

Hi Jakub,

On 3/1/22 6:34 PM, Jakub Kicinski wrote:
> On Sat, 26 Feb 2022 00:49:26 -0800 Dongli Zhang wrote:
>> +void kfree_skb_list(struct sk_buff *segs)
>> +{
>> +	kfree_skb_list_reason(segs, SKB_DROP_REASON_NOT_SPECIFIED);
>> +}
>>  EXPORT_SYMBOL(kfree_skb_list);
> 
> Why not make it a static inline now, like we did with kfree_skb()?
> 

I will move kfree_skb_list() to include/linux/skbuff.h as static inline.

Thank you very much!

Dongli Zhang

[PATCH net-next v4 2/4] net: tap: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

The TAP can be used as vhost-net backend. E.g., the tap_handle_frame() is
the interface to forward the skb from TAP to vhost-net/virtio-net.

However, there are many "goto drop" in the TAP driver. Therefore, the
kfree_skb_reason() is involved at each "goto drop" to help userspace
ftrace/ebpf to track the reason for the loss of packets.

The below reasons are introduced:

- SKB_DROP_REASON_SKB_CSUM
- SKB_DROP_REASON_SKB_COPY_DATA
- SKB_DROP_REASON_SKB_GSO_SEG
- SKB_DROP_REASON_DEV_HDR
- SKB_DROP_REASON_FULL_RING

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
---
Changed since v1:
  - revise the reason name
Changed since v2:
  - declare drop_reason as type "enum skb_drop_reason"
  - handle the drop in skb_list_walk_safe() case

 drivers/net/tap.c          | 35 +++++++++++++++++++++++++----------
 include/linux/skbuff.h     |  9 +++++++++
 include/trace/events/skb.h |  5 +++++
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 8e3a28ba6b28..b48f519fe63f 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -322,6 +322,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 	struct tap_dev *tap;
 	struct tap_queue *q;
 	netdev_features_t features = TAP_FEATURES;
+	enum skb_drop_reason drop_reason;
 
 	tap = tap_dev_get_rcu(dev);
 	if (!tap)
@@ -343,12 +344,16 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 		struct sk_buff *segs = __skb_gso_segment(skb, features, false);
 		struct sk_buff *next;
 
-		if (IS_ERR(segs))
+		if (IS_ERR(segs)) {
+			drop_reason = SKB_DROP_REASON_SKB_GSO_SEG;
 			goto drop;
+		}
 
 		if (!segs) {
-			if (ptr_ring_produce(&q->ring, skb))
+			if (ptr_ring_produce(&q->ring, skb)) {
+				drop_reason = SKB_DROP_REASON_FULL_RING;
 				goto drop;
+			}
 			goto wake_up;
 		}
 
@@ -356,8 +361,9 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 		skb_list_walk_safe(segs, skb, next) {
 			skb_mark_not_on_list(skb);
 			if (ptr_ring_produce(&q->ring, skb)) {
-				kfree_skb(skb);
-				kfree_skb_list(next);
+				drop_reason = SKB_DROP_REASON_FULL_RING;
+				kfree_skb_reason(skb, drop_reason);
+				kfree_skb_list_reason(next, drop_reason);
 				break;
 			}
 		}
@@ -369,10 +375,14 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 		 */
 		if (skb->ip_summed == CHECKSUM_PARTIAL &&
 		    !(features & NETIF_F_CSUM_MASK) &&
-		    skb_checksum_help(skb))
+		    skb_checksum_help(skb)) {
+			drop_reason = SKB_DROP_REASON_SKB_CSUM;
 			goto drop;
-		if (ptr_ring_produce(&q->ring, skb))
+		}
+		if (ptr_ring_produce(&q->ring, skb)) {
+			drop_reason = SKB_DROP_REASON_FULL_RING;
 			goto drop;
+		}
 	}
 
 wake_up:
@@ -383,7 +393,7 @@ rx_handler_result_t tap_handle_frame(struct sk_buff **pskb)
 	/* Count errors/drops only here, thus don't care about args. */
 	if (tap->count_rx_dropped)
 		tap->count_rx_dropped(tap);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 	return RX_HANDLER_CONSUMED;
 }
 EXPORT_SYMBOL_GPL(tap_handle_frame);
@@ -632,6 +642,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 	int depth;
 	bool zerocopy = false;
 	size_t linear;
+	enum skb_drop_reason drop_reason;
 
 	if (q->flags & IFF_VNET_HDR) {
 		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
@@ -696,8 +707,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 	else
 		err = skb_copy_datagram_from_iter(skb, 0, from, len);
 
-	if (err)
+	if (err) {
+		drop_reason = SKB_DROP_REASON_SKB_COPY_DATA;
 		goto err_kfree;
+	}
 
 	skb_set_network_header(skb, ETH_HLEN);
 	skb_reset_mac_header(skb);
@@ -706,8 +719,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 	if (vnet_hdr_len) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr,
 					    tap_is_little_endian(q));
-		if (err)
+		if (err) {
+			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;
+		}
 	}
 
 	skb_probe_transport_header(skb);
@@ -738,7 +753,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 	return total_len;
 
 err_kfree:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 
 err:
 	rcu_read_lock();
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bc3f7822110c..9f523da4d3f2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -380,6 +380,15 @@ enum skb_drop_reason {
 					 * the ofo queue, corresponding to
 					 * LINUX_MIB_TCPOFOMERGE
 					 */
+	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */
+	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
+					 * sk_buff
+					 */
+	SKB_DROP_REASON_SKB_GSO_SEG,	/* gso segmentation error */
+	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
+					 * device driver specific header
+					 */
+	SKB_DROP_REASON_FULL_RING,	/* ring buffer is full */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 2ab7193313aa..5b5f1351dcde 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -37,6 +37,11 @@
 	EM(SKB_DROP_REASON_TCP_OLD_DATA, TCP_OLD_DATA)		\
 	EM(SKB_DROP_REASON_TCP_OVERWINDOW, TCP_OVERWINDOW)	\
 	EM(SKB_DROP_REASON_TCP_OFOMERGE, TCP_OFOMERGE)		\
+	EM(SKB_DROP_REASON_SKB_CSUM, SKB_CSUM)			\
+	EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA)	\
+	EM(SKB_DROP_REASON_SKB_GSO_SEG, SKB_GSO_SEG)		\
+	EM(SKB_DROP_REASON_DEV_HDR, DEV_HDR)			\
+	EM(SKB_DROP_REASON_FULL_RING, FULL_RING)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
-- 
2.17.1

Re: [PATCH net-next v4 2/4] net: tap: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Sat, 26 Feb 2022 00:49:27 -0800 Dongli Zhang wrote:
> +	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */

Can we spell it out a little more? It sounds like the checksum was
incorrect. Will it be clear that computing the checksum failed, rather
than checksum validation failed?

> +	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
> +					 * sk_buff
> +					 */

Here should we specify that it's copying from user space?

> +	SKB_DROP_REASON_SKB_GSO_SEG,	/* gso segmentation error */
> +	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
> +					 * device driver specific header
> +					 */

How about:
device driver specific header / metadata was invalid

to broaden the scope also to devices which don't transfer the metadata
in form of a header?

> +	SKB_DROP_REASON_FULL_RING,	/* ring buffer is full */

Re: [PATCH net-next v4 2/4] net: tap: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Jakub,

On 3/1/22 6:42 PM, Jakub Kicinski wrote:
> On Sat, 26 Feb 2022 00:49:27 -0800 Dongli Zhang wrote:
>> +	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */
> 
> Can we spell it out a little more? It sounds like the checksum was
> incorrect. Will it be clear that computing the checksum failed, rather
> than checksum validation failed?

I am just trying to make the reasons as generic as possible so that:

1. We may minimize the number of reasons.

2. People may re-use the same reason for all CSUM related issue.

> 
>> +	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
>> +					 * sk_buff
>> +					 */
> 
> Here should we specify that it's copying from user space?

Same as above. I am minimizing the number of reasons so that any memory copy for
sk_buff may re-use this reason.

Please let me know if you think it is very significant to specialize the usage
of reason. I will then add "sk_buff csum computation failed" and "userspace".

> 
>> +	SKB_DROP_REASON_SKB_GSO_SEG,	/* gso segmentation error */
>> +	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>> +					 * device driver specific header
>> +					 */
> 
> How about:
> device driver specific header / metadata was invalid
> 
> to broaden the scope also to devices which don't transfer the metadata
> in form of a header?

I will add 'metadata'.

Thank you very much!

Dongli Zhang

> 
>> +	SKB_DROP_REASON_FULL_RING,	/* ring buffer is full */

Re: [PATCH net-next v4 2/4] net: tap: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Wed, 2 Mar 2022 09:43:29 -0800 Dongli Zhang wrote:
> On 3/1/22 6:42 PM, Jakub Kicinski wrote:
> > On Sat, 26 Feb 2022 00:49:27 -0800 Dongli Zhang wrote:  
> >> +	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */  
> > 
> > Can we spell it out a little more? It sounds like the checksum was
> > incorrect. Will it be clear that computing the checksum failed, rather
> > than checksum validation failed?  
> 
> I am just trying to make the reasons as generic as possible so that:
> 
> 1. We may minimize the number of reasons.
> 
> 2. People may re-use the same reason for all CSUM related issue.

The generic nature is fine, my concern is to clearly differentiate
errors in _validating_ the checksum from errors in _generating_ them.
"sk_buff checksum error" does not explain which one had taken place.

> >> +	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
> >> +					 * sk_buff
> >> +					 */  
> > 
> > Here should we specify that it's copying from user space?  
> 
> Same as above. I am minimizing the number of reasons so that any memory copy for
> sk_buff may re-use this reason.

IIUC this failure is equivalent to user passing an invalid buffer. 
I mean something like:

	send(fd, (void *)random(), 1000, 0);

I'd be tempted to call the reason something link SKB_UCOPY_FAULT.
To indicate it's a problem copying from user space. EFAULT is the
typical errno for that. WDYT?

Re: [PATCH net-next v4 2/4] net: tap: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Jakub,

On 3/2/22 11:03 AM, Jakub Kicinski wrote:
> On Wed, 2 Mar 2022 09:43:29 -0800 Dongli Zhang wrote:
>> On 3/1/22 6:42 PM, Jakub Kicinski wrote:
>>> On Sat, 26 Feb 2022 00:49:27 -0800 Dongli Zhang wrote:  
>>>> +	SKB_DROP_REASON_SKB_CSUM,	/* sk_buff checksum error */  
>>>
>>> Can we spell it out a little more? It sounds like the checksum was
>>> incorrect. Will it be clear that computing the checksum failed, rather
>>> than checksum validation failed?  
>>
>> I am just trying to make the reasons as generic as possible so that:
>>
>> 1. We may minimize the number of reasons.
>>
>> 2. People may re-use the same reason for all CSUM related issue.
> 
> The generic nature is fine, my concern is to clearly differentiate
> errors in _validating_ the checksum from errors in _generating_ them.
> "sk_buff checksum error" does not explain which one had taken place.

This is for skb_checksum_help() and it is for csum computation.

Therefore, I will keep SKB_DROP_REASON_SKB_CSUM and add 'computation' or
'generating' to the comments.

> 
>>>> +	SKB_DROP_REASON_SKB_COPY_DATA,	/* failed to copy data from or to
>>>> +					 * sk_buff
>>>> +					 */  
>>>
>>> Here should we specify that it's copying from user space?  
>>
>> Same as above. I am minimizing the number of reasons so that any memory copy for
>> sk_buff may re-use this reason.
> 
> IIUC this failure is equivalent to user passing an invalid buffer. 
> I mean something like:
> 
> 	send(fd, (void *)random(), 1000, 0);
> 
> I'd be tempted to call the reason something link SKB_UCOPY_FAULT.
> To indicate it's a problem copying from user space. EFAULT is the
> typical errno for that. WDYT?
> 

So far the reason is used for below functions' return value:

- tap_get_user() -> zerocopy_sg_from_iter()
- tap_get_user() -> skb_copy_datagram_from_iter()
- tun_net_xmit() -> skb_orphan_frags_rx() -> skb_copy_ubufs()

I will switch to SKB_UCOPY_FAULT.

Thank you very much!

Dongli Zhang

[PATCH net-next v4 3/4] net: tun: split run_ebpf_filter() and pskb_trim() into different "if statement"

[Dongli Zhang]

No functional change.

Just to split the if statement into different conditions to use
kfree_skb_reason() to trace the reason later.

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
---
 drivers/net/tun.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fed85447701a..aa27268edc5f 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1086,7 +1086,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto drop;
 
 	len = run_ebpf_filter(tun, skb, len);
-	if (len == 0 || pskb_trim(skb, len))
+	if (len == 0)
+		goto drop;
+
+	if (pskb_trim(skb, len))
 		goto drop;
 
 	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
-- 
2.17.1

[PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the
interface to forward the skb from TUN to vhost-net/virtio-net.

However, there are many "goto drop" in the TUN driver. Therefore, the
kfree_skb_reason() is involved at each "goto drop" to help userspace
ftrace/ebpf to track the reason for the loss of packets.

The below reasons are introduced:

- SKB_DROP_REASON_SKB_PULL
- SKB_DROP_REASON_SKB_TRIM
- SKB_DROP_REASON_DEV_READY
- SKB_DROP_REASON_TAP_FILTER
- SKB_DROP_REASON_TAP_TXFILTER

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v1:
  - revise the reason name
Changed since v2:
  - declare drop_reason as type "enum skb_drop_reason"
Changed since v3:
  - rename to TAP_FILTER and TAP_TXFILTER
  - honor reverse xmas tree style declaration for 'drop_reason' in
    tun_net_xmit()

 drivers/net/tun.c          | 37 ++++++++++++++++++++++++++++---------
 include/linux/skbuff.h     | 10 ++++++++++
 include/trace/events/skb.h |  5 +++++
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index aa27268edc5f..73ad2bb5e8ae 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1058,6 +1058,7 @@ static unsigned int run_ebpf_filter(struct tun_struct *tun,
 static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct tun_struct *tun = netdev_priv(dev);
+	enum skb_drop_reason drop_reason;
 	int txq = skb->queue_mapping;
 	struct netdev_queue *queue;
 	struct tun_file *tfile;
@@ -1067,8 +1068,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	tfile = rcu_dereference(tun->tfiles[txq]);
 
 	/* Drop packet if interface is not attached */
-	if (!tfile)
+	if (!tfile) {
+		drop_reason = SKB_DROP_REASON_DEV_READY;
 		goto drop;
+	}
 
 	if (!rcu_dereference(tun->steering_prog))
 		tun_automq_xmit(tun, skb);
@@ -1078,22 +1081,32 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Drop if the filter does not like it.
 	 * This is a noop if the filter is disabled.
 	 * Filter can be enabled only for the TAP devices. */
-	if (!check_filter(&tun->txflt, skb))
+	if (!check_filter(&tun->txflt, skb)) {
+		drop_reason = SKB_DROP_REASON_TAP_TXFILTER;
 		goto drop;
+	}
 
 	if (tfile->socket.sk->sk_filter &&
-	    sk_filter(tfile->socket.sk, skb))
+	    sk_filter(tfile->socket.sk, skb)) {
+		drop_reason = SKB_DROP_REASON_SOCKET_FILTER;
 		goto drop;
+	}
 
 	len = run_ebpf_filter(tun, skb, len);
-	if (len == 0)
+	if (len == 0) {
+		drop_reason = SKB_DROP_REASON_TAP_FILTER;
 		goto drop;
+	}
 
-	if (pskb_trim(skb, len))
+	if (pskb_trim(skb, len)) {
+		drop_reason = SKB_DROP_REASON_SKB_TRIM;
 		goto drop;
+	}
 
-	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
+	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC))) {
+		drop_reason = SKB_DROP_REASON_SKB_COPY_DATA;
 		goto drop;
+	}
 
 	skb_tx_timestamp(skb);
 
@@ -1104,8 +1117,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	nf_reset_ct(skb);
 
-	if (ptr_ring_produce(&tfile->tx_ring, skb))
+	if (ptr_ring_produce(&tfile->tx_ring, skb)) {
+		drop_reason = SKB_DROP_REASON_FULL_RING;
 		goto drop;
+	}
 
 	/* NETIF_F_LLTX requires to do our own update of trans_start */
 	queue = netdev_get_tx_queue(dev, txq);
@@ -1122,7 +1137,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 drop:
 	atomic_long_inc(&dev->tx_dropped);
 	skb_tx_error(skb);
-	kfree_skb(skb);
+	kfree_skb_reason(skb, drop_reason);
 	rcu_read_unlock();
 	return NET_XMIT_DROP;
 }
@@ -1720,6 +1735,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash = 0;
 	int skb_xdp = 1;
 	bool frags = tun_napi_frags_enabled(tfile);
+	enum skb_drop_reason drop_reason;
 
 	if (!(tun->flags & IFF_NO_PI)) {
 		if (len < sizeof(pi))
@@ -1823,9 +1839,10 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 		if (err) {
 			err = -EFAULT;
+			drop_reason = SKB_DROP_REASON_SKB_COPY_DATA;
 drop:
 			atomic_long_inc(&tun->dev->rx_dropped);
-			kfree_skb(skb);
+			kfree_skb_reason(skb, drop_reason);
 			if (frags) {
 				tfile->napi.skb = NULL;
 				mutex_unlock(&tfile->napi_mutex);
@@ -1872,6 +1889,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	case IFF_TAP:
 		if (frags && !pskb_may_pull(skb, ETH_HLEN)) {
 			err = -ENOMEM;
+			drop_reason = SKB_DROP_REASON_SKB_PULL;
 			goto drop;
 		}
 		skb->protocol = eth_type_trans(skb, tun->dev);
@@ -1925,6 +1943,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	if (unlikely(!(tun->dev->flags & IFF_UP))) {
 		err = -EIO;
 		rcu_read_unlock();
+		drop_reason = SKB_DROP_REASON_DEV_READY;
 		goto drop;
 	}
 
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9f523da4d3f2..9a0a15a31591 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -385,10 +385,20 @@ enum skb_drop_reason {
 					 * sk_buff
 					 */
 	SKB_DROP_REASON_SKB_GSO_SEG,	/* gso segmentation error */
+	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
+	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */
 	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
 					 * device driver specific header
 					 */
+	SKB_DROP_REASON_DEV_READY,	/* device is not ready */
 	SKB_DROP_REASON_FULL_RING,	/* ring buffer is full */
+	SKB_DROP_REASON_TAP_FILTER,	/* dropped by (ebpf) filter directly
+					 * attached to tun/tap, e.g., via
+					 * TUNSETFILTEREBPF
+					 */
+	SKB_DROP_REASON_TAP_TXFILTER,	/* dropped by tx filter implemented
+					 * at tun/tap, e.g., check_filter()
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index 5b5f1351dcde..e8dcf784ac17 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -40,8 +40,13 @@
 	EM(SKB_DROP_REASON_SKB_CSUM, SKB_CSUM)			\
 	EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA)	\
 	EM(SKB_DROP_REASON_SKB_GSO_SEG, SKB_GSO_SEG)		\
+	EM(SKB_DROP_REASON_SKB_PULL, SKB_PULL)			\
+	EM(SKB_DROP_REASON_SKB_TRIM, SKB_TRIM)			\
 	EM(SKB_DROP_REASON_DEV_HDR, DEV_HDR)			\
+	EM(SKB_DROP_REASON_DEV_READY, DEV_READY)		\
 	EM(SKB_DROP_REASON_FULL_RING, FULL_RING)		\
+	EM(SKB_DROP_REASON_TAP_FILTER, TAP_FILTER)		\
+	EM(SKB_DROP_REASON_TAP_TXFILTER, TAP_TXFILTER)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
-- 
2.17.1

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[David Ahern]

On 2/26/22 1:49 AM, Dongli Zhang wrote:
> The TUN can be used as vhost-net backend. E.g, the tun_net_xmit() is the
> interface to forward the skb from TUN to vhost-net/virtio-net.
> 
> However, there are many "goto drop" in the TUN driver. Therefore, the
> kfree_skb_reason() is involved at each "goto drop" to help userspace
> ftrace/ebpf to track the reason for the loss of packets.
> 
> The below reasons are introduced:
> 
> - SKB_DROP_REASON_SKB_PULL
> - SKB_DROP_REASON_SKB_TRIM
> - SKB_DROP_REASON_DEV_READY
> - SKB_DROP_REASON_TAP_FILTER
> - SKB_DROP_REASON_TAP_TXFILTER
> 
> Cc: Joao Martins <joao.m.martins@oracle.com>
> Cc: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> Changed since v1:
>   - revise the reason name
> Changed since v2:
>   - declare drop_reason as type "enum skb_drop_reason"
> Changed since v3:
>   - rename to TAP_FILTER and TAP_TXFILTER
>   - honor reverse xmas tree style declaration for 'drop_reason' in
>     tun_net_xmit()
> 
>  drivers/net/tun.c          | 37 ++++++++++++++++++++++++++++---------
>  include/linux/skbuff.h     | 10 ++++++++++
>  include/trace/events/skb.h |  5 +++++
>  3 files changed, 43 insertions(+), 9 deletions(-)
> 

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

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:
> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */

IDK if these are not too low level and therefore lacking meaning.

What are your thoughts David?

Would it be better to up level the names a little bit and call SKB_PULL
something like "HDR_TRUNC" or "HDR_INV" or "HDR_ERR" etc or maybe
"L2_HDR_ERR" since in this case we seem to be pulling off ETH_HLEN?

For SKB_TRIM the error comes from allocation failures, there may be
a whole bunch of skb helpers which will fail only under mem pressure,
would it be better to identify them and return some ENOMEM related
reason, since, most likely, those will be noise to whoever is tracking
real errors?

>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>  					 * device driver specific header
>  					 */
> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */

What is ready? link is not up? peer not connected? can we expand?

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[David Ahern]

On 3/1/22 7:50 PM, Jakub Kicinski wrote:
> On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:
>> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
>> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */
> 
> IDK if these are not too low level and therefore lacking meaning.
> 
> What are your thoughts David?

I agree. Not every kfree_skb is worthy of a reason. "Internal
housekeeping" errors are random and nothing a user / admin can do about
drops.

IMHO, the value of the reason code is when it aligns with SNMP counters
(original motivation for this direction) and relevant details like TCP
or UDP checksum mismatch, packets for a socket that is not open, socket
is full, ring buffer is full, packets for "other host", etc.

> 
> Would it be better to up level the names a little bit and call SKB_PULL
> something like "HDR_TRUNC" or "HDR_INV" or "HDR_ERR" etc or maybe
> "L2_HDR_ERR" since in this case we seem to be pulling off ETH_HLEN?
> 
> For SKB_TRIM the error comes from allocation failures, there may be
> a whole bunch of skb helpers which will fail only under mem pressure,
> would it be better to identify them and return some ENOMEM related
> reason, since, most likely, those will be noise to whoever is tracking
> real errors?
> 
>>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>>  					 * device driver specific header
>>  					 */
>> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */
> 
> What is ready? link is not up? peer not connected? can we expand?

As I recall in this case it is the tfile for a tun device disappeared -
ie., a race condition.

Re: [Internet]Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[imagedong(董梦龙)]

On 2022/3/2 AM 11:29，“David Ahern”<dsahern@gmail.com> write:

>On 3/1/22 7:50 PM, Jakub Kicinski wrote:
>> On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:
>>> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
>>> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */
>> 
[...]
>>>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>>>  					 * device driver specific header
>>>  					 */
>>> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */
>> 
>> What is ready? link is not up? peer not connected? can we expand?
>
>As I recall in this case it is the tfile for a tun device disappeared -
>ie., a race condition.

This seems is that tun is not attached to a file (the tun device file
is not opened?) Maybe TAP_UNATTACHED is more suitable :)

Re: [Internet]Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Menglong,

On 3/1/22 8:16 PM, imagedong(董梦龙) wrote:
> 
> 
> On 2022/3/2 AM 11:29，“David Ahern”<dsahern@gmail.com> write:
> 
>> On 3/1/22 7:50 PM, Jakub Kicinski wrote:
>>> On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:
>>>> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
>>>> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */
>>>
> [...]
>>>>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>>>>  					 * device driver specific header
>>>>  					 */
>>>> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */
>>>
>>> What is ready? link is not up? peer not connected? can we expand?
>>
>> As I recall in this case it is the tfile for a tun device disappeared -
>> ie., a race condition.
> 
> This seems is that tun is not attached to a file (the tun device file
> is not opened?) Maybe TAP_UNATTACHED is more suitable :)
> 
> 

Thank you very much for the suggestions! TAP_UNATTACHED is more suitable.

The tap/tun are only two of drivers in linux kernel. We have already introduced
another two tap/tun specific reasons.

My concern is we may finally have too many reasons. That's why I am always
trying to define the reason as generic as possible so that they will be re-used
by most networking drivers.

We may expand the reason if it is fine to have too many reasons for skb_drop_reason.

Dongli Zhang

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Tue, 1 Mar 2022 20:29:37 -0700 David Ahern wrote:
> On 3/1/22 7:50 PM, Jakub Kicinski wrote:
> > On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:  
> >> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
> >> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */  
> > 
> > IDK if these are not too low level and therefore lacking meaning.
> > 
> > What are your thoughts David?  
> 
> I agree. Not every kfree_skb is worthy of a reason. "Internal
> housekeeping" errors are random and nothing a user / admin can do about
> drops.
> 
> IMHO, the value of the reason code is when it aligns with SNMP counters
> (original motivation for this direction) and relevant details like TCP
> or UDP checksum mismatch, packets for a socket that is not open, socket
> is full, ring buffer is full, packets for "other host", etc.

Agreed :(

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Jakub and David,

On 3/1/22 6:50 PM, Jakub Kicinski wrote:
> On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:
>> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
>> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */
> 
> IDK if these are not too low level and therefore lacking meaning.
> 
> What are your thoughts David?
> 
> Would it be better to up level the names a little bit and call SKB_PULL
> something like "HDR_TRUNC" or "HDR_INV" or "HDR_ERR" etc or maybe
> "L2_HDR_ERR" since in this case we seem to be pulling off ETH_HLEN?

This is for device driver and I think for most of cases the people understanding
source code will be involved. I think SKB_PULL is more meaningful to people
understanding source code.

The functions to pull data to skb is commonly used with the same pattern, and
not only for ETH_HLEN. E.g., I randomly found below in kernel source code.

1071 static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
1072 {
... ...
1102         pulled_sci = pskb_may_pull(skb, macsec_extra_len(true));
1103         if (!pulled_sci) {
1104                 if (!pskb_may_pull(skb, macsec_extra_len(false)))
1105                         goto drop_direct;
1106         }
... ...
1254 drop_direct:
1255         kfree_skb(skb);
1256         *pskb = NULL;
1257         return RX_HANDLER_CONSUMED;


About 'L2_HDR_ERR', I am curious what the user/administrator may do as next
step, while the 'SKB_PULL' will be very clear to the developers which kernel
operation (e.g., to pull some protocol/hdr data to sk_buff data) is with the issue.

I may use 'L2_HDR_ERR' if you prefer.

> 
> For SKB_TRIM the error comes from allocation failures, there may be
> a whole bunch of skb helpers which will fail only under mem pressure,
> would it be better to identify them and return some ENOMEM related
> reason, since, most likely, those will be noise to whoever is tracking
> real errors?

The reasons I want to use SKB_TRIM:

1. To have SKB_PULL and SKB_TRIM (perhaps more SKB_XXX in the future in the same
set).

2. Although so that SKB_TRIM is always caused by ENOMEM, suppose if there is new
return values by pskb_trim(), the reason is not going to be valid any longer.


I may use SKB_DROP_REASON_NOMEM if you prefer.

Another concern is that many functions may return -ENOMEM. It is more likely
that if there are two "goto drop" to return -ENOMEM, we will not be able to tell
from which function the sk_buff is dropped, e.g.,

if (function_A()) {
    reason = -ENOMEM;
    goto drop;
}

if (function_B()) {
    reason = -ENOMEM;
    goto drop;
}

> 
>>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>>  					 * device driver specific header
>>  					 */
>> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */
> 
> What is ready? link is not up? peer not connected? can we expand?
> 

In this patchset, it is for either:

- tun->tfiles[txq] is not set, or

- !(tun->dev->flags & IFF_UP)

I want to make it very generic so that the sk_buff dropped due to any device
level data structure that is not up/ready/initialized/allocated will use this
reason in the future.


Thank you very much for suggestions!

Dongli Zhang

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Wed, 2 Mar 2022 10:19:30 -0800 Dongli Zhang wrote:
> On 3/1/22 6:50 PM, Jakub Kicinski wrote:
> > On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:  
> >> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
> >> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */  
> > 
> > IDK if these are not too low level and therefore lacking meaning.
> > 
> > What are your thoughts David?
> > 
> > Would it be better to up level the names a little bit and call SKB_PULL
> > something like "HDR_TRUNC" or "HDR_INV" or "HDR_ERR" etc or maybe
> > "L2_HDR_ERR" since in this case we seem to be pulling off ETH_HLEN?  
> 
> This is for device driver and I think for most of cases the people understanding
> source code will be involved. I think SKB_PULL is more meaningful to people
> understanding source code.
> 
> The functions to pull data to skb is commonly used with the same pattern, and
> not only for ETH_HLEN. E.g., I randomly found below in kernel source code.
> 
> 1071 static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
> 1072 {
> ... ...
> 1102         pulled_sci = pskb_may_pull(skb, macsec_extra_len(true));
> 1103         if (!pulled_sci) {
> 1104                 if (!pskb_may_pull(skb, macsec_extra_len(false)))
> 1105                         goto drop_direct;
> 1106         }
> ... ...
> 1254 drop_direct:
> 1255         kfree_skb(skb);
> 1256         *pskb = NULL;
> 1257         return RX_HANDLER_CONSUMED;
> 
> 
> About 'L2_HDR_ERR', I am curious what the user/administrator may do as next
> step, while the 'SKB_PULL' will be very clear to the developers which kernel
> operation (e.g., to pull some protocol/hdr data to sk_buff data) is with the issue.
> 
> I may use 'L2_HDR_ERR' if you prefer.

We don't have to break it out per layer if you prefer. Let's call it
HDR_TRUNC.

I don't like SKB_PULL, people using these trace points are as likely 
to be BPF developers as kernel developers and skb_pull will not be
meaningful to them. Besides the code can check if header is not
truncated in other ways than pskb_may_pull(). And calling things 
by the name of the helper that failed is bad precedent.

> > For SKB_TRIM the error comes from allocation failures, there may be
> > a whole bunch of skb helpers which will fail only under mem pressure,
> > would it be better to identify them and return some ENOMEM related
> > reason, since, most likely, those will be noise to whoever is tracking
> > real errors?  
> 
> The reasons I want to use SKB_TRIM:
> 
> 1. To have SKB_PULL and SKB_TRIM (perhaps more SKB_XXX in the future in the same
> set).
> 
> 2. Although so that SKB_TRIM is always caused by ENOMEM, suppose if there is new
> return values by pskb_trim(), the reason is not going to be valid any longer.
> 
> 
> I may use SKB_DROP_REASON_NOMEM if you prefer.
> 
> Another concern is that many functions may return -ENOMEM. It is more likely
> that if there are two "goto drop" to return -ENOMEM, we will not be able to tell
> from which function the sk_buff is dropped, e.g.,
> 
> if (function_A()) {
>     reason = -ENOMEM;
>     goto drop;
> }
> 
> if (function_B()) {
>     reason = -ENOMEM;
>     goto drop;
> }

Are you saying that you're intending to break out skb drop reasons 
by what entity failed to allocate memory? I'd think "skb was dropped
because of OOM" is what should be reported. What we were trying to
allocate is not very relevant (and can be gotten from the stack trace 
if needed).

> >>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
> >>  					 * device driver specific header
> >>  					 */
> >> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */  
> > 
> > What is ready? link is not up? peer not connected? can we expand?
> 
> In this patchset, it is for either:
> 
> - tun->tfiles[txq] is not set, or
> 
> - !(tun->dev->flags & IFF_UP)
> 
> I want to make it very generic so that the sk_buff dropped due to any device
> level data structure that is not up/ready/initialized/allocated will use this
> reason in the future.

Let's expand the documentation so someone reading thru the enum can
feel confident if they are using this reason correctly.

Side note - you may want to switch to inline comments to make writing
more verbose documentation, I mean:

	/* This is the explanation of reason one which explains what
	 * reason ones means, and how it should be used. We can make
	 * use of full line width this way.
         */
	SKB_DROP_REASON_ONE,
	/* And this is an explanation for reason two. */
	SKB_DROP_REASON_TWO,

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Jakub,

On 3/2/22 11:17 AM, Jakub Kicinski wrote:
> On Wed, 2 Mar 2022 10:19:30 -0800 Dongli Zhang wrote:
>> On 3/1/22 6:50 PM, Jakub Kicinski wrote:
>>> On Sat, 26 Feb 2022 00:49:29 -0800 Dongli Zhang wrote:  
>>>> +	SKB_DROP_REASON_SKB_PULL,	/* failed to pull sk_buff data */
>>>> +	SKB_DROP_REASON_SKB_TRIM,	/* failed to trim sk_buff data */  
>>>
>>> IDK if these are not too low level and therefore lacking meaning.
>>>
>>> What are your thoughts David?
>>>
>>> Would it be better to up level the names a little bit and call SKB_PULL
>>> something like "HDR_TRUNC" or "HDR_INV" or "HDR_ERR" etc or maybe
>>> "L2_HDR_ERR" since in this case we seem to be pulling off ETH_HLEN?  
>>
>> This is for device driver and I think for most of cases the people understanding
>> source code will be involved. I think SKB_PULL is more meaningful to people
>> understanding source code.
>>
>> The functions to pull data to skb is commonly used with the same pattern, and
>> not only for ETH_HLEN. E.g., I randomly found below in kernel source code.
>>
>> 1071 static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>> 1072 {
>> ... ...
>> 1102         pulled_sci = pskb_may_pull(skb, macsec_extra_len(true));
>> 1103         if (!pulled_sci) {
>> 1104                 if (!pskb_may_pull(skb, macsec_extra_len(false)))
>> 1105                         goto drop_direct;
>> 1106         }
>> ... ...
>> 1254 drop_direct:
>> 1255         kfree_skb(skb);
>> 1256         *pskb = NULL;
>> 1257         return RX_HANDLER_CONSUMED;
>>
>>
>> About 'L2_HDR_ERR', I am curious what the user/administrator may do as next
>> step, while the 'SKB_PULL' will be very clear to the developers which kernel
>> operation (e.g., to pull some protocol/hdr data to sk_buff data) is with the issue.
>>
>> I may use 'L2_HDR_ERR' if you prefer.
> 
> We don't have to break it out per layer if you prefer. Let's call it
> HDR_TRUNC.
> 
> I don't like SKB_PULL, people using these trace points are as likely 
> to be BPF developers as kernel developers and skb_pull will not be
> meaningful to them. Besides the code can check if header is not
> truncated in other ways than pskb_may_pull(). And calling things 
> by the name of the helper that failed is bad precedent.

I will switch to SKB_DROP_REASON_HDR_TRUNC.

> 
>>> For SKB_TRIM the error comes from allocation failures, there may be
>>> a whole bunch of skb helpers which will fail only under mem pressure,
>>> would it be better to identify them and return some ENOMEM related
>>> reason, since, most likely, those will be noise to whoever is tracking
>>> real errors?  
>>
>> The reasons I want to use SKB_TRIM:
>>
>> 1. To have SKB_PULL and SKB_TRIM (perhaps more SKB_XXX in the future in the same
>> set).
>>
>> 2. Although so that SKB_TRIM is always caused by ENOMEM, suppose if there is new
>> return values by pskb_trim(), the reason is not going to be valid any longer.
>>
>>
>> I may use SKB_DROP_REASON_NOMEM if you prefer.
>>
>> Another concern is that many functions may return -ENOMEM. It is more likely
>> that if there are two "goto drop" to return -ENOMEM, we will not be able to tell
>> from which function the sk_buff is dropped, e.g.,
>>
>> if (function_A()) {
>>     reason = -ENOMEM;
>>     goto drop;
>> }
>>
>> if (function_B()) {
>>     reason = -ENOMEM;
>>     goto drop;
>> }
> 
> Are you saying that you're intending to break out skb drop reasons 
> by what entity failed to allocate memory? I'd think "skb was dropped

Yes.

> because of OOM" is what should be reported. What we were trying to
> allocate is not very relevant (and can be gotten from the stack trace 
> if needed).

I think OOM is not enough. Although it may not be the case in this patchset,
sometimes the allocation is failed because we are allocating a large chunk of
physically continuous pages (kmalloc vs. vmalloc) while there is still plenty of
memory pages available.

As a kernel developer, it is very significant for me to identify the specific
line/function and specific data structure that cause the error. E.g, the bug
filer may be chasing which line is making trouble.

It is less likely to SKB_TRIM more than once in a driver function, compared to
ENOMEM.

I am the user of this patchset and I prefer to make my work easier in the future :)

> 
>>>>  	SKB_DROP_REASON_DEV_HDR,	/* there is something wrong with
>>>>  					 * device driver specific header
>>>>  					 */
>>>> +	SKB_DROP_REASON_DEV_READY,	/* device is not ready */  
>>>
>>> What is ready? link is not up? peer not connected? can we expand?
>>
>> In this patchset, it is for either:
>>
>> - tun->tfiles[txq] is not set, or
>>
>> - !(tun->dev->flags & IFF_UP)
>>
>> I want to make it very generic so that the sk_buff dropped due to any device
>> level data structure that is not up/ready/initialized/allocated will use this
>> reason in the future.
> 
> Let's expand the documentation so someone reading thru the enum can
> feel confident if they are using this reason correctly.
> 
> Side note - you may want to switch to inline comments to make writing
> more verbose documentation, I mean:
> 
> 	/* This is the explanation of reason one which explains what
> 	 * reason ones means, and how it should be used. We can make
> 	 * use of full line width this way.
>          */
> 	SKB_DROP_REASON_ONE,
> 	/* And this is an explanation for reason two. */
> 	SKB_DROP_REASON_TWO,
> 

I will expand the comments.

Thank you very much!

Dongli Zhang

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Jakub Kicinski]

On Wed, 2 Mar 2022 14:21:31 -0800 Dongli Zhang wrote:
> > because of OOM" is what should be reported. What we were trying to
> > allocate is not very relevant (and can be gotten from the stack trace 
> > if needed).  
> 
> I think OOM is not enough. Although it may not be the case in this patchset,
> sometimes the allocation is failed because we are allocating a large chunk of
> physically continuous pages (kmalloc vs. vmalloc) while there is still plenty of
> memory pages available.
> 
> As a kernel developer, it is very significant for me to identify the specific
> line/function and specific data structure that cause the error. E.g, the bug
> filer may be chasing which line is making trouble.
> 
> It is less likely to SKB_TRIM more than once in a driver function, compared to
> ENOMEM.

Nack, trim is meaningless.

Re: [PATCH net-next v4 4/4] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Jakub,

On 3/2/22 9:21 PM, Jakub Kicinski wrote:
> On Wed, 2 Mar 2022 14:21:31 -0800 Dongli Zhang wrote:
>>> because of OOM" is what should be reported. What we were trying to
>>> allocate is not very relevant (and can be gotten from the stack trace 
>>> if needed).  
>>
>> I think OOM is not enough. Although it may not be the case in this patchset,
>> sometimes the allocation is failed because we are allocating a large chunk of
>> physically continuous pages (kmalloc vs. vmalloc) while there is still plenty of
>> memory pages available.
>>
>> As a kernel developer, it is very significant for me to identify the specific
>> line/function and specific data structure that cause the error. E.g, the bug
>> filer may be chasing which line is making trouble.
>>
>> It is less likely to SKB_TRIM more than once in a driver function, compared to
>> ENOMEM.
> 
> Nack, trim is meaningless.
> 

I will use SKB_DROP_REASON_NOMEM.

Thank you very much!

Dongli Zhang