netdev - Re: [PATCH net-next 3/6] net: sched: RED: Introduce an ECN tail-dropping mode

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]

Message-ID: <87tv2wy1zy.fsf@mellanox.com>
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;
>>  	}
>>