Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 20/11/2024 12:45, Sabrina Dubroca wrote:
2024-10-29, 11:47:21 +0100, Antonio Quartulli wrote:
+static int ovpn_udp4_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
+ struct dst_cache *cache, struct sock *sk,
+ struct sk_buff *skb)
+{
[...]
+ if (unlikely(!inet_confirm_addr(sock_net(sk), NULL, 0, fl.saddr,
+ RT_SCOPE_HOST))) {
+ /* we may end up here when the cached address is not usable
+* anymore. In this case we reset address/cache and perform a
+* new look up
+*/
+ fl.saddr = 0;
+ bind->local.ipv4.s_addr = 0;
Here we're updating bind->local without holding peer->lock, that's
inconsistent with ovpn_peer_update_local_endpoint.
ACK
+static int ovpn_udp6_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
+ struct dst_cache *cache, struct sock *sk,
+ struct sk_buff *skb)
+{
[...]
+ if (unlikely(!ipv6_chk_addr(sock_net(sk), &fl.saddr, NULL, 0))) {
+ /* we may end up here when the cached address is not usable
+* anymore. In this case we reset address/cache and perform a
+* new look up
+*/
+ fl.saddr = in6addr_any;
+ bind->local.ipv6 = in6addr_any;
And here as well.
ACK
Will fix both.
Thank you.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 21/11/2024 01:29, Sergey Ryazanov wrote:
On 15.11.2024 16:39, Antonio Quartulli wrote:
On 11/11/2024 00:54, Sergey Ryazanov wrote:
Another one forgotten question, sorry about this. Please find the
question inlined.
On 29.10.2024 12:47, Antonio Quartulli wrote:
/* 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;
+ }
The above check implies that kernel can feed a network device with
skb- >protocol value mismatches actual skb content. Can you share
any example of such case?
If you just want to be sure that the user packet is either IPv4 or
IPv6 then it can be done like this and without error messages:
/* Support only IPv4 or IPv6 traffic transporting */
if (unlikely(skb->protocol == ETH_P_IP || skb->protocol == ETH_P_IPV6))
goto drop;
It look good, but I will still increase the drop counter, because
something entered the interface and we are trashing it.
Sure. I just shared a minimalistic example and don't mind if the case
will be counted. Just a small hint, the counter can be moved to the
'drop:' label below.
ok, will double check. thanks
And sorry for misguiding, the '->protocol' field value has network
endians, so constants should be wrapped in htons():
if (unlikely(skb->protocol == htons(ETH_P_IP) ||
skb->protocol == htons(ETH_P_IPV6)))
yap yap, already considered. thanks for pointing it out though.
goto drop;
Why not printing a message? The interface is not Ethernet based, so I
think we should not expect anything else other than v4 or v6, no?
Non-Ethernet encapsulation doesn't give any guaranty that packets will
be IPv4/IPv6 only. There are 65k possible 'protocols' and this is an
interface function, which technically can be called with any protocol type.
With this given, nobody wants to flood the log with messages for every
MPLS/LLDP/etc packet. Especially with messages saying that the packet is
malformed and giving no clue, why the packet was considered wrong.
Ok, I see. I am dropping the message then.
Regards,
--
Sergey
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 15.11.2024 16:39, Antonio Quartulli wrote:
On 11/11/2024 00:54, Sergey Ryazanov wrote:
Another one forgotten question, sorry about this. Please find the
question inlined.
On 29.10.2024 12:47, Antonio Quartulli wrote:
/* 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;
+ }
The above check implies that kernel can feed a network device with
skb- >protocol value mismatches actual skb content. Can you share any
example of such case?
If you just want to be sure that the user packet is either IPv4 or
IPv6 then it can be done like this and without error messages:
/* Support only IPv4 or IPv6 traffic transporting */
if (unlikely(skb->protocol == ETH_P_IP || skb->protocol == ETH_P_IPV6))
goto drop;
It look good, but I will still increase the drop counter, because
something entered the interface and we are trashing it.
Sure. I just shared a minimalistic example and don't mind if the case
will be counted. Just a small hint, the counter can be moved to the
'drop:' label below.
And sorry for misguiding, the '->protocol' field value has network
endians, so constants should be wrapped in htons():
if (unlikely(skb->protocol == htons(ETH_P_IP) ||
skb->protocol == htons(ETH_P_IPV6)))
goto drop;
Why not printing a message? The interface is not Ethernet based, so I
think we should not expect anything else other than v4 or v6, no?
Non-Ethernet encapsulation doesn't give any guaranty that packets will
be IPv4/IPv6 only. There are 65k possible 'protocols' and this is an
interface function, which technically can be called with any protocol type.
With this given, nobody wants to flood the log with messages for every
MPLS/LLDP/etc packet. Especially with messages saying that the packet is
malformed and giving no clue, why the packet was considered wrong.
--
Sergey
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
2024-10-29, 11:47:21 +0100, Antonio Quartulli wrote:
> +static int ovpn_udp4_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
> + struct dst_cache *cache, struct sock *sk,
> + struct sk_buff *skb)
> +{
[...]
> + if (unlikely(!inet_confirm_addr(sock_net(sk), NULL, 0, fl.saddr,
> + RT_SCOPE_HOST))) {
> + /* we may end up here when the cached address is not usable
> + * anymore. In this case we reset address/cache and perform a
> + * new look up
> + */
> + fl.saddr = 0;
> + bind->local.ipv4.s_addr = 0;
Here we're updating bind->local without holding peer->lock, that's
inconsistent with ovpn_peer_update_local_endpoint.
> +static int ovpn_udp6_output(struct ovpn_struct *ovpn, struct ovpn_bind *bind,
> + struct dst_cache *cache, struct sock *sk,
> + struct sk_buff *skb)
> +{
[...]
> + if (unlikely(!ipv6_chk_addr(sock_net(sk), &fl.saddr, NULL, 0))) {
> + /* we may end up here when the cached address is not usable
> + * anymore. In this case we reset address/cache and perform a
> + * new look up
> + */
> + fl.saddr = in6addr_any;
> + bind->local.ipv6 = in6addr_any;
And here as well.
--
Sabrina
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 11/11/2024 00:54, Sergey Ryazanov wrote:
Another one forgotten question, sorry about this. Please find the
question inlined.
On 29.10.2024 12:47, Antonio Quartulli wrote:
/* 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;
+ }
The above check implies that kernel can feed a network device with skb-
>protocol value mismatches actual skb content. Can you share any
example of such case?
If you just want to be sure that the user packet is either IPv4 or IPv6
then it can be done like this and without error messages:
/* Support only IPv4 or IPv6 traffic transporting */
if (unlikely(skb->protocol == ETH_P_IP || skb->protocol == ETH_P_IPV6))
goto drop;
It look good, but I will still increase the drop counter, because
something entered the interface and we are trashing it.
Why not printing a message? The interface is not Ethernet based, so I
think we should not expect anything else other than v4 or v6, no?
Thanks.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
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__
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
2024-11-11, 00:32:51 +0200, Sergey Ryazanov wrote:
> On 29.10.2024 12:47, Antonio Quartulli wrote:
> > +static void ovpn_encrypt_post(struct sk_buff *skb, int ret)
> > +{
> > + struct ovpn_peer *peer = ovpn_skb_cb(skb)->peer;
> > +
> > + if (unlikely(ret < 0))
> > + goto err;
> > +
> > + skb_mark_not_on_list(skb);
> > +
> > + switch (peer->sock->sock->sk->sk_protocol) {
> > + case IPPROTO_UDP:
> > + ovpn_udp_send_skb(peer->ovpn, peer, skb);
> > + break;
> > + default:
> > + /* no transport configured yet */
> > + goto err;
> > + }
>
> Did you consider calling protocol specific sending function indirectly?
> E.g.:
>
> peer->sock->send(peer, skb);
In a case where
- only 2 implementations exist
- no other implementation is likely to be added in the future
- both implementations are part of the same module
I don't think indirect calls are beneficial (especially after the
meltdown/etc mitigations, see for example 4f24ed77dec9 ("udp: use
indirect call wrappers for GRO socket lookup"), 0e219ae48c3b ("net:
use indirect calls helpers for L3 handler hooks"), and many others
similar patches).
[...]
> > + 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;
> > }
> > diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> > index
> > d9788a0cc99b5839c466c35d1b2266cc6b95fb72..aff3e9e99b7d2dd2fa68484d9a396d43f75a6d0b
> > 100644
> > --- a/drivers/net/ovpn/peer.c
> > +++ b/drivers/net/ovpn/peer.c
[very long chunk of Antonio's patch quoted without comments]
Please trim your replies to only the necessary context.
--
Sabrina
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
Another one forgotten question, sorry about this. Please find the
question inlined.
On 29.10.2024 12:47, Antonio Quartulli wrote:
/* 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;
+ }
The above check implies that kernel can feed a network device with
skb->protocol value mismatches actual skb content. Can you share any
example of such case?
If you just want to be sure that the user packet is either IPv4 or IPv6
then it can be done like this and without error messages:
/* Support only IPv4 or IPv6 traffic transporting */
if (unlikely(skb->protocol == ETH_P_IP || skb->protocol == ETH_P_IPV6))
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;
+
+ 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;
}
--
Sergey
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 29.10.2024 12:47, Antonio Quartulli wrote:
Packets sent over the ovpn interface are processed and transmitted to the
connected peer, if any.
Implementation is UDP only. TCP will be added by a later patch.
Note: no crypto/encapsulation exists yet. packets are just captured and
sent.
Signed-off-by: Antonio Quartulli
---
drivers/net/ovpn/io.c | 138 +++-
drivers/net/ovpn/peer.c | 37 +++-
drivers/net/ovpn/peer.h | 4 +
drivers/net/ovpn/skb.h | 51 +++
drivers/net/ovpn/udp.c | 232
drivers/net/ovpn/udp.h | 8 ++
6 files changed, 468 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index
ad3813419c33cbdfe7e8ad6f5c8b444a3540a69f..77ba4d33ae0bd2f52e8bd1c06a182d24285297b4
100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -9,14 +9,150 @@
#include
#include
+#include
#include "io.h"
+#include "ovpnstruct.h"
+#include "peer.h"
+#include "udp.h"
+#include "skb.h"
+#include "socket.h"
+
+static void ovpn_encrypt_post(struct sk_buff *skb, int ret)
+{
+ struct ovpn_peer *peer = ovpn_skb_cb(skb)->peer;
+
+ if (unlikely(ret < 0))
+ goto err;
+
+ skb_mark_not_on_list(skb);
+
+ switch (peer->sock->sock->sk->sk_protocol) {
+ case IPPROTO_UDP:
+ ovpn_udp_send_skb(peer->ovpn, peer, skb);
+ break;
+ default:
+ /* no transport configured yet */
+ goto err;
+ }
Did you consider calling protocol specific sending function indirectly?
E.g.:
peer->sock->send(peer, skb);
+ /* skb passed down the stack - don't free it */
+ skb = NULL;
+err:
+ if (unlikely(skb))
+ dev_core_stats_tx_dropped_inc(peer->ovpn->dev);
+ ovpn_peer_put(peer);
+ kfree_skb(skb);
+}
+
+static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb)
+{
+ ovpn_skb_cb(skb)->peer = peer;
+
+ /* take a reference to the peer because the crypto code may run async.
+* ovpn_encrypt_post() will release it upon completion
+*/
+ if (unlikely(!ovpn_peer_hold(peer))) {
+ DEBUG_NET_WARN_ON_ONCE(1);
+ return false;
+ }
+
+ ovpn_encrypt_post(skb, 0);
+ return true;
+}
+
+/* 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.
+
+ /* 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);
+}
/* 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
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
On 30/10/2024 18:14, Sabrina Dubroca wrote:
2024-10-29, 11:47:21 +0100, Antonio Quartulli wrote:
+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;
+ }
+
+ /* this might be a GSO-segmented skb list: process each skb
+* independently
+*/
+ skb_list_walk_safe(skb, curr, next)
nit (if you end up reposting): there should probably be some braces
around the (multi-line) loop body.
ACK
+ if (unlikely(!ovpn_encrypt_one(peer, curr))) {
+ dev_core_stats_tx_dropped_inc(ovpn->dev);
+ kfree_skb(curr);
+ }
+void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
+ struct sk_buff *skb)
+{
[...]
+ /* 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);
If I'm following things correctly, that's already been counted:
ovpn_udp_output -> ovpn_udp4_output -> udp_tunnel_xmit_skb
-> iptunnel_xmit
-> iptunnel_xmit_stats
which does (on success) the same thing as dev_sw_netstats_tx_add. On
Right. This means we can remove that call to tx_add().
failure it increments a different tx_dropped counter than what
dev_core_stats_tx_dropped_inc, but they should get summed in the end.
It seems they are summed up in dev_get_tstats64(), therefore I should
remove the tx_dropped_inc() call to avoid double counting.
Thanks!
Cheers,
+}
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 08/23] ovpn: implement basic TX path (UDP)
2024-10-29, 11:47:21 +0100, Antonio Quartulli wrote:
> +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;
> + }
> +
> + /* this might be a GSO-segmented skb list: process each skb
> + * independently
> + */
> + skb_list_walk_safe(skb, curr, next)
nit (if you end up reposting): there should probably be some braces
around the (multi-line) loop body.
> + if (unlikely(!ovpn_encrypt_one(peer, curr))) {
> + dev_core_stats_tx_dropped_inc(ovpn->dev);
> + kfree_skb(curr);
> + }
> +void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
> +struct sk_buff *skb)
> +{
[...]
> + /* 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);
If I'm following things correctly, that's already been counted:
ovpn_udp_output -> ovpn_udp4_output -> udp_tunnel_xmit_skb
-> iptunnel_xmit
-> iptunnel_xmit_stats
which does (on success) the same thing as dev_sw_netstats_tx_add. On
failure it increments a different tx_dropped counter than what
dev_core_stats_tx_dropped_inc, but they should get summed in the end.
> +}
--
Sabrina
