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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Mar 2020 10:48:17 +0100 From: Petr Machata <petrm@...lanox.com> To: Jakub Kicinski <kuba@...nel.org> Cc: Ido Schimmel <idosch@...sch.org>, netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com, jhs@...atatu.com, xiyou.wangcong@...il.com, mlxsw@...lanox.com, Ido Schimmel <idosch@...lanox.com> Subject: Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode Jakub Kicinski <kuba@...nel.org> writes: > On Mon, 9 Mar 2020 20:35:00 +0200 Ido Schimmel wrote: >> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c >> index f9839d68b811..d72db7643a37 100644 >> --- a/net/sched/sch_red.c >> +++ b/net/sched/sch_red.c >> @@ -44,7 +44,8 @@ struct red_sched_data { >> struct Qdisc *qdisc; >> }; >> >> -#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | TC_RED_ADAPTATIVE) >> +#define RED_SUPPORTED_FLAGS (TC_RED_ECN | TC_RED_HARDDROP | \ >> + TC_RED_ADAPTATIVE | TC_RED_TAILDROP) >> >> static inline int red_use_ecn(struct red_sched_data *q) >> { >> @@ -56,6 +57,11 @@ static inline int red_use_harddrop(struct red_sched_data *q) >> return q->flags & TC_RED_HARDDROP; >> } >> >> +static inline int red_use_taildrop(struct red_sched_data *q) > > Please don't do static inlines in C code, even if the neighboring code > does. Ok. >> +{ >> + return q->flags & TC_RED_TAILDROP; >> +} >> + >> static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch, >> struct sk_buff **to_free) >> { >> @@ -76,23 +82,36 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch, >> >> case RED_PROB_MARK: >> qdisc_qstats_overlimit(sch); >> - if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) { >> + if (!red_use_ecn(q)) { >> q->stats.prob_drop++; >> goto congestion_drop; >> } >> >> - q->stats.prob_mark++; >> + if (INET_ECN_set_ce(skb)) { >> + q->stats.prob_mark++; >> + } else if (red_use_taildrop(q)) { > > This condition is inverted, no? > > If user requested taildrop the packet should be queued. Argh, yes! I was meddling with different ways of coding the if-else after Ido reviewed it and screwed it up. Thanks for catching it. My tests have not caught this, because they all hit the hard mark case :( >> + q->stats.prob_drop++; >> + goto congestion_drop; >> + } >> + >> + /* Non-ECT packet in ECN taildrop mode: queue it. */ >> break; >> >> case RED_HARD_MARK: >> qdisc_qstats_overlimit(sch); >> - if (red_use_harddrop(q) || !red_use_ecn(q) || >> - !INET_ECN_set_ce(skb)) { >> + if (red_use_harddrop(q) || !red_use_ecn(q)) { >> + q->stats.forced_drop++; >> + goto congestion_drop; >> + } >> + >> + if (INET_ECN_set_ce(skb)) { >> + q->stats.forced_mark++; >> + } else if (!red_use_taildrop(q)) { > > This one looks correct. > >> q->stats.forced_drop++; >> goto congestion_drop; >> } >> >> - q->stats.forced_mark++; >> + /* Non-ECT packet in ECN taildrop mode: queue it. */ >> break; >> } >>
Powered by blists - more mailing lists