netdev - Re: [Patch net-next] net_sched: remove the unsafe __skb_array

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: <7d126ce3-15c6-92ad-c9b1-da9a3149dd3e@gmail.com>
Date:   Thu, 21 Dec 2017 19:06:32 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     jakub.kicinski@...ronome.com
Subject: Re: [Patch net-next] net_sched: remove the unsafe __skb_array_empty()

On 12/21/2017 04:03 PM, Cong Wang wrote:
> __skb_array_empty() is only safe if array is never resized.
> pfifo_fast_dequeue() is called in TX BH context and without
> qdisc lock, so even after we disable BH on ->reset() path
> we can still race with other CPU's.
> 
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Reported-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Cc: John Fastabend <john.fastabend@...il.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>  net/sched/sch_generic.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 00ddb5f8f430..9279258ce060 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -622,9 +622,6 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
>  	for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
>  		struct skb_array *q = band2list(priv, band);
>  
> -		if (__skb_array_empty(q))
> -			continue;
> -
>  		skb = skb_array_consume_bh(q);
>  	}
>  	if (likely(skb)) {
> 

So this is a performance thing we don't want to grab the consumer lock on
empty bands. Which can be fairly common depending on traffic patterns.

Although its not logical IMO to have both reset and dequeue running at
the same time. Some skbs would get through others would get sent, sort
of a mess. I don't see how it can be an issue. The never resized bit
in the documentation is referring to resizing the ring size _not_ popping
off elements of the ring. array_empty just reads the consumer head.
The only ring resizing in pfifo fast should be at init and destroy where
enqueue/dequeue should be disconnected by then. Although based on the
trace I missed a case.

I think the right fix is to only call reset/destroy patterns after
waiting a grace period and for all tx_action calls in-flight to
complete. This is also better going forward for more complex qdiscs.

.John