Message-ID: <7b7f8808-9e4a-4302-9266-9a051c2ff27b@openvpn.net>
Date: Thu, 14 Nov 2024 16:25:56 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)

On 10/11/2024 23:32, Sergey Ryazanov wrote:
[...]
>> +/* send skb to connected peer, if any */
>> +static void ovpn_send(struct ovpn_struct *ovpn, struct sk_buff *skb,
>> +              struct ovpn_peer *peer)
>> +{
>> +    struct sk_buff *curr, *next;
>> +
>> +    if (likely(!peer))
>> +        /* retrieve peer serving the destination IP of this packet */
>> +        peer = ovpn_peer_get_by_dst(ovpn, skb);
>> +    if (unlikely(!peer)) {
>> +        net_dbg_ratelimited("%s: no peer to send data to\n",
>> +                    ovpn->dev->name);
>> +        dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +        goto drop;
>> +    }
> 
> The function is called only from ovpn_xmit_special() and from 
> ovpn_net_xmit(). The keepalive always provides a peer object, while 
> ovpn_net_xmit() never do it. If we move the peer lookup call into 
> ovpn_net_xmit() then we can eliminate all the above peer checks.

yeah, I think that's a good idea! See below..

> 
>> +
>> +    /* this might be a GSO-segmented skb list: process each skb
>> +     * independently
>> +     */
>> +    skb_list_walk_safe(skb, curr, next)
>> +        if (unlikely(!ovpn_encrypt_one(peer, curr))) {
>> +            dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +            kfree_skb(curr);
>> +        }
>> +
>> +    /* skb passed over, no need to free */
>> +    skb = NULL;
>> +drop:
>> +    if (likely(peer))
>> +        ovpn_peer_put(peer);
>> +    kfree_skb_list(skb);
>> +}

..because this error path disappears as well.

And I can move the stats increment to ovpn_net_xmit() in order to avoid 
counting keepalive packets as vpn data.

>>   /* Send user data to the network
>>    */
>>   netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>> +    struct ovpn_struct *ovpn = netdev_priv(dev);
>> +    struct sk_buff *segments, *curr, *next;
>> +    struct sk_buff_head skb_list;
>> +    __be16 proto;
>> +    int ret;
>> +
>> +    /* reset netfilter state */
>> +    nf_reset_ct(skb);
>> +
>> +    /* verify IP header size in network packet */
>> +    proto = ovpn_ip_check_protocol(skb);
>> +    if (unlikely(!proto || skb->protocol != proto)) {
>> +        net_err_ratelimited("%s: dropping malformed payload packet\n",
>> +                    dev->name);
>> +        dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +        goto drop;
>> +    }
>> +
>> +    if (skb_is_gso(skb)) {
>> +        segments = skb_gso_segment(skb, 0);
>> +        if (IS_ERR(segments)) {
>> +            ret = PTR_ERR(segments);
>> +            net_err_ratelimited("%s: cannot segment packet: %d\n",
>> +                        dev->name, ret);
>> +            dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +            goto drop;
>> +        }
>> +
>> +        consume_skb(skb);
>> +        skb = segments;
>> +    }
>> +
>> +    /* from this moment on, "skb" might be a list */
>> +
>> +    __skb_queue_head_init(&skb_list);
>> +    skb_list_walk_safe(skb, curr, next) {
>> +        skb_mark_not_on_list(curr);
>> +
>> +        curr = skb_share_check(curr, GFP_ATOMIC);
>> +        if (unlikely(!curr)) {
>> +            net_err_ratelimited("%s: skb_share_check failed\n",
>> +                        dev->name);
>> +            dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +            continue;
>> +        }
>> +
>> +        __skb_queue_tail(&skb_list, curr);
>> +    }
>> +    skb_list.prev->next = NULL;
>> +
> 
> I belive, the peer lookup should be done here to call ovpn_send() with 
> proper peer object and simplify it.

ACK

> 
>> +    ovpn_send(ovpn, skb_list.next, NULL);
>> +
>> +    return NETDEV_TX_OK;
>> +
>> +drop:
>>       skb_tx_error(skb);
>> -    kfree_skb(skb);
>> +    kfree_skb_list(skb);
>>       return NET_XMIT_DROP;
>>   }

[...]

>> +/**
>> + * ovpn_udp_send_skb - prepare skb and send it over via UDP
>> + * @ovpn: the openvpn instance
>> + * @peer: the destination peer
>> + * @skb: the packet to send
>> + */
>> +void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>> +               struct sk_buff *skb)
>> +{
>> +    struct ovpn_bind *bind;
>> +    unsigned int pkt_len;
>> +    struct socket *sock;
>> +    int ret = -1;
>> +
>> +    skb->dev = ovpn->dev;
>> +    /* no checksum performed at this layer */
>> +    skb->ip_summed = CHECKSUM_NONE;
>> +
>> +    /* get socket info */
>> +    sock = peer->sock->sock;
>> +    if (unlikely(!sock)) {
>> +        net_warn_ratelimited("%s: no sock for remote peer\n", __func__);
> 
> If we do not have netdev_{err,warn,etc}_ratelimited() helper functions, 
> can we at least emulate it like this:
> 
> net_warn_ratelimited("%s: no UDP sock for remote peer #%u\n",
>                       netdev_name(ovpn->dev), peer->id);

that's what I try to do, but some prints have escaped my axe.
Will fix that, thanks!

> 
> or just use netdev_warn_once(...) since the condition looks more 
> speculative than expected.
> 
> Peer id and interface name are more informative than just a function name.

Yeah, I use the function name in some debug messages, although not 
extremely useful.

Will make sure the iface name is always printed (there are similar 
occurrences like this)

> 
>> +        goto out;
>> +    }
>> +
>> +    rcu_read_lock();
>> +    /* get binding */
>> +    bind = rcu_dereference(peer->bind);
>> +    if (unlikely(!bind)) {
>> +        net_warn_ratelimited("%s: no bind for remote peer\n", __func__);
> 
> Ditto
> 
>> +        goto out_unlock;
>> +    }
>> +
>> +    /* crypto layer -> transport (UDP) */
>> +    pkt_len = skb->len;
>> +    ret = ovpn_udp_output(ovpn, bind, &peer->dst_cache, sock->sk, skb);
>> +
>> +out_unlock:
>> +    rcu_read_unlock();
>> +out:
>> +    if (unlikely(ret < 0)) {
>> +        dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +        kfree_skb(skb);
>> +        return;
>> +    }
>> +
>> +    dev_sw_netstats_tx_add(ovpn->dev, 1, pkt_len);
>> +}
>> +
>>   /**
>>    * ovpn_udp_socket_attach - set udp-tunnel CBs on socket and link it 
>> to ovpn
>>    * @sock: socket to configure
>> diff --git a/drivers/net/ovpn/udp.h b/drivers/net/ovpn/udp.h
>> index 
>> f2507f8f2c71ea9d5e5ac5446801e2d56f86700f..e60f8cd2b4ac8f910aabcf8ed546af59d6ca4be4 100644
>> --- a/drivers/net/ovpn/udp.h
>> +++ b/drivers/net/ovpn/udp.h
>> @@ -9,9 +9,17 @@
>>   #ifndef _NET_OVPN_UDP_H_
>>   #define _NET_OVPN_UDP_H_
>> +#include <linux/skbuff.h>
>> +#include <net/sock.h>
>> +
>> +struct ovpn_peer;
>>   struct ovpn_struct;
>> +struct sk_buff;
> 
> This declaration looks odd since we already have skbuff.h included above.

I believe originally there was no include, then I need to add that.
Will double check,

Thanks a lot!
Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists