Re: [Cake] [PATCH net v2] sched: sch_cake: add bounds checks to host bulk flow fairness counts

[Paolo Abeni <pabeni@redhat.com>]

On 1/9/25 1:47 PM, Toke Høiland-Jørgensen wrote:
> Paolo Abeni <pabeni@redhat.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