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,