From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.toke.dk (mail.toke.dk [45.145.95.4]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id E106A3B2A4 for ; Thu, 9 Jan 2025 07:47:19 -0500 (EST) From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=20161023; t=1736426838; bh=SQuxm+ItjKfTHqDCnhmWPP3mexlv7P5RG77HW1/uIAQ=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=YQNBVHHRUFbGfQ7S1lj/AaI1UHht3Yop0leBB6u0I4shd8583P21MwnUnaK/RcQVn im1b8k+iLaZ4a6wI+S448xPTyPkgaP4Wq5hns8uqvpV6pVvhGasQhEQC7UczKJt0PA qdl7aPqmFc1Kk/yXQQqUBIx16LMd2KG0KII6zTkFSsFgiqjDY8t2jVh1N4FSGLjwuG 6VHJ+OPmnfpZSJEp9+tQce1fjl52TRhoOctIXaL54+PRngLCWaYuWEq1Jt7+KcNnf0 lMTX08vw8hH3DdFtY4OU4yUuw5vT0lVTQOPzkJCDKo5Q1iLMU/sc6MwVnnLIbVxxE4 sCT+HzLk+MFpQ== To: Paolo Abeni , Jamal Hadi Salim , Cong Wang , Jiri Pirko Cc: syzbot+f63600d288bfb7057424@syzkaller.appspotmail.com, "David S. Miller" , Eric Dumazet , Jakub Kicinski , Simon Horman , cake@lists.bufferbloat.net, netdev@vger.kernel.org In-Reply-To: References: <20250107120105.70685-1-toke@redhat.com> Date: Thu, 09 Jan 2025 13:47:17 +0100 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <87plkwi27e.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] [PATCH net v2] sched: sch_cake: add bounds checks to host bulk flow fairness counts X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Jan 2025 12:47:20 -0000 Paolo Abeni writes: > On 1/7/25 1:01 PM, Toke H=C3=B8iland-J=C3=B8rgensen wrote: >> 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. >>=20 >> 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). >>=20 >> v2: >> - Remove now-unused srchost and dsthost local variables in cake_dequeue() > > Small nit: the changelog should come after the '---' separator. No need > to repost just for this. Oh, I was under the impression that we wanted them preserved in the git log (and hence above the ---). Is that not the case (anymore?)? >> Fixes: 546ea84d07e3 ("sched: sch_cake: fix bulk flow accounting logic fo= r host fairness") >> Reported-by: syzbot+f63600d288bfb7057424@syzkaller.appspotmail.com >> Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen >> --- >> net/sched/sch_cake.c | 140 +++++++++++++++++++++++-------------------- >> 1 file changed, 75 insertions(+), 65 deletions(-) >>=20 >> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c >> index 8d8b2db4653c..2c2e2a67f3b2 100644 >> --- a/net/sched/sch_cake.c >> +++ b/net/sched/sch_cake.c >> @@ -627,6 +627,63 @@ static bool cake_ddst(int flow_mode) >> return (flow_mode & CAKE_FLOW_DUAL_DST) =3D=3D CAKE_FLOW_DUAL_DST; >> } >>=20=20 >> +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 =3D 1; >> + >> + if (cake_dsrc(flow_mode)) >> + host_load =3D max(host_load, >> + q->hosts[flow->srchost].srchost_bulk_flow_count); >> + >> + if (cake_ddst(flow_mode)) >> + host_load =3D 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; > > dithering is now applied on both enqueue and dequeue, while prior to > this patch it only happened on dequeue. Is that intentional? can't lead > to (small) flow_deficit increase? Yeah, that was deliberate. The flow quantum is only set on enqueue when the flow is first initialised as a sparse flow, not for every packet. The only user-visible effect I can see this having is that the maximum packet size that can be sent while a flow stays sparse will now vary with +/- one byte in some cases. I am pretty sure this won't have any consequence in practice, and I don't think it's worth complicating the code (with a 'dither' argument to cake_flow_get_quantum(), say) to preserve the old behaviour. I guess I should have mentioned in the commit message that this was deliberate. Since it seems you'll be editing that anyway (cf the above), how about adding a paragraph like: As part of this change, the flow quantum calculation is consolidated into a helper function, which means that the dithering applied to the host 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. -Toke