[PATCH net-next 0/8] net: few debug refinements

[PATCH net-next 0/8] net: few debug refinements

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Adopt DEBUG_NET_WARN_ON_ONCE() in some places where it makes sense.

Add checks in napi_consume_skb() and __napi_alloc_skb()

Make sure napi_get_frags() does not use page fragments
for skb->head.

Eric Dumazet (8):
  net: use DEBUG_NET_WARN_ON_ONCE() in __release_sock()
  net: use DEBUG_NET_WARN_ON_ONCE() in dev_loopback_xmit()
  net: use DEBUG_NET_WARN_ON_ONCE() in inet_sock_destruct()
  net: use DEBUG_NET_WARN_ON_ONCE() in sk_stream_kill_queues()
  af_unix: use DEBUG_NET_WARN_ON_ONCE()
  net: use DEBUG_NET_WARN_ON_ONCE() in skb_release_head_state()
  net: add debug checks in napi_consume_skb and __napi_alloc_skb()
  net: add napi_get_frags_check() helper

 net/core/dev.c     | 20 +++++++++++++++++++-
 net/core/skbuff.c  |  5 +++--
 net/core/sock.c    |  2 +-
 net/core/stream.c  |  6 +++---
 net/ipv4/af_inet.c |  8 ++++----
 net/unix/af_unix.c |  8 ++++----
 6 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 1/8] net: use DEBUG_NET_WARN_ON_ONCE() in __release_sock()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Check against skb dst in socket backlog has never triggered
in past years.

Keep the check only for CONFIG_DEBUG_NET=y builds.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 2ff40dd0a7a652029cca1743109286b50c2a17f3..f5062d9e122275a511efbc4d30de2ee501182498 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2844,7 +2844,7 @@ void __release_sock(struct sock *sk)
 		do {
 			next = skb->next;
 			prefetch(next);
-			WARN_ON_ONCE(skb_dst_is_noref(skb));
+			DEBUG_NET_WARN_ON_ONCE(skb_dst_is_noref(skb));
 			skb_mark_not_on_list(skb);
 			sk_backlog_rcv(sk, skb);
 
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 2/8] net: use DEBUG_NET_WARN_ON_ONCE() in dev_loopback_xmit()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

One check in dev_loopback_xmit() has not caught issues
in the past.

Keep it for CONFIG_DEBUG_NET=y builds only.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 08ce317fcec89609f6f8e9335b3d9f57e813024d..27ad09ad80a4550097ce4d113719a558b5e2a811 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3925,7 +3925,7 @@ int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
 	skb->pkt_type = PACKET_LOOPBACK;
 	if (skb->ip_summed == CHECKSUM_NONE)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
-	WARN_ON(!skb_dst(skb));
+	DEBUG_NET_WARN_ON_ONCE(!skb_dst(skb));
 	skb_dst_force(skb);
 	netif_rx(skb);
 	return 0;
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 3/8] net: use DEBUG_NET_WARN_ON_ONCE() in inet_sock_destruct()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

inet_sock_destruct() has four warnings which have been
useful to point to kernel bugs in the past.

However they are potentially a problem because they
could flood the syslog, and really only a developper
can make sense of them.

Keep the checks for CONFIG_DEBUG_NET=y builds,
and issue them once only.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/af_inet.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 93da9f783bec52e4bda6213dc78ef3820e180cc4..5ec85ffe1e13afb333e40819b2a5e60b003d44a7 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -148,10 +148,10 @@ void inet_sock_destruct(struct sock *sk)
 		return;
 	}
 
-	WARN_ON(atomic_read(&sk->sk_rmem_alloc));
-	WARN_ON(refcount_read(&sk->sk_wmem_alloc));
-	WARN_ON(sk->sk_wmem_queued);
-	WARN_ON(sk_forward_alloc_get(sk));
+	DEBUG_NET_WARN_ON_ONCE(atomic_read(&sk->sk_rmem_alloc));
+	DEBUG_NET_WARN_ON_ONCE(refcount_read(&sk->sk_wmem_alloc));
+	DEBUG_NET_WARN_ON_ONCE(sk->sk_wmem_queued);
+	DEBUG_NET_WARN_ON_ONCE(sk_forward_alloc_get(sk));
 
 	kfree(rcu_dereference_protected(inet->inet_opt, 1));
 	dst_release(rcu_dereference_protected(sk->sk_dst_cache, 1));
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 4/8] net: use DEBUG_NET_WARN_ON_ONCE() in sk_stream_kill_queues()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

sk_stream_kill_queues() has three checks which have been
useful to detect kernel bugs in the past.

However they are potentially a problem because they
could flood the syslog, and really only a developper
can make sense of them.

