netdev - Re: [PATCH net-next v2] GTP: add support for flow based tunneling API

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, 14 Dec 2020 09:29:33 +0100
From:   Jonas Bonn <jonas@...rbonn.se>
To:     Pravin Shelar <pravin.ovn@...il.com>
Cc:     Pravin B Shelar <pbshelar@...com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Pablo Neira Ayuso <pablo@...filter.org>, laforge@...monks.org
Subject: Re: [PATCH net-next v2] GTP: add support for flow based tunneling API

Hi Pravin,

On 13/12/2020 20:32, Pravin Shelar wrote:
> On Sat, Dec 12, 2020 at 11:56 PM Jonas Bonn <jonas@...rbonn.se> wrote:
>>
>> Hi Pravin,
>>
>> I've been thinking a bit about this and find it more and more
>> interesting.  Could you post a bit of information about the ip-route
>> changes you'll make in order to support GTP LWT encapsulation?  Could
>> you provide an example command line?
>>
> This is done as part of the magma core project
> (https://www.magmacore.org/) that needs OVS GTP support.
> I have started with OVS integration first, there are unit tests that
> validate the GTP support. This is datapath related test, that has the
> setup commands:
> https://github.com/pshelar/ovs/blob/6ec6a2a86adc56c7c9dcab7b3a7b70bb6dad35c9/tests/system-layer3-tunnels.at#L158

That link just shows the classic setup using gtp-link and gtp-tunnel 
from libgtpnl.  It doesn't exercise LWT at all.

> Once OVS patches are upstream I can post patches for ip-route command.

No, you should do it the other way around, please.  Post the ip-route 
changes along with this so we can see where this is going.

>>> +#include <net/dst_metadata.h>
>>>    #include <net/net_namespace.h>
>>>    #include <net/protocol.h>
>>>    #include <net/ip.h>
>>> @@ -73,6 +74,9 @@ struct gtp_dev {
>>>        unsigned int            hash_size;
>>>        struct hlist_head       *tid_hash;
>>>        struct hlist_head       *addr_hash;
>>> +     /* Used by flow based tunnel. */
>>> +     bool                    collect_md;
>>> +     struct socket           *collect_md_sock;
>>
>> I'm not convinced that you need to special-case LWT in this way.  It
>> should be possible to just use the regular sk1u socket.  I know that the
>> sk1u socket is created in userspace and might be set up to listen on the
>> wrong address, but that's a user error if they try to use that device
>> with LWT.  You could easily make the sk1u socket an optional parameter
>> and create it (as you do in your patch) if it's not provided.  Then
>> ip_tunnel_collect_metadata() would tell you whether to get the
>> encapsulaton details from the tunnel itself or whether to look up a PDP
>> context.  That should suffice, right?
>>
> Sounds good. I have added it as part of v3.
> Just to be clear, I still need collect_md_sock to keep reference to
> the socket that is created as part of the newlink in kernel space.

Why?  I don't see that there's anything special enough about that socket 
that you can't just use it as sk1u.  You might need to massage the types 
a bit, but that doesn't seem like a big problem.  What am I missing?

/Jonas

> 
> Thanks,
> Pravin.
>