netdev - Re: [PATCH net-next 00/10] net: l3mdev: Support for sockets bound to enslaved device

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:   Fri, 4 Aug 2017 14:17:27 -0600
From:   David Ahern <dsahern@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 00/10] net: l3mdev: Support for sockets bound to
 enslaved device

On 8/1/17 6:41 PM, David Miller wrote:
> From: David Ahern <dsahern@...il.com>
> Date: Mon, 31 Jul 2017 20:13:16 -0700
> 
>> Existing code for socket lookups already pass in 6+ arguments. Rather
>> than add another for the enslaved device index, the existing lookups
>> are converted to use a new sk_lookup struct. From there, the enslaved
>> device index becomes another element of the struct.
> 
> Sorry, not gonna happen :-)
> 
> I know it's difficult, but maybe we should think about why we're
> passing so much crap into each lookup.\

The 'crap' is essential data to find a socket -- the hash table (common
lookup functions for multiple backends), destination address, dest port,
source address, source port and device index make 6 parameters to be
matched. The socket data is what I consolidated into 1 struct.
Ultimately that data has to make its way from high level wrappers to
compute_score and INET{6}_MATCH at the very bottom.

> 
> And perhaps, why it can't (for example) be constituted in the lookup
> function itself given sufficient (relevant) context.

There are several contexts for lookups -- ingress packets (ipv4 and v6,
tcp, udp, updlite, dccp), error paths for those protocols, netfilter and
socket diagnostic API. The 5 socket parameters do not have a consistent
storage across those contexts.

Further there are several layers of wrappers to what are the real lookup
functions, each wrapper abstracting some piece of data. The call
heirarchy for inet lookups for example:

tcp_v4_early_demux
    __inet_lookup_established

tcp_v4_rcv / dccp_v4_rcv
    __inet_lookup_skb
        __inet_lookup
            __inet_lookup_listener
            __inet_lookup_established

tcp_v4_rcv - TCP_TW_SYN
    inet_lookup_listener
        __inet_lookup_listener

nf_tproxy_get_sock_v4
    inet_lookup_listener
        __inet_lookup_listener

    inet_lookup_established
        __inet_lookup_established

inet_diag_find_one_icsk / nf_socket_get_sock_v4
    inet_lookup
        __inet_lookup
            __inet_lookup_listener
            __inet_lookup_established

tcp_v4_send_reset - MD5 path
    __inet_lookup_listener

tcp_v4_err / dccp_v4_err
    __inet_lookup_established

> 
> I think passing a big struct into the lookups, by reference, is a big
> step backwards.
> 
> For one thing, if you pass it by pointer then the compiler can't
> potentially pass parts in registers even if it could.  However
> if you pass it by value, that's actually a possibility.
> 
> But I'd like to avoid this on-stack blob altogether if possible.

Just sent a v2 that just adds sdif to the existing functions.