[PATCH 0/2] tun/tap: use kfree_skb_reason() to trace dropped skb

[PATCH 0/2] 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. This is to
use kfree_skb_reason() to trace the dropped skb for those two drivers. The
tun and tap are commonly used as virtio-net/vhost-net backend.

This is the 'stacktrace' example for tap when the skb is dropped because
the ptr ring between tap and vhost-net is full.

    kworker/13:0-9759    [013] ..s1.  1439.053393: kfree_skb: skbaddr=000000004109db76 protocol=2054 location=00000000db8dd81c reason: PTR_FULL
    kworker/13:0-9759    [013] ..s2.  1439.053431: <stack trace>
 => trace_event_raw_event_kfree_skb
 => kfree_skb_reason.part.0
 => tap_handle_frame
 => __netif_receive_skb_core
 => __netif_receive_skb_one_core
 => process_backlog
 => __napi_poll
 => net_rx_action
 => __do_softirq
 => do_softirq.part.0
 => netif_rx_ni
 => macvlan_broadcast
 => macvlan_process_broadcast
 => process_one_work
 => worker_thread
 => kthread
 => ret_from_fork

 
This is the 'stacktrace' example for tun when the skb is dropped because
the ptr ring between run and vhost-net is full.

           <idle>-0       [000] b.s2.   499.675592: kfree_skb: skbaddr=00000000ff79867d protocol=2054 location=00000000635128db reason: PTR_FULL
          <idle>-0       [000] b.s3.   499.675612: <stack trace>
 => trace_event_raw_event_kfree_skb
 => kfree_skb_reason.part.0
 => tun_net_xmit
 => dev_hard_start_xmit
 => sch_direct_xmit
 => __dev_queue_xmit
 => br_dev_queue_push_xmit
 => br_handle_frame_finish
 => br_handle_frame
 => __netif_receive_skb_core
 => __netif_receive_skb_list_core
 => netif_receive_skb_list_internal
 => napi_complete_done
 => ixgbe_poll
 => __napi_poll
 => net_rx_action
 => __do_softirq
 => __irq_exit_rcu
 => common_interrupt
 => asm_common_interrupt
 => cpuidle_enter_state
 => cpuidle_enter
 => do_idle
 => cpu_startup_entry
 => start_kernel
 => secondary_startup_64_no_verify



 drivers/net/tap.c          | 30 ++++++++++++++++++++++--------
 drivers/net/tun.c          | 33 +++++++++++++++++++++++++--------
 include/linux/skbuff.h     | 11 +++++++++++
 include/trace/events/skb.h | 11 +++++++++++
 4 files changed, 69 insertions(+), 16 deletions(-)


Thank you very much!

Dongli Zhang

[PATCH 1/2] 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

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/net/tap.c          | 30 ++++++++++++++++++++++--------
 include/linux/skbuff.h     |  5 +++++
 include/trace/events/skb.h |  5 +++++
 3 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 8e3a28ba6b28..232572289e63 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;
+	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	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_SEGMENT;
 			goto drop;
+		}
 
 		if (!segs) {
-			if (ptr_ring_produce(&q->ring, skb))
+			if (ptr_ring_produce(&q->ring, skb)) {
+				drop_reason = SKB_DROP_REASON_PTR_FULL;
 				goto drop;
+			}
 			goto wake_up;
 		}
 
@@ -369,10 +374,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_CHECKSUM;
 			goto drop;
-		if (ptr_ring_produce(&q->ring, skb))
+		}
+		if (ptr_ring_produce(&q->ring, skb)) {
+			drop_reason = SKB_DROP_REASON_PTR_FULL;
 			goto drop;
+		}
 	}
 
 wake_up:
@@ -383,7 +392,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 +641,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 	int depth;
 	bool zerocopy = false;
 	size_t linear;
+	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	if (q->flags & IFF_VNET_HDR) {
 		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
@@ -696,8 +706,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 +718,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_VIRTNET_HDR;
 			goto err_kfree;
+		}
 	}
 
 	skb_probe_transport_header(skb);
@@ -738,7 +752,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 8a636e678902..16c30d2e20dc 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -320,6 +320,11 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TCP_CSUM,
 	SKB_DROP_REASON_SOCKET_FILTER,
 	SKB_DROP_REASON_UDP_CSUM,
+	SKB_DROP_REASON_SKB_GSO_SEGMENT,
+	SKB_DROP_REASON_SKB_CHECKSUM,
+	SKB_DROP_REASON_SKB_COPY_DATA,
+	SKB_DROP_REASON_PTR_FULL,
+	SKB_DROP_REASON_VIRTNET_HDR,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index a8a64b97504d..bf1509c31cea 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -16,6 +16,11 @@
 	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_SKB_GSO_SEGMENT, SKB_GSO_SEGMENT)	\
