Message-ID: <11915c70-ec5e-4d94-b890-f07f41094e2c@redhat.com>
Date: Thu, 9 Jan 2025 16:06:35 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Toke Høiland-Jørgensen <toke@...e.dk>,
 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

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

[...]
>> 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.

Thanks,

Paolo

Powered by blists - more mailing lists