netdev - Re: [PATCH net-next v2 10/12] gtp: add IPv6 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]

Date:   Sat, 12 Dec 2020 08:05:40 +0100
From:   Jonas Bonn <jonas@...rbonn.se>
To:     Pravin Shelar <pravin.ovn@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Pablo Neira Ayuso <pablo@...filter.org>, laforge@...monks.org
Subject: Re: [PATCH net-next v2 10/12] gtp: add IPv6 support

Hi Pravin,

On 12/12/2020 06:51, Pravin Shelar wrote:
> On Fri, Dec 11, 2020 at 4:29 AM Jonas Bonn <jonas@...rbonn.se> wrote:
>>
>> This patch adds support for handling IPv6.  Both the GTP tunnel and the
>> tunneled packets may be IPv6; as they constitute independent streams,
>> both v4-over-v6 and v6-over-v4 are supported, as well.
>>
>> This patch includes only the driver functionality for IPv6 support.  A
>> follow-on patch will add support for configuring the tunnels with IPv6
>> addresses.
>>
>> Signed-off-by: Jonas Bonn <jonas@...rbonn.se>
>> ---
>>   drivers/net/gtp.c | 330 +++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 269 insertions(+), 61 deletions(-)
>>


>> +       /* Get IP version of _inner_ packet */
>> +       ipver = inner_ip_hdr(skb)->version;
>> +
>> +       switch (ipver) {
>> +       case 4:
>> +               skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IP));
>> +               r = gtp_check_ms_ipv4(skb, pctx, hdrlen, role);
> I don't see a need to set inner header on receive path, we are any
> ways removing outer header from this packet in same function.
> 
>> +               break;
>> +       case 6:
>> +               skb_set_inner_protocol(skb, cpu_to_be16(ETH_P_IPV6));
>> +               r = gtp_check_ms_ipv6(skb, pctx, hdrlen, role);
>> +               break;
>> +       }
>> +
>> +       if (!r) {
>>                  netdev_dbg(pctx->dev, "No PDP ctx for this MS\n");
>>                  return 1;
>>          }
>> @@ -193,6 +256,8 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
>>                                   !net_eq(sock_net(pctx->sk), dev_net(pctx->dev))))
>>                  return -1;
>>
>> +       skb->protocol = skb->inner_protocol;
>> +
> iptunnel_pull_header() can set the protocol, so it would be better to
> pass the correct inner protocol.
> 

Yes, your comments above are correct.  I'll fix that.



>>          netdev_dbg(pctx->dev, "forwarding packet from GGSN to uplink\n");
>>
>>          /* Now that the UDP and the GTP header have been removed, set up the
>> @@ -201,7 +266,7 @@ static int gtp_rx(struct pdp_ctx *pctx, struct sk_buff *skb,
>>           */
>>          skb_reset_network_header(skb);
>>
>> -       skb->dev = pctx->dev;
>> +       __skb_tunnel_rx(skb, pctx->dev, sock_net(pctx->sk));
>>
> No need to call skb_tunnel_rx() given iptunnel_pull_header() function
> is already called and it does take care of clearing the context.

Right.  The only difference seems to be that __skb_tunnel_rx also does:

skb->dev = dev;

iptunnel_pull_header excludes that.  I can't see that setting the 
skb->dev will actually change anything for this driver, but it was there 
previously.  Thoughts?

>>
>> +static struct dst_entry *gtp_get_v6_dst(struct sk_buff *skb,
>> +                                       struct net_device *dev,
>> +                                       struct pdp_ctx *pctx,
>> +                                       struct in6_addr *saddr)
>> +{
>> +       const struct sock *sk = pctx->sk;
>> +       struct dst_entry *dst = NULL;
>> +       struct flowi6 fl6;
>> +
>> +       memset(&fl6, 0, sizeof(fl6));
>> +       fl6.flowi6_mark = skb->mark;
>> +       fl6.flowi6_proto = IPPROTO_UDP;
>> +       fl6.daddr = pctx->peer_addr;
>> +
>> +       dst = ipv6_stub->ipv6_dst_lookup_flow(sock_net(sk), sk, &fl6, NULL);
>> +       if (IS_ERR(dst)) {
>> +               netdev_dbg(pctx->dev, "no route to %pI6\n", &fl6.daddr);
>> +               return ERR_PTR(-ENETUNREACH);
>> +       }
>> +       if (dst->dev == pctx->dev) {
>> +               netdev_dbg(pctx->dev, "circular route to %pI6\n", &fl6.daddr);
>> +               dst_release(dst);
>> +               return ERR_PTR(-ELOOP);
>> +       }
>> +
>> +       *saddr = fl6.saddr;
>> +
>> +       return dst;
>> +}
>> +
> IPv6 related functionality needs to be protected by IS_ENABLED(CONFIG_IPV6).

Yes, you're probably right.  Given that IPv6 isn't really optional in 
contexts where this driver is relevant, however, I'm almost inclined to 
switch this around and make the driver depend on the availability of IPv6...

/Jonas