+	EM(SKB_DROP_REASON_SKB_CHECKSUM, SKB_CHECKSUM)		\
+	EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA)	\
+	EM(SKB_DROP_REASON_PTR_FULL, PTR_FULL)			\
+	EM(SKB_DROP_REASON_VIRTNET_HDR, VIRTNET_HDR)		\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
-- 
2.17.1

[PATCH 2/2] 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.

Cc: Joao Martins <joao.m.martins@oracle.com>
Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/net/tun.c          | 33 +++++++++++++++++++++++++--------
 include/linux/skbuff.h     |  6 ++++++
 include/trace/events/skb.h |  6 ++++++
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index fed85447701a..d67f2419dbb4 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct netdev_queue *queue;
 	struct tun_file *tfile;
 	int len = skb->len;
+	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	rcu_read_lock();
 	tfile = rcu_dereference(tun->tfiles[txq]);
 
 	/* Drop packet if interface is not attached */
-	if (!tfile)
+	if (!tfile) {
+		drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED;
 		goto drop;
+	}
 
 	if (!rcu_dereference(tun->steering_prog))
 		tun_automq_xmit(tun, skb);
@@ -1078,19 +1081,27 @@ 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_RUN_FILTER;
 		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_SKB_TRIM;
 		goto drop;
+	}
 
 	len = run_ebpf_filter(tun, skb, len);
-	if (len == 0 || pskb_trim(skb, len))
+	if (len == 0 || 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_ORPHAN_FRAGS;
 		goto drop;
+	}
 
 	skb_tx_timestamp(skb);
 
@@ -1101,8 +1112,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_PTR_FULL;
 		goto drop;
+	}
 
 	/* NETIF_F_LLTX requires to do our own update of trans_start */
 	queue = netdev_get_tx_queue(dev, txq);
@@ -1119,7 +1132,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;
 }
@@ -1717,6 +1730,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);
+	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	if (!(tun->flags & IFF_NO_PI)) {
 		if (len < sizeof(pi))
@@ -1820,9 +1834,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);
@@ -1869,6 +1884,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);
@@ -1922,6 +1938,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_DOWN;
 		goto drop;
 	}
 
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 16c30d2e20dc..db2ef8e8d878 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -323,8 +323,14 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_SKB_GSO_SEGMENT,
 	SKB_DROP_REASON_SKB_CHECKSUM,
 	SKB_DROP_REASON_SKB_COPY_DATA,
+	SKB_DROP_REASON_SKB_TRIM,
+	SKB_DROP_REASON_SKB_ORPHAN_FRAGS,
+	SKB_DROP_REASON_SKB_PULL,
+	SKB_DROP_REASON_DEV_NOT_ATTACHED,
+	SKB_DROP_REASON_DEV_DOWN,
 	SKB_DROP_REASON_PTR_FULL,
 	SKB_DROP_REASON_VIRTNET_HDR,
+	SKB_DROP_REASON_TAP_RUN_FILTER,
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index bf1509c31cea..03121373d2f0 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -19,8 +19,14 @@
 	EM(SKB_DROP_REASON_SKB_GSO_SEGMENT, SKB_GSO_SEGMENT)	\
 	EM(SKB_DROP_REASON_SKB_CHECKSUM, SKB_CHECKSUM)		\
 	EM(SKB_DROP_REASON_SKB_COPY_DATA, SKB_COPY_DATA)	\
+	EM(SKB_DROP_REASON_SKB_TRIM, SKB_TRIM)			\
+	EM(SKB_DROP_REASON_SKB_ORPHAN_FRAGS, SKB_ORPHAN_FRAGS)	\
+	EM(SKB_DROP_REASON_SKB_PULL, SKB_PULL)			\
+	EM(SKB_DROP_REASON_DEV_NOT_ATTACHED, DEV_NOT_ATTACHED)	\
+	EM(SKB_DROP_REASON_DEV_DOWN, DEV_DOWN)			\
 	EM(SKB_DROP_REASON_PTR_FULL, PTR_FULL)			\
 	EM(SKB_DROP_REASON_VIRTNET_HDR, VIRTNET_HDR)		\
+	EM(SKB_DROP_REASON_TAP_RUN_FILTER, TAP_RUN_FILTER)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
-- 
2.17.1

Re: [PATCH 1/2] net: tap: track dropped skb via kfree_skb_reason()

[Eric Dumazet]

