73 lines
3.4 KiB
Diff
73 lines
3.4 KiB
Diff
From 546ea84d07e3e324644025e2aae2d12ea4c5896e Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Toke=20H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>
|
|
Date: Tue, 3 Sep 2024 18:08:45 +0200
|
|
Subject: [PATCH] sched: sch_cake: fix bulk flow accounting logic for host
|
|
fairness
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
In sch_cake, we keep track of the count of active bulk flows per host,
|
|
when running in dst/src host fairness mode, which is used as the
|
|
round-robin weight when iterating through flows. The count of active
|
|
bulk flows is updated whenever a flow changes state.
|
|
|
|
This has a peculiar interaction with the hash collision handling: when a
|
|
hash collision occurs (after the set-associative hashing), the state of
|
|
the hash bucket is simply updated to match the new packet that collided,
|
|
and if host fairness is enabled, that also means assigning new per-host
|
|
state to the flow. For this reason, the bulk flow counters of the
|
|
host(s) assigned to the flow are decremented, before new state is
|
|
assigned (and the counters, which may not belong to the same host
|
|
anymore, are incremented again).
|
|
|
|
Back when this code was introduced, the host fairness mode was always
|
|
enabled, so the decrement was unconditional. When the configuration
|
|
flags were introduced the *increment* was made conditional, but
|
|
the *decrement* was not. Which of course can lead to a spurious
|
|
decrement (and associated wrap-around to U16_MAX).
|
|
|
|
AFAICT, when host fairness is disabled, the decrement and wrap-around
|
|
happens as soon as a hash collision occurs (which is not that common in
|
|
itself, due to the set-associative hashing). However, in most cases this
|
|
is harmless, as the value is only used when host fairness mode is
|
|
enabled. So in order to trigger an array overflow, sch_cake has to first
|
|
be configured with host fairness disabled, and while running in this
|
|
mode, a hash collision has to occur to cause the overflow. Then, the
|
|
qdisc has to be reconfigured to enable host fairness, which leads to the
|
|
array out-of-bounds because the wrapped-around value is retained and
|
|
used as an array index. It seems that syzbot managed to trigger this,
|
|
which is quite impressive in its own right.
|
|
|
|
This patch fixes the issue by introducing the same conditional check on
|
|
decrement as is used on increment.
|
|
|
|
The original bug predates the upstreaming of cake, but the commit listed
|
|
in the Fixes tag touched that code, meaning that this patch won't apply
|
|
before that.
|
|
|
|
Fixes: 712639929912 ("sch_cake: Make the dual modes fairer")
|
|
Reported-by: syzbot+7fe7b81d602cc1e6b94d@syzkaller.appspotmail.com
|
|
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
|
|
Link: https://patch.msgid.link/20240903160846.20909-1-toke@redhat.com
|
|
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
|
|
---
|
|
net/sched/sch_cake.c | 11 +++++++----
|
|
1 file changed, 7 insertions(+), 4 deletions(-)
|
|
|
|
--- a/net/sched/sch_cake.c
|
|
+++ b/net/sched/sch_cake.c
|
|
@@ -833,8 +833,10 @@ skip_hash:
|
|
allocate_dst = cake_ddst(flow_mode);
|
|
|
|
if (q->flows[outer_hash + k].set == CAKE_SET_BULK) {
|
|
- cake_dec_srchost_bulk_flow_count(q, &q->flows[outer_hash + k], flow_mode);
|
|
- cake_dec_dsthost_bulk_flow_count(q, &q->flows[outer_hash + k], flow_mode);
|
|
+ if (allocate_src)
|
|
+ q->hosts[q->flows[reduced_hash].srchost].srchost_bulk_flow_count--;
|
|
+ if (allocate_dst)
|
|
+ q->hosts[q->flows[reduced_hash].dsthost].dsthost_bulk_flow_count--;
|
|
}
|
|
found:
|
|
/* reserve queue for future packets in same flow */
|