Skip to content

Commit

Permalink
sched: sch_cake: add bounds checks to host bulk flow fairness counts
Browse files Browse the repository at this point in the history 
Even though we fixed a logic error in the commit cited below, syzbot
still managed to trigger an underflow of the per-host bulk flow
counters, leading to an out of bounds memory access.

To avoid any such logic errors causing out of bounds memory accesses,
this commit factors out all accesses to the per-host bulk flow counters
to a series of helpers that perform bounds-checking before any
increments and decrements. This also has the benefit of improving
readability by moving the conditional checks for the flow mode into
these helpers, instead of having them spread out throughout the
code (which was the cause of the original logic error).

As part of this change, the flow quantum calculation is consolidated
into a helper function, which means that the dithering applied to the
ost load scaling is now applied both in the DRR rotation and when a
sparse flow's quantum is first initiated. The only user-visible effect
of this is that the maximum packet size that can be sent while a flow
stays sparse will now vary with +/- one byte in some cases. This should
not make a noticeable difference in practice, and thus it's not worth
complicating the code to preserve the old behaviour.

Fixes: 546ea84 ("sched: sch_cake: fix bulk flow accounting logic for host fairness")
Reported-by: [email protected]
Signed-off-by: Toke Høiland-Jørgensen <[email protected]>
Acked-by: Dave Taht <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
  • Loading branch information
@tohojo @kuba-moo
tohojo authored and kuba-moo committed Jan 9, 2025
1 parent 2d2d4f6 commit 737d4d9
Showing 1 changed file with 75 additions and 65 deletions.
140 changes: 75 additions & 65 deletions net/sched/sch_cake.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,63 @@ static bool cake_ddst(int flow_mode)
return (flow_mode & CAKE_FLOW_DUAL_DST) == CAKE_FLOW_DUAL_DST;
}

static void cake_dec_srchost_bulk_flow_count(struct cake_tin_data *q,
struct cake_flow *flow,
int flow_mode)
{
if (likely(cake_dsrc(flow_mode) &&
q->hosts[flow->srchost].srchost_bulk_flow_count))
q->hosts[flow->srchost].srchost_bulk_flow_count--;
}

static void cake_inc_srchost_bulk_flow_count(struct cake_tin_data *q,
struct cake_flow *flow,
int flow_mode)
{
if (likely(cake_dsrc(flow_mode) &&
q->hosts[flow->srchost].srchost_bulk_flow_count < CAKE_QUEUES))
q->hosts[flow->srchost].srchost_bulk_flow_count++;
}

static void cake_dec_dsthost_bulk_flow_count(struct cake_tin_data *q,
struct cake_flow *flow,
int flow_mode)
{
if (likely(cake_ddst(flow_mode) &&
q->hosts[flow->dsthost].dsthost_bulk_flow_count))
q->hosts[flow->dsthost].dsthost_bulk_flow_count--;
}

static void cake_inc_dsthost_bulk_flow_count(struct cake_tin_data *q,
struct cake_flow *flow,
int flow_mode)
{
if (likely(cake_ddst(flow_mode) &&
q->hosts[flow->dsthost].dsthost_bulk_flow_count < CAKE_QUEUES))
q->hosts[flow->dsthost].dsthost_bulk_flow_count++;
}

static u16 cake_get_flow_quantum(struct cake_tin_data *q,
struct cake_flow *flow,
int flow_mode)
{
u16 host_load = 1;

if (cake_dsrc(flow_mode))
host_load = max(host_load,
q->hosts[flow->srchost].srchost_bulk_flow_count);

if (cake_ddst(flow_mode))
host_load = max(host_load,
q->hosts[flow->dsthost].dsthost_bulk_flow_count);

/* The get_random_u16() is a way to apply dithering to avoid
* accumulating roundoff errors
*/
return (q->flow_quantum * quantum_div[host_load] +
get_random_u16()) >> 16;
}

static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
int flow_mode, u16 flow_override, u16 host_override)
{
Expand Down Expand Up @@ -773,10 +830,8 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
allocate_dst = cake_ddst(flow_mode);

if (q->flows[outer_hash + k].set == CAKE_SET_BULK) {
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--;
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);
}
found:
/* reserve queue for future packets in same flow */
Expand All @@ -801,9 +856,10 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
q->hosts[outer_hash + k].srchost_tag = srchost_hash;
found_src:
srchost_idx = outer_hash + k;
if (q->flows[reduced_hash].set == CAKE_SET_BULK)
q->hosts[srchost_idx].srchost_bulk_flow_count++;
q->flows[reduced_hash].srchost = srchost_idx;

if (q->flows[reduced_hash].set == CAKE_SET_BULK)
cake_inc_srchost_bulk_flow_count(q, &q->flows[reduced_hash], flow_mode);
}

if (allocate_dst) {
Expand All @@ -824,9 +880,10 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
q->hosts[outer_hash + k].dsthost_tag = dsthost_hash;
found_dst:
dsthost_idx = outer_hash + k;
if (q->flows[reduced_hash].set == CAKE_SET_BULK)
q->hosts[dsthost_idx].dsthost_bulk_flow_count++;
q->flows[reduced_hash].dsthost = dsthost_idx;

if (q->flows[reduced_hash].set == CAKE_SET_BULK)
cake_inc_dsthost_bulk_flow_count(q, &q->flows[reduced_hash], flow_mode);
}
}