On Mon, Feb 7, 2022 at 7:55 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
> 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
>
> Cc: Joao Martins <joao.m.martins@oracle.com>
> Cc: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  drivers/net/tap.c          | 30 ++++++++++++++++++++++--------
>  include/linux/skbuff.h     |  5 +++++
>  include/trace/events/skb.h |  5 +++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 8e3a28ba6b28..232572289e63 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;
> +       int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>
>         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_SEGMENT;
>                         goto drop;
> +               }
>
>                 if (!segs) {
> -                       if (ptr_ring_produce(&q->ring, skb))
> +                       if (ptr_ring_produce(&q->ring, skb)) {
> +                               drop_reason = SKB_DROP_REASON_PTR_FULL;

PTR_FULL is strange .... How about FULL_RING, or FULL_QUEUE ?

Re: [PATCH 1/2] net: tap: track dropped skb via kfree_skb_reason()

[David Ahern]

On 2/7/22 7:55 PM, Dongli Zhang wrote:
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index 8e3a28ba6b28..232572289e63 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;
> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;

maybe I missed an exit path, but I believe drop_reason is always set
before a goto jump, so this init is not needed.

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

This reason points to a line of code, not the real reason for the drop.
If you unwind __skb_gso_segment the only failure there is ENOMEM. The
reason code needs to be meaningful to users, not just code references.


>  			goto drop;
> +		}
>  
>  		if (!segs) {
> -			if (ptr_ring_produce(&q->ring, skb))
> +			if (ptr_ring_produce(&q->ring, skb)) {
> +				drop_reason = SKB_DROP_REASON_PTR_FULL;

similar comment to Eric - PTR_FULL needs to be more helpful.

>  				goto drop;
> +			}
>  			goto wake_up;
>  		}
>  
> @@ -369,10 +374,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_CHECKSUM;

That is not helpful explanation of the root cause; it is more of a code
reference.


>  			goto drop;
> -		if (ptr_ring_produce(&q->ring, skb))
> +		}
> +		if (ptr_ring_produce(&q->ring, skb)) {
> +			drop_reason = SKB_DROP_REASON_PTR_FULL;

ditto above comment

>  			goto drop;
> +		}
>  	}
>  
>  wake_up:
> @@ -383,7 +392,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 +641,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  	int depth;
>  	bool zerocopy = false;
>  	size_t linear;
> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  
>  	if (q->flags & IFF_VNET_HDR) {
>  		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
> @@ -696,8 +706,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;

As mentioned above, plus unwind the above functions and give a more
explicit description of why the above fails.

>  		goto err_kfree;
> +	}
>  
>  	skb_set_network_header(skb, ETH_HLEN);
>  	skb_reset_mac_header(skb);
> @@ -706,8 +718,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_VIRTNET_HDR;

and here too.

Re: [PATCH 2/2] net: tun: track dropped skb via kfree_skb_reason()

[David Ahern]

On 2/7/22 7:55 PM, 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.
> 
> Cc: Joao Martins <joao.m.martins@oracle.com>
> Cc: Joe Jin <joe.jin@oracle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  drivers/net/tun.c          | 33 +++++++++++++++++++++++++--------
>  include/linux/skbuff.h     |  6 ++++++
>  include/trace/events/skb.h |  6 ++++++
>  3 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index fed85447701a..d67f2419dbb4 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  	struct netdev_queue *queue;
>  	struct tun_file *tfile;
>  	int len = skb->len;
> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;


>  
>  	rcu_read_lock();
>  	tfile = rcu_dereference(tun->tfiles[txq]);
>  
>  	/* Drop packet if interface is not attached */
> -	if (!tfile)
> +	if (!tfile) {
> +		drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED;

That is going to be a confusing reason code (tap device existed to get
here) and does not really explain this error.


>  		goto drop;
> +	}
>  
>  	if (!rcu_dereference(tun->steering_prog))
>  		tun_automq_xmit(tun, skb);
> @@ -1078,19 +1081,27 @@ 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_RUN_FILTER;

just SKB_DROP_REASON_TAP_FILTER

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

SKB_DROP_REASON_SOCKET_FILTER

The remainder of your changes feels like another variant of your
previous "function / line" reason code. You are creating new reason
codes for every goto failure with a code based name. The reason needs to
be the essence of the failure in a user friendly label.

Re: [PATCH 1/2] net: tap: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi Eric,

