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: <20200309151255.29bb3a4b@kicinski-fedora-PC1C0HJN>
Date:   Mon, 9 Mar 2020 15:12:55 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jiri@...lanox.com,
        petrm@...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

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.

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

> +			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;
>  	}
>