Expand Down Expand Up @@ -1839,10 +1896,6 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,

/* flowchain */
if (!flow->set || flow->set == CAKE_SET_DECAYING) {
struct cake_host *srchost = &b->hosts[flow->srchost];
struct cake_host *dsthost = &b->hosts[flow->dsthost];
u16 host_load = 1;

if (!flow->set) {
list_add_tail(&flow->flowchain, &b->new_flows);
} else {
Expand All @@ -1852,31 +1905,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
flow->set = CAKE_SET_SPARSE;
b->sparse_flow_count++;

if (cake_dsrc(q->flow_mode))
host_load = max(host_load, srchost->srchost_bulk_flow_count);

if (cake_ddst(q->flow_mode))
host_load = max(host_load, dsthost->dsthost_bulk_flow_count);

flow->deficit = (b->flow_quantum *
quantum_div[host_load]) >> 16;
flow->deficit = cake_get_flow_quantum(b, flow, q->flow_mode);
} else if (flow->set == CAKE_SET_SPARSE_WAIT) {
struct cake_host *srchost = &b->hosts[flow->srchost];
struct cake_host *dsthost = &b->hosts[flow->dsthost];

/* this flow was empty, accounted as a sparse flow, but actually
* in the bulk rotation.
*/
flow->set = CAKE_SET_BULK;
b->sparse_flow_count--;
b->bulk_flow_count++;

if (cake_dsrc(q->flow_mode))
srchost->srchost_bulk_flow_count++;

if (cake_ddst(q->flow_mode))
dsthost->dsthost_bulk_flow_count++;

cake_inc_srchost_bulk_flow_count(b, flow, q->flow_mode);
cake_inc_dsthost_bulk_flow_count(b, flow, q->flow_mode);
}

if (q->buffer_used > q->buffer_max_used)
Expand Down Expand Up @@ -1933,13 +1972,11 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
{
struct cake_sched_data *q = qdisc_priv(sch);
struct cake_tin_data *b = &q->tins[q->cur_tin];
struct cake_host *srchost, *dsthost;
ktime_t now = ktime_get();
struct cake_flow *flow;
struct list_head *head;
bool first_flow = true;
struct sk_buff *skb;
u16 host_load;
u64 delay;
u32 len;

Expand Down Expand Up @@ -2039,11 +2076,6 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
q->cur_flow = flow - b->flows;
first_flow = false;

/* triple isolation (modified DRR++) */
srchost = &b->hosts[flow->srchost];
dsthost = &b->hosts[flow->dsthost];
host_load = 1;

/* flow isolation (DRR++) */
if (flow->deficit <= 0) {
/* Keep all flows with deficits out of the sparse and decaying
Expand All @@ -2055,11 +2087,8 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
b->sparse_flow_count--;
b->bulk_flow_count++;

if (cake_dsrc(q->flow_mode))
srchost->srchost_bulk_flow_count++;

if (cake_ddst(q->flow_mode))
dsthost->dsthost_bulk_flow_count++;
cake_inc_srchost_bulk_flow_count(b, flow, q->flow_mode);
cake_inc_dsthost_bulk_flow_count(b, flow, q->flow_mode);

flow->set = CAKE_SET_BULK;
} else {
Expand All @@ -2071,19 +2100,7 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
}
}

if (cake_dsrc(q->flow_mode))
host_load = max(host_load, srchost->srchost_bulk_flow_count);

if (cake_ddst(q->flow_mode))
host_load = max(host_load, dsthost->dsthost_bulk_flow_count);

WARN_ON(host_load > CAKE_QUEUES);

/* The get_random_u16() is a way to apply dithering to avoid
* accumulating roundoff errors
*/
flow->deficit += (b->flow_quantum * quantum_div[host_load] +
get_random_u16()) >> 16;
flow->deficit += cake_get_flow_quantum(b, flow, q->flow_mode);
list_move_tail(&flow->flowchain, &b->old_flows);

goto retry;
Expand All @@ -2107,11 +2124,8 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
if (flow->set == CAKE_SET_BULK) {
b->bulk_flow_count--;

if (cake_dsrc(q->flow_mode))
srchost->srchost_bulk_flow_count--;

if (cake_ddst(q->flow_mode))
dsthost->dsthost_bulk_flow_count--;
cake_dec_srchost_bulk_flow_count(b, flow, q->flow_mode);
cake_dec_dsthost_bulk_flow_count(b, flow, q->flow_mode);

b->decaying_flow_count++;
} else if (flow->set == CAKE_SET_SPARSE ||
Expand All @@ -2129,12 +2143,8 @@ static struct sk_buff *cake_dequeue(struct Qdisc *sch)
else if (flow->set == CAKE_SET_BULK) {
b->bulk_flow_count--;

if (cake_dsrc(q->flow_mode))
srchost->srchost_bulk_flow_count--;

if (cake_ddst(q->flow_mode))
dsthost->dsthost_bulk_flow_count--;

cake_dec_srchost_bulk_flow_count(b, flow, q->flow_mode);
cake_dec_dsthost_bulk_flow_count(b, flow, q->flow_mode);
} else
b->decaying_flow_count--;

Expand Down

0 comments on commit 737d4d9

Please sign in to comment.