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: <55532F5A.5070007@mojatatu.com>
Date:	Wed, 13 May 2015 07:02:50 -0400
From:	Jamal Hadi Salim <jhs@...atatu.com>
To:	Alexei Starovoitov <alexei.starovoitov@...il.com>,
	Florian Westphal <fw@...len.de>
CC:	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
	daniel@...earbox.net
Subject: Re: [PATCH -next] net: core: set qdisc pkt len before tc_classify

On 05/12/15 13:03, Alexei Starovoitov wrote:
> 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?
>

I dont think we need the stab on the ingress but we do need to account
for gso. So option #1 with qdisc_pkt_len_init() alone is the only thing
needed. i.e Florian's change becomes:

-	qdisc_bstats_update_cpu(cl->q, skb);
+	qdisc_pkt_len_init(skb)
  	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
+	qdisc_bstats_update_cpu(cl->q, skb);

Alexei, why do you say this option will have overhead?

cheers,
jamal
--
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