Re: [PATCH net v4 1/2] net/sched: Restrict conditions for adding duplicating netems to qdisc tree

[Paolo Abeni <pabeni@redhat.com>]

On 7/1/25 12:39 AM, Cong Wang wrote:
> On Mon, Jun 30, 2025 at 07:32:48AM -0400, Jamal Hadi Salim wrote:
>> On Sun, Jun 29, 2025 at 4:16 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Sat, Jun 28, 2025 at 05:25:25PM -0400, Jamal Hadi Salim wrote:
>>>> your approach was to overwrite the netem specific cb which is exposed
>>>> via the cb ->data that can be overwritten for example by a trivial
>>>> ebpf program attach to any level of the hierarchy. This specific
>>>> variant from Cong is not accessible to ebpf but as i expressed my view
>>>> in other email i feel it is not a good solution.
>>>>
>>>> https://lore.kernel.org/netdev/CAM0EoMk4dxOFoN_=3yOy+XrtU=yvjJXAw3fVTmN9=M=R=vtbxA@mail.gmail.com/
>>>
>>> Hi Jamal,
>>>
>>> I have two concerns regarding your/Will's proposal:
>>>
>>> 1) I am not sure whether disallowing such case is safe. From my
>>> understanding this case is not obviously or logically wrong. So if we
>>> disallow it, we may have a chance to break some application.
>>>
>>
>> I dont intentionaly creating a loop-inside-a-loop as being correct.
>> Stephen, is this a legit use case?
>> Agreed that we need to be careful about some corner cases which may
>> look crazy but are legit.
> 
> Maybe I misunderstand your patch, to me duplicating packets in
> parallel sub-hierarchy is not wrong, may be even useful.
> 
>>
>>> 2) Singling out this case looks not elegant to me.
>>
>> My thinking is to long term disallow all nonsense hierarchy use cases,
>> such as this one, with some
>> "feature bits". ATM, it's easy to catch the bad configs within a
>> single qdisc in ->init() but currently not possible if it affects a
>> hierarchy.
> 
> The problem with this is it becomes harder to get a clear big picture,
> today netem, tomorrow maybe hfsc etc.? We could end up with hiding such
> bad-config-prevention code in different Qdisc's.
> 
> With the approach I suggested, we have a central place (probably
> sch_api.c) to have all the logics, nothing is hidden, easier to
> understand and easier to introduce more bad-config-prevention code.

Since an agreement about an optimal long term solution looks not
trivial, and the proposed code is AFACS solving the issue at hand, WDYT
about accepting this one and incrementally improve it as needed?

/P