netdev - Re: [PATCH -next] net: core: set qdisc pkt len before tc

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: <20150512170322.GA3524@Alexeis-MBP.westell.com>
Date:	Tue, 12 May 2015 10:03:23 -0700
From:	Alexei Starovoitov <alexei.starovoitov@...il.com>
To:	Florian Westphal <fw@...len.de>
Cc:	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
	daniel@...earbox.net, jhs@...atatu.com
Subject: Re: [PATCH -next] net: core: set qdisc pkt len before tc_classify

On Tue, May 12, 2015 at 05:16:46PM +0200, Florian Westphal wrote:
> > 
> > A qdisc might have a stab (cf qdisc_calculate_pkt_len() )
> 
> Thanks for pointing this out Eric.

Indeed. Thanks Eric!

> I was under impression that stab was only useful for egress but in
> fact tc did support ingress stab too.

That was my impression as well. Though it was allowed to add
qdisc_size_table to ingress, it's useless. Nothing takes advantage
of recomputed qdisc_pkt_len. It can only mess with stats, which
seems to be already broken:
- egress qdiscs do qdisc_bstats_update() only at dequeue, whereas
  ingress double counts dropped packets
- qdisc_bstats_update() does:
  bstats->bytes += qdisc_pkt_len(skb);
  bstats->packets += skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
  but nothing on ingress does qdisc_pkt_len_init().
  So when we see gso packet on ingress the stats are very wrong.

I think we should fix ingress stats as a whole.
Option 1:
 do qdisc_pkt_len_init() + optional call into size_table
 to update stats only after classifers returned TC_ACT_OK.
 Cons: extra overhead per packet only to update ingress qdisc stats.
Option 2:
 use byte and packet counts from underlying netdev, when
 reporting stats via ingress qdisc.
 Pros: No arithmetic in the fast path.

I think option 2 is much preferred, since it's faster and
equally accurate.

Jamal, what's your take?

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html