netdev - Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

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: <50F50537.3030608@broadcom.com>
Date:	Tue, 15 Jan 2013 09:28:55 +0200
From:	"Yuval Mintz" <yuvalmin@...adcom.com>
To:	"Eric Dumazet" <eric.dumazet@...il.com>
cc:	davem@...emloft.net, netdev@...r.kernel.org, eilong@...adcom.com,
	ariele@...adcom.com
Subject: Re: [PATCH net-next 09/10] bnx2x: Added FW GRO bridging support

On 01/14/2013 08:44 PM, Eric Dumazet wrote:
> On Mon, 2013-01-14 at 09:22 -0800, Eric Dumazet wrote:
> 
> What is the value of gso_segs ?
> 
> The reason I am pointing this out is the recent change in commit
> 1def9238d4aa2146924994aa4b7dc861f03b9362
> (net_sched: more precise pkt_len computation)
> 
> bnx2x not setting gso_segs means that qdisc accounting on ingress is
> completely wrong.

Hi Eric,

First I just want to state that you're totally correct about the gso_segs -
bnx2x is not setting it correctly (it's currently totally omitted), and so
it would incorrectly affect the accounting.

However, notice this behaviour was not introduced in this patch -
Since 621b4d6 bnx2x is using FW GRO, overriding the kernel's GRO implementation:
As the bnx2x driver is supplied with the aggregated packet from its FW,
the bnx2x passes a value in the `gso_size' field of its skb, causing
`skb_is_gso' to return `true'.
This will cause the aggregated skb to override the GRO machinations
(in `dev_gro_receive'), overriding all calls to `gro_receive' and thus also
the call to `skb_gro_receive' which whould have incremented `gso_segs'.

This patch actually tries to correct said behaviour, obviously failing
with the gso_segs but still improving the current state of bnx2x GRO
in bridging scenarios.

>> This looks weird to me. This should be called by GRO stack only.

I think this is the main question - we could try and implement this
inside the network-core itself, but as said behaviour is unique to the
bnx2x driver (correct me if I'm wrong, but I'm unaware of any other
driver which does GRO without the kernel GRO implementation), the
solution is specially tailored for the bnx2x driver.

We could either:
  1. Continue with this patch, later sending a patch correcting gso_segs,
     as this is not a new issue.
  2. Send a V2 patch-series which will also set gso_segs correctly.
  3. Send a V2 patch-series which omits this patch, and later send an RFC
     for some kernel implementation which fixes the issue.

Your thoughts on this matter will be greatly appreciated.

Thanks,
Yuval

--
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