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 16:53:19 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Michel Machado <michel@...irati.com.br>
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 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;
...

> 
>    2. sch_prio.c does not have a global limit on the number of packets on
> all its queues, only a limit per queue.

It can be useful to sch_prio.c as well, why not?
prio_enqueue()
{
...
+	if (count > sch->global_limit)
+		prio_tail_drop(sch);   /* to be implemented */
        ret = qdisc_enqueue(skb, qdisc, to_free);

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

> 
>    Given the divergences, adding flags to sch_prio.c will essentially keep
> both implementations together instead of being isolated as being proposed.

I don't agree. There aren't that many flags. I see only 2, one which
makes sense to sch_prio as it is already (the global limit) and from
where it should drop, the overflown packet or from tail.

All other code will be reused: stats handling, netlink handling,
enqueue and dequeue at least.

If we add this other qdisc, named as it is, it will be very confusing
to sysadmins: both are named very closely and work essentially in the
same way, but one drops from tail and another drops the incoming
packet.

> 
>    On the speed point, there may not be noticeable difference between both
> qdiscs because the enqueueing and dequeueing costs of both qdics are O(1).
> Notice that the "extra work" (i.e. dropping lower priority packets) is a key
> aspect of skbprio since it gives routers a cheap way to choose which packets
> to drop during a DoS.

On that I agree. I was more referring to something like: "lets not make
sch_prio slow and implement a new one instead.", which I don't it's
valid because the extra "cost" is only visible when it's already
dropping packets. Hopefully it's clearer now :)

[]s
Marcelo