On 2/7/22 8:32 PM, Eric Dumazet wrote:
> On Mon, Feb 7, 2022 at 7:55 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>
>> 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
>>
>> Cc: Joao Martins <joao.m.martins@oracle.com>
>> Cc: Joe Jin <joe.jin@oracle.com>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>  drivers/net/tap.c          | 30 ++++++++++++++++++++++--------
>>  include/linux/skbuff.h     |  5 +++++
>>  include/trace/events/skb.h |  5 +++++
>>  3 files changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
>> index 8e3a28ba6b28..232572289e63 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;
>> +       int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>
>>         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_SEGMENT;
>>                         goto drop;
>> +               }
>>
>>                 if (!segs) {
>> -                       if (ptr_ring_produce(&q->ring, skb))
>> +                       if (ptr_ring_produce(&q->ring, skb)) {
>> +                               drop_reason = SKB_DROP_REASON_PTR_FULL;
> 
> PTR_FULL is strange .... How about FULL_RING, or FULL_QUEUE ?
> 

I will use FULL_RING.

Thank you very much!

Dongli Zhang

Re: [PATCH 1/2] net: tap: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi David,

On 2/7/22 8:47 PM, David Ahern wrote:
> On 2/7/22 7:55 PM, Dongli Zhang wrote:
>> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
>> index 8e3a28ba6b28..232572289e63 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;
>> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> 
> maybe I missed an exit path, but I believe drop_reason is always set
> before a goto jump, so this init is not needed.

For tun/tap, the drop_reason is always set. I will avoid initialing the
'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_SEGMENT;
> 
> This reason points to a line of code, not the real reason for the drop.
> If you unwind __skb_gso_segment the only failure there is ENOMEM. The
> reason code needs to be meaningful to users, not just code references.

The __skb_gso_segment()->skb_mac_gso_segment() may return error as well.

Here I prefer to introduce a new reason because __skb_gso_segment() and
skb_gso_segment() are called at many locations.

E.g., to just select a driver that I never use.

2100 static void r8152_csum_workaround(struct r8152 *tp, struct sk_buff *skb,
2101                                   struct sk_buff_head *list)
... ...
2109                 segs = skb_gso_segment(skb, features);
2110                 if (IS_ERR(segs) || !segs)
2111                         goto drop;
... ...
2130 drop:
2131                 stats = &tp->netdev->stats;
2132                 stats->tx_dropped++;
2133                 dev_kfree_skb(skb);
2134         }

There are many such patterns. That's why I introduce a new reason for skb gso
segmentation so that developers may re-use it.

> 
> 
>>  			goto drop;
>> +		}
>>  
>>  		if (!segs) {
>> -			if (ptr_ring_produce(&q->ring, skb))
>> +			if (ptr_ring_produce(&q->ring, skb)) {
>> +				drop_reason = SKB_DROP_REASON_PTR_FULL;
> 
> similar comment to Eric - PTR_FULL needs to be more helpful.

I will use 'FULL_RING' as suggested by Eric.

> 
>>  				goto drop;
>> +			}
>>  			goto wake_up;
>>  		}
>>  
>> @@ -369,10 +374,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_CHECKSUM;
> 
> That is not helpful explanation of the root cause; it is more of a code
> reference.

To expand a function may generate a deep call graph. Any modification to the
callees in the call graph may have impact in the future.

The skb_checksum_help() is commonly used in the linux kernel to decide if there
is any error, e.g.,

 809 int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 810                  int (*output)(struct net *, struct sock *, struct sk_buff *))
 811 {
... ...
 859         if (skb->ip_summed == CHECKSUM_PARTIAL &&
 860             (err = skb_checksum_help(skb)))
 861                 goto fail;
... ...
 985 fail:
 986         IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
 987                       IPSTATS_MIB_FRAGFAILS);
 988         kfree_skb(skb);
 989         return err;
 990 }

That's why I prefer to introduce a new reason, which can be used by developers
for other subsystem/drivers.

