Message-ID: <87ikqohswh.fsf@toke.dk>
Date: Thu, 09 Jan 2025 17:08:14 +0100
From: Toke Høiland-Jørgensen <toke@...e.dk>
To: Paolo Abeni <pabeni@...hat.com>, Jamal Hadi Salim <jhs@...atatu.com>,
 Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>
Cc: syzbot+f63600d288bfb7057424@...kaller.appspotmail.com, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Simon Horman <horms@...nel.org>,
 cake@...ts.bufferbloat.net, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] sched: sch_cake: add bounds checks to host bulk
 flow fairness counts

Paolo Abeni <pabeni@...hat.com> writes:

> On 1/9/25 1:47 PM, Toke Høiland-Jørgensen wrote:
>> Paolo Abeni <pabeni@...hat.com> writes:
>>> On 1/7/25 1:01 PM, Toke Høiland-Jørgensen 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.
>>>>
>>>> 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).
>>>>
>>>> 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?)?
>
> It was some time ago. Is this way since a while:
>
> https://elixir.bootlin.com/linux/v6.13-rc3/source/Documentation/process/maintainer-netdev.rst#L229

Huh, whaddyaknow. Thanks for the pointer.

> [...]
>>> 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.
>
> Understood, and fine by me.
>
>> 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.
>
> It's in Jakub's hands now, possibly he could prefer a repost to reduce
> the maintainer's overhead.

Alright, sure, I'll respin :)

-Toke

Powered by blists - more mailing lists