netdev - Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jul 2018 17:03:31 -0400
From:   Michel Machado <michel@...irati.com.br>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     Nishanth Devarajan <ndev2021@...il.com>, xiyou.wangcong@...il.com,
        jhs@...atatu.com, jiri@...nulli.us, davem@...emloft.net,
        netdev@...r.kernel.org, doucette@...edu
Subject: Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

On 07/09/2018 03:53 PM, Marcelo Ricardo Leitner wrote:
> On Mon, Jul 09, 2018 at 02:18:33PM -0400, Michel Machado wrote:
>> On 07/09/2018 11:44 AM, Marcelo Ricardo Leitner wrote:
>>> On Sat, Jul 07, 2018 at 03:43:55PM +0530, Nishanth Devarajan wrote:
>>>> net/sched: add skbprio scheduer
>>>>
>>>> Skbprio (SKB Priority Queue) is a queueing discipline that prioritizes packets
>>>> according to their skb->priority field. Under congestion, already-enqueued lower
>>>> priority packets will be dropped to make space available for higher priority
>>>> packets. Skbprio was conceived as a solution for denial-of-service defenses that
>>>> need to route packets with different priorities as a means to overcome DoS
>>>> attacks.
>>>
>>> Why can't we implement this as a new flag for sch_prio.c?
>>>
>>> I don't see why this duplication is needed, especially because it will
>>> only be "slower" (as in, it will do more work) when qdisc is already
>>> full and dropping packets anyway.
>>
>>     sch_prio.c and skbprio diverge on a number of aspects:
>>
>>     1. sch_prio.c supports up to 16 priorities whereas skbprio 64. This is
>> not just a matter of changing a constant since sch_prio.c doesn't use
>> skb->priority.
> 
> Yes it does use skb->priority for classifying into a band:
> 
> prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
> {
>          struct prio_sched_data *q = qdisc_priv(sch);
>          u32 band = skb->priority;
> ...

    Changing TC_PRIO_MAX from 15 to 63 risks breaking backward 
compatibility with applications.

>>     3. The queues of sch_prio.c are struct Qdisc, which don't have a method
>> to drop at its tail.
> 
> That can be implemented, most likely as prio_tail_drop() as above.

    struct Qdisc represents *all* qdiscs. My knowledge of the other 
qdiscs is limited, but not all qdiscs may have a meaningful method to 
drop at the tail. For example: a qdisc that works over flows may not 
know with flow is the tail. Not to mention that this would be a 
widespread patch to only support this new prio qdisc. It would be 
prudent to wait for the production success of the proposed, 
self-contained qdisc before making this commitment.

[ ]'s
Michel Machado