> 
> 
>>  			goto drop;
>> -		if (ptr_ring_produce(&q->ring, skb))
>> +		}
>> +		if (ptr_ring_produce(&q->ring, skb)) {
>> +			drop_reason = SKB_DROP_REASON_PTR_FULL;
> 
> ditto above comment
I will use 'FULL_RING' as suggested by Eric.

> 
>>  			goto drop;
>> +		}
>>  	}
>>  
>>  wake_up:
>> @@ -383,7 +392,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 +641,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>>  	int depth;
>>  	bool zerocopy = false;
>>  	size_t linear;
>> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>  
>>  	if (q->flags & IFF_VNET_HDR) {
>>  		vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
>> @@ -696,8 +706,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;
> 
> As mentioned above, plus unwind the above functions and give a more
> explicit description of why the above fails.

The __zerocopy_sg_from_iter() and skb_copy_datagram_from_iter() are commonly
used by linux kernel. That's why a new reason is introduced. E.g.,

4924 int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
4925 {
... ...
4955         err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
4956         if (err)
4957                 goto err_free;
... ...
4969 err_free:
4970         kfree_skb(skb);

After expanding the function, one of failure is due to copy_from_iter(). I think
COPY_DATA is able to represent the failure at many locations involving the
function to copy data.

Any other places involving data copy failure (e.g., due to privilege issue,
use-after-free, length issue) may re-use this reason.

> 
>>  		goto err_kfree;
>> +	}
>>  
>>  	skb_set_network_header(skb, ETH_HLEN);
>>  	skb_reset_mac_header(skb);
>> @@ -706,8 +718,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_VIRTNET_HDR;
> 
> and here too.
> 

The virtio_net_hdr_to_skb() may return error for a variety of reasons. To simply
return -EFAULT does not help.

Indeed it may help at TAP/TUN if this is the only place returning -EFAULT.
However, it is not helpful when there are two "goto drop;" returning the same
-EFAULT.

Here I am trying to introduce a virtio-net specific reason, saying the
virtio-net header is invalid. Perhaps SKB_DROP_REASON_PV_HDR (for all PV
drivers) or SKB_DROP_REASON_INVALID_METADATA.


In my opinion, the kfree_skb_reason() is not for admin, but for developer.

The admin helps run tcpdump and narrows down the location and reason that the
pakcket is dropped, while the developer conducts code analysis to identify the
specific reason.

Therefore, I think the kfree_skb_reason() should:

1. Be friendly to user (admin) to collect data (e.g., via dropwatch/ebpf/ftrace).

2. Be friendly to developer to analyze the issue.

Thank you very much!

Dongli Zhang

Re: [PATCH 2/2] net: tun: track dropped skb via kfree_skb_reason()

[Dongli Zhang]

Hi David,

On 2/7/22 9:03 PM, David Ahern wrote:
> On 2/7/22 7:55 PM, 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.
>>
>> Cc: Joao Martins <joao.m.martins@oracle.com>
>> Cc: Joe Jin <joe.jin@oracle.com>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>  drivers/net/tun.c          | 33 +++++++++++++++++++++++++--------
>>  include/linux/skbuff.h     |  6 ++++++
>>  include/trace/events/skb.h |  6 ++++++
>>  3 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index fed85447701a..d67f2419dbb4 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -1062,13 +1062,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	struct netdev_queue *queue;
>>  	struct tun_file *tfile;
>>  	int len = skb->len;
>> +	int drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> 

I will avoid initializing here.

> 
>>  
>>  	rcu_read_lock();
>>  	tfile = rcu_dereference(tun->tfiles[txq]);
>>  
>>  	/* Drop packet if interface is not attached */
>> -	if (!tfile)
>> +	if (!tfile) {
>> +		drop_reason = SKB_DROP_REASON_DEV_NOT_ATTACHED;

Initially I was using TUN_NOT_ATTACHED. I used a more generic DEV_NOT_ATTACHED
in order to re-use the reason in the future.

How about TUN specific TUN_NOT_ATTACHED, as the core issue is because the below
is not hit.

rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);

> 
> That is going to be a confusing reason code (tap device existed to get
> here) and does not really explain this error.
> 
> 
>>  		goto drop;
>> +	}
>>  
>>  	if (!rcu_dereference(tun->steering_prog))
>>  		tun_automq_xmit(tun, skb);
>> @@ -1078,19 +1081,27 @@ 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_RUN_FILTER;
> 
> just SKB_DROP_REASON_TAP_FILTER

I will use SKB_DROP_REASON_TAP_FILTER.

> 
>>  		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_SKB_TRIM;
> 
> SKB_DROP_REASON_SOCKET_FILTER

Sorry for my mistake, I should have re-used this SKB_DROP_REASON_SOCKET_FILTER.

> 
> The remainder of your changes feels like another variant of your
> previous "function / line" reason code. You are creating new reason
> codes for every goto failure with a code based name. The reason needs to
> be the essence of the failure in a user friendly label.
> 

The remainder are:

- SKB_DROP_REASON_SKB_TRIM
- SKB_DROP_REASON_SKB_ORPHAN_FRAGS
- SKB_DROP_REASON_SKB_PULL
- SKB_DROP_REASON_DEV_DOWN
- SKB_DROP_REASON_SKB_COPY_DATA (introduced by Patch 1/2)

I tried to make them self-explaining and re-usable to other developers.

Yes, I am creating new reason codes for every goto failure with a code based
name because each function might be failed due to many reasons. In addition, I
need to avoid duplicate 'drop_reason' returned by a function in order to help
developer identify the specific line of code that the sk_buff is dropped.

Thank you very much!

Dongli Zhang