netdev - Re: [RFC PATCH 2/4] net: enable UDP gro on demand.

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]

Date:   Mon, 17 Sep 2018 12:18:20 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        steffen.klassert@...unet.com
Subject: Re: [RFC PATCH 2/4] net: enable UDP gro on demand.

On Sun, 2018-09-16 at 14:23 -0400, Willem de Bruijn wrote:
> That udp gro implementation is clearly less complete than yours in
> this patchset. The point I wanted to bring up for discussion is not the
> protocol implementation, but the infrastructure for enabling it
> conditionally.

I'm still [trying to] processing your patchset ;) So please perdon me
for any obvious interpretation mistakes...

> Assuming cycle cost is comparable, what do you think of  using the
> existing sk offload callbacks to enable this on a per-socket basis?

I have no objection about that, if there are no performance drawbacks.
In my measures retpoline costs is relevant for every indirect call
added. Using the existing sk offload approach will require an
additional indirect call per packet compared to the implementation
here.

> As for the protocol-wide knob, I do strongly prefer something that can
> work for all protocols, not just UDP. 

I like the general infrastructure idea. I think there is some agreement
in avoiding the addition of more user-controllable knobs, as we already
have a lot of them. If I read your patch correctly, user-space need to
enable/disable the UDO GSO explicitly via procfs, right?

I tried to look for something that does not require user action.

> I also implemented a version that
> atomically swaps the struct ptr instead of the flag based approach I sent
> for review. I'm fairly agnostic about that point. 

I think/fear security oriented guys may scream for the somewhat large
deconstification ?!?

> One subtle issue is that I
> believe we need to keep the gro_complete callbacks enabled, as gro
> packets may be queued for completion when gro_receive gets disabled.

Good point, thanks! I missed that.

Cheers,

Paolo