Keep the checks for CONFIG_DEBUG_NET=y builds,
and issue them once only.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/stream.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/stream.c b/net/core/stream.c
index 06b36c730ce8a29bb2d8984495e780931907ca72..a5aa3620be95574c6d0f371f5943bb3b8f36cb4c 100644
--- a/net/core/stream.c
+++ b/net/core/stream.c
@@ -196,13 +196,13 @@ void sk_stream_kill_queues(struct sock *sk)
 	__skb_queue_purge(&sk->sk_receive_queue);
 
 	/* Next, the write queue. */
-	WARN_ON(!skb_queue_empty(&sk->sk_write_queue));
+	DEBUG_NET_WARN_ON_ONCE(!skb_queue_empty(&sk->sk_write_queue));
 
 	/* Account for returned memory. */
 	sk_mem_reclaim_final(sk);
 
-	WARN_ON(sk->sk_wmem_queued);
-	WARN_ON(sk->sk_forward_alloc);
+	DEBUG_NET_WARN_ON_ONCE(sk->sk_wmem_queued);
+	DEBUG_NET_WARN_ON_ONCE(sk->sk_forward_alloc);
 
 	/* It is _impossible_ for the backlog to contain anything
 	 * when we get here.  All user references to this socket
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH net-next 4/8] net: use DEBUG_NET_WARN_ON_ONCE() in sk_stream_kill_queues()

[Jakub Kicinski]

On Tue,  7 Jun 2022 10:17:28 -0700 Eric Dumazet wrote:
> sk_stream_kill_queues() has three checks which have been
> useful to detect kernel bugs in the past.
> 
> However they are potentially a problem because they
> could flood the syslog, and really only a developper
> can make sense of them.
> 
> Keep the checks for CONFIG_DEBUG_NET=y builds,
> and issue them once only.

I feel like 3 & 4 had caught plenty of bugs which triggered only 
in production / at scale. In my head DEBUG_NET_WARN_ON_ONCE() is 
great for things we are relatively sure syzbot will trigger.
Am I mis-characterizing things or should we WARN_ON_ONCE() those?

Re: [PATCH net-next 4/8] net: use DEBUG_NET_WARN_ON_ONCE() in sk_stream_kill_queues()

[Paolo Abeni]

On Tue, 2022-06-07 at 21:10 -0700, Jakub Kicinski wrote:
> On Tue,  7 Jun 2022 10:17:28 -0700 Eric Dumazet wrote:
> > sk_stream_kill_queues() has three checks which have been
> > useful to detect kernel bugs in the past.
> > 
> > However they are potentially a problem because they
> > could flood the syslog, and really only a developper
> > can make sense of them.
> > 
> > Keep the checks for CONFIG_DEBUG_NET=y builds,
> > and issue them once only.
> 
> I feel like 3 & 4 had caught plenty of bugs which triggered only 
> in production / at scale. 
> 
I have a somewhat similar experience: I hit a few races spotted by the
warnings in patches 3 and 4 observable only in non-debug build.

The checks in patch 4 are almost rendundant with the ones in patch 3 -
at least in my experience I could not trigger the first without hitting
the latter. Perhaps we could use WARN_ON_ONCE only in patch 3?

Thanks!

Paolo

Re: [PATCH net-next 4/8] net: use DEBUG_NET_WARN_ON_ONCE() in sk_stream_kill_queues()

[Eric Dumazet]

On Wed, Jun 8, 2022 at 1:11 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2022-06-07 at 21:10 -0700, Jakub Kicinski wrote:
> > On Tue,  7 Jun 2022 10:17:28 -0700 Eric Dumazet wrote:
> > > sk_stream_kill_queues() has three checks which have been
> > > useful to detect kernel bugs in the past.
> > >
> > > However they are potentially a problem because they
> > > could flood the syslog, and really only a developper
> > > can make sense of them.
> > >
> > > Keep the checks for CONFIG_DEBUG_NET=y builds,
> > > and issue them once only.
> >
> > I feel like 3 & 4 had caught plenty of bugs which triggered only
> > in production / at scale.
> >
> I have a somewhat similar experience: I hit a few races spotted by the
> warnings in patches 3 and 4 observable only in non-debug build.
>
> The checks in patch 4 are almost rendundant with the ones in patch 3 -
> at least in my experience I could not trigger the first without hitting
> the latter. Perhaps we could use WARN_ON_ONCE only in patch 3?
>

Well, I certainly can stick to WARN_ON_ONCE().

For syzbot it is really the same thing.

[PATCH net-next 5/8] af_unix: use DEBUG_NET_WARN_ON_ONCE()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Replace four WARN_ON() that have not triggered recently
with DEBUG_NET_WARN_ON_ONCE().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/unix/af_unix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 654dcef7cfb3849463be9d905ae625319fbae406..ae254195aac87f196a93853443048b40e332cc60 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -302,7 +302,7 @@ static void __unix_remove_socket(struct sock *sk)
 
 static void __unix_insert_socket(struct sock *sk)
 {
-	WARN_ON(!sk_unhashed(sk));
+	DEBUG_NET_WARN_ON_ONCE(!sk_unhashed(sk));
 	sk_add_node(sk, &unix_socket_table[sk->sk_hash]);
 }
 
@@ -554,9 +554,9 @@ static void unix_sock_destructor(struct sock *sk)
 		u->oob_skb = NULL;
 	}
 #endif
-	WARN_ON(refcount_read(&sk->sk_wmem_alloc));
-	WARN_ON(!sk_unhashed(sk));
-	WARN_ON(sk->sk_socket);
+	DEBUG_NET_WARN_ON_ONCE(refcount_read(&sk->sk_wmem_alloc));
+	DEBUG_NET_WARN_ON_ONCE(!sk_unhashed(sk));
+	DEBUG_NET_WARN_ON_ONCE(sk->sk_socket);
 	if (!sock_flag(sk, SOCK_DEAD)) {
 		pr_info("Attempt to release alive unix socket: %p\n", sk);
 		return;
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 6/8] net: use DEBUG_NET_WARN_ON_ONCE() in skb_release_head_state()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Remove this check from fast path unless CONFIG_DEBUG_NET=y

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b661040c100efbb59aea58ed31247f820292bd64..cf83d9b8f41dc28ae11391f2651e5abee3dcde8f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -728,7 +728,7 @@ void skb_release_head_state(struct sk_buff *skb)
 {
 	skb_dst_drop(skb);
 	if (skb->destructor) {
-		WARN_ON(in_hardirq());
+		DEBUG_NET_WARN_ON_ONCE(in_hardirq());
 		skb->destructor(skb);
 	}
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 7/8] net: add debug checks in napi_consume_skb and __napi_alloc_skb()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Commit 6454eca81eae ("net: Use lockdep_assert_in_softirq()
in napi_consume_skb()") added a check in napi_consume_skb()
which is a bit weak.

napi_consume_skb() and __napi_alloc_skb() should only
be used from BH context, not from hard irq or nmi context,
otherwise we could have races.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index cf83d9b8f41dc28ae11391f2651e5abee3dcde8f..fec75f8bf1f416f05c14f76009a15412b2559b6e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -560,6 +560,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
 	struct sk_buff *skb;
 	void *data;
 
+	DEBUG_NET_WARN_ON_ONCE(!in_softirq());
 	len += NET_SKB_PAD + NET_IP_ALIGN;
 
 	/* If requested length is either too small or too big,
@@ -981,7 +982,7 @@ void napi_consume_skb(struct sk_buff *skb, int budget)
 		return;
 	}
 
-	lockdep_assert_in_softirq();
+	DEBUG_NET_WARN_ON_ONCE(!in_softirq());
 
 	if (!skb_unref(skb))
 		return;
-- 
2.36.1.255.ge46751e96f-goog

[PATCH net-next 8/8] net: add napi_get_frags_check() helper

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

This is a follow up of commit 3226b158e67c
("net: avoid 32 x truesize under-estimation for tiny skbs")

When/if we increase MAX_SKB_FRAGS, we better make sure
the old bug will not come back.

Adding a check in napi_get_frags() would be costly,
even if using DEBUG_NET_WARN_ON_ONCE().

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 27ad09ad80a4550097ce4d113719a558b5e2a811..4ce9b2563a116066d85bae7a862e38fb160ef0e2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6351,6 +6351,23 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
 }
 EXPORT_SYMBOL(dev_set_threaded);
 
+/* Double check that napi_get_frags() allocates skbs with
+ * skb->head being backed by slab, not a page fragment.
+ * This is to make sure bug fixed in 3226b158e67c
+ * ("net: avoid 32 x truesize under-estimation for tiny skbs")
+ * does not accidentally come back.
+ */
+static void napi_get_frags_check(struct napi_struct *napi)
+{
+	struct sk_buff *skb;
+
+	local_bh_disable();
+	skb = napi_get_frags(napi);
+	WARN_ON_ONCE(skb && skb->head_frag);
+	napi_free_frags(napi);
+	local_bh_enable();
+}
+
 void netif_napi_add_weight(struct net_device *dev, struct napi_struct *napi,
 			   int (*poll)(struct napi_struct *, int), int weight)
 {
@@ -6378,6 +6395,7 @@ void netif_napi_add_weight(struct net_device *dev, struct napi_struct *napi,
 	set_bit(NAPI_STATE_NPSVC, &napi->state);
 	list_add_rcu(&napi->dev_list, &dev->napi_list);
 	napi_hash_add(napi);
+	napi_get_frags_check(napi);
 	/* Create kthread for this napi if dev->threaded is set.
 	 * Clear dev->threaded if kthread creation failed so that
 	 * threaded mode will not be enabled in napi_enable().
-- 
2.36.1.255.ge46751e96f-goog