Re: [PATCH net-next v2 4/4] netdevsim: account dropped packet length in stats on queue free

[Breno Leitao <leitao@debian.org>]

On Fri, Jun 13, 2025 at 03:40:01AM -0700, Breno Leitao wrote:
> Add a call to dev_dstats_rx_dropped_add() in nsim_queue_free() to
> account for the number of packets dropped when purging the skb queue.
> 
> This improves the accuracy of RX drop statistics reported by
> netdevsim.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Breno Leitao <leitao@debian.org>

> ---
>  drivers/net/netdevsim/netdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index de309ff69e43e..6e8fb8922ace2 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -634,7 +634,10 @@ static struct nsim_rq *nsim_queue_alloc(void)
>  
>  static void nsim_queue_free(struct nsim_rq *rq)
>  {
> +	struct net_device *dev = rq->napi.dev;
> +
>  	hrtimer_cancel(&rq->napi_timer);
> +	dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);

This is wrong and it will cause the kernel to crash in some cases, given
we can get here with dev == NULL, in the following path:

	nsim_destroy (drivers/net/netdevsim/netdev.c:1067) netdevsim
	nsim_dev_reload_destroy (drivers/net/netdevsim/dev.c:426 drivers/net/netdevsim/dev.c:1429 drivers/net/netdevsim/dev.c:1440 drivers/net/netdevsim/dev.c:1661) netdevsim
	nsim_dev_reload_down (drivers/net/netdevsim/dev.c:?) netdevsim
	devlink_reload (net/devlink/dev.c:462)
	? lock_acquire (kernel/locking/lockdep.c:5871)
	? devlink_remote_reload_actions_performed (net/devlink/dev.c:446)
	devlink_nl_reload_doit (net/devlink/dev.c:?)
	? devlink_reload (net/devlink/dev.c:520)
	? __nla_parse (lib/nlattr.c:732)
	genl_family_rcv_msg_doit (net/netlink/genetlink.c:1115)
	? genl_family_rcv_msg_dumpit (net/netlink/genetlink.c:1088)
	genl_rcv_msg (net/netlink/genetlink.c:? net/netlink/genetlink.c:1210)
	? genl_release (net/netlink/genetlink.c:1201)
	? devlink_nl_pre_doit_port (net/devlink/netlink.c:257)
	? devlink_reload (net/devlink/dev.c:520)
	? devlink_nl_post_doit (net/devlink/netlink.c:288)
	? __lock_acquire (kernel/locking/lockdep.c:?)
	netlink_rcv_skb (net/netlink/af_netlink.c:2534)
	? genl_release (net/netlink/genetlink.c:1201)
	? netlink_ack_tlv_fill (net/netlink/af_netlink.c:2511)
	? down_read (./arch/x86/include/asm/atomic64_64.h:20 ./include/linux/atomic/atomic-arch-fallback.h:2629 ./include/linux/atomic/atomic-long.h:79 ./include/linux/atomic/atomic-instrumented.h:3224 kernel/locking/rwsem.c:176 kernel/locking/rwsem.c:181 kernel/locking/rwsem.c:256 kernel/locking/rwsem.c:1247 kernel/locking/rwsem.c:1261 kernel/locking/rwsem.c:1526)
	genl_rcv (net/netlink/genetlink.c:1220)
	netlink_unicast (net/netlink/af_netlink.c:1314 net/netlink/af_netlink.c:1339)
	netlink_sendmsg (net/netlink/af_netlink.c:1883)
	? netlink_getsockopt (net/netlink/af_netlink.c:1802)
	? __might_fault (mm/memory.c:6991)
	__sys_sendto (net/socket.c:715 net/socket.c:727 net/socket.c:2180)
	? __ia32_sys_getpeername (net/socket.c:2147)
	? __might_fault (mm/memory.c:6991)
	? __bpf_trace_rseq_ip_fixup (kernel/rseq.c:425)
	__x64_sys_sendto (net/socket.c:2187 net/socket.c:2183 net/socket.c:2183)
	? entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
	do_syscall_64 (arch/x86/entry/syscall_64.c:?)
	? exc_page_fault (arch/x86/mm/fault.c:1536)
	entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)

I am wondering if this additional patch is too ugly:

	diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
	index 6e8fb8922ace2..328c7e83f2823 100644
	--- a/drivers/net/netdevsim/netdev.c
	+++ b/drivers/net/netdevsim/netdev.c
	@@ -637,7 +637,8 @@ static void nsim_queue_free(struct nsim_rq *rq)
		struct net_device *dev = rq->napi.dev;

		hrtimer_cancel(&rq->napi_timer);
	-       dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);
	+       if (dev)
	+               dev_dstats_rx_dropped_add(dev, rq->skb_queue.qlen);
		skb_queue_purge_reason(&rq->skb_queue, SKB_DROP_REASON_QUEUE_PURGE);
		kfree(rq);
	}