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] [day] [month] [year] [list]

Date:   Mon, 17 Sep 2018 10:07:06 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.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 Mon, Sep 17, 2018 at 6:18 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.

Fair enough. The question is whether it is significant to the real workload.
This is also an issue with GRO processing in general, all those callbacks
as well as the two cacheline lookups to get to each callback.

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

No, like other GRO callbacks, the feature is enabled by default.

Patch 7/8 disables the most expensive part behind a static key
until a socket actually registers a GRO callback, whether tunnel
or the new application GRO.

Patch 6/9 makes it possible to disable any protocol completely,
indeed through a sysctl.

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

Hmm.. yes, interesting point. Since const pointers are a compile time
feature, in practice I don't think that they buy any protection against
callback pointer rewriting. Let me think about that some more.

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