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.
