netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling
@ 2025-06-06 16:51 Eric Dumazet
  2025-06-06 18:37 ` Toke Høiland-Jørgensen
  2025-06-09 22:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 3+ messages in thread
From: Eric Dumazet @ 2025-06-06 16:51 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Paolo Abeni
  Cc: Simon Horman, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
	Toke Høiland-Jørgensen, netdev, eric.dumazet,
	Eric Dumazet, Marcus Wichelmann

SFQ has an assumption of always being able to queue at least one packet.

However, after the blamed commit, sch->q.len can be inflated by packets
in sch->gso_skb, and an enqueue() on an empty SFQ qdisc can be followed
by an immediate drop.

Fix sfq_drop() to properly clear q->tail in this situation.

Tested:

ip netns add lb
ip link add dev to-lb type veth peer name in-lb netns lb
ethtool -K to-lb tso off                 # force qdisc to requeue gso_skb
ip netns exec lb ethtool -K in-lb gro on # enable NAPI
ip link set dev to-lb up
ip -netns lb link set dev in-lb up
ip addr add dev to-lb 192.168.20.1/24
ip -netns lb addr add dev in-lb 192.168.20.2/24
tc qdisc replace dev to-lb root sfq limit 100

ip netns exec lb netserver

netperf -H 192.168.20.2 -l 100 &
netperf -H 192.168.20.2 -l 100 &
netperf -H 192.168.20.2 -l 100 &
netperf -H 192.168.20.2 -l 100 &

Fixes: a53851e2c321 ("net: sched: explicit locking in gso_cpu fallback")
Reported-by: Marcus Wichelmann <marcus.wichelmann@hetzner-cloud.de>
Closes: https://lore.kernel.org/netdev/9da42688-bfaa-4364-8797-e9271f3bdaef@hetzner-cloud.de/
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_sfq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index b912ad99aa15d95b297fb28d0fd0baa9c21ab5cd..77fa02f2bfcd56a36815199aa2e7987943ea226f 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -310,7 +310,10 @@ static unsigned int sfq_drop(struct Qdisc *sch, struct sk_buff **to_free)
 		/* It is difficult to believe, but ALL THE SLOTS HAVE LENGTH 1. */
 		x = q->tail->next;
 		slot = &q->slots[x];
-		q->tail->next = slot->next;
+		if (slot->next == x)
+			q->tail = NULL; /* no more active slots */
+		else
+			q->tail->next = slot->next;
 		q->ht[slot->hash] = SFQ_EMPTY_SLOT;
 		goto drop;
 	}
-- 
2.50.0.rc0.604.gd4ff7b7c86-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling
  2025-06-06 16:51 [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling Eric Dumazet
@ 2025-06-06 18:37 ` Toke Høiland-Jørgensen
  2025-06-09 22:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: Toke Høiland-Jørgensen @ 2025-06-06 18:37 UTC (permalink / raw)
  To: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni
  Cc: Simon Horman, Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev,
	eric.dumazet, Eric Dumazet, Marcus Wichelmann

Eric Dumazet <edumazet@google.com> writes:

> SFQ has an assumption of always being able to queue at least one packet.
>
> However, after the blamed commit, sch->q.len can be inflated by packets
> in sch->gso_skb, and an enqueue() on an empty SFQ qdisc can be followed
> by an immediate drop.
>
> Fix sfq_drop() to properly clear q->tail in this situation.
>
> Tested:
>
> ip netns add lb
> ip link add dev to-lb type veth peer name in-lb netns lb
> ethtool -K to-lb tso off                 # force qdisc to requeue gso_skb
> ip netns exec lb ethtool -K in-lb gro on # enable NAPI
> ip link set dev to-lb up
> ip -netns lb link set dev in-lb up
> ip addr add dev to-lb 192.168.20.1/24
> ip -netns lb addr add dev in-lb 192.168.20.2/24
> tc qdisc replace dev to-lb root sfq limit 100
>
> ip netns exec lb netserver
>
> netperf -H 192.168.20.2 -l 100 &
> netperf -H 192.168.20.2 -l 100 &
> netperf -H 192.168.20.2 -l 100 &
> netperf -H 192.168.20.2 -l 100 &
>
> Fixes: a53851e2c321 ("net: sched: explicit locking in gso_cpu fallback")
> Reported-by: Marcus Wichelmann <marcus.wichelmann@hetzner-cloud.de>
> Closes: https://lore.kernel.org/netdev/9da42688-bfaa-4364-8797-e9271f3bdaef@hetzner-cloud.de/
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks for the quick fix!

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling
  2025-06-06 16:51 [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling Eric Dumazet
  2025-06-06 18:37 ` Toke Høiland-Jørgensen
@ 2025-06-09 22:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-06-09 22:50 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: davem, kuba, pabeni, horms, jhs, xiyou.wangcong, jiri, toke,
	netdev, eric.dumazet, marcus.wichelmann

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  6 Jun 2025 16:51:27 +0000 you wrote:
> SFQ has an assumption of always being able to queue at least one packet.
> 
> However, after the blamed commit, sch->q.len can be inflated by packets
> in sch->gso_skb, and an enqueue() on an empty SFQ qdisc can be followed
> by an immediate drop.
> 
> Fix sfq_drop() to properly clear q->tail in this situation.
> 
> [...]

Here is the summary with links:
  - [net] net_sched: sch_sfq: fix a potential crash on gso_skb handling
    https://git.kernel.org/netdev/net/c/82ffbe7776d0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-06-09 22:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-06 16:51 [PATCH net] net_sched: sch_sfq: fix a potential crash on gso_skb handling Eric Dumazet
2025-06-06 18:37 ` Toke Høiland-Jørgensen
2025-06-09 22:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).