[PATCH net 0/3] pull request: ovpn for net 2025-07-03

[PATCH net 0/3] pull request: ovpn for net 2025-07-03

[Antonio Quartulli]

Hi netdev-team,

In this batch you can find the following bug fixes:

Patch 1: make sure to propagate any socket FW mark set by userspace
(via SO_MARK) to skbs being sent over that socket.

Patch 2: reject netlink PEER_NEW/SET messages if the read-only attribute
OVPN_A_PEER_LOCAL_PORT was specified.

Patch 3: reset the skb's GSO state when moving a packet from transport
to tunnel layer.

Please pull or let me know of any issue!

Thanks a lot.
Antonio,

The following changes since commit 72fb83735c71e3f6f025ab7f5dbfec7c9e26b6cc:

  Merge tag 'for-net-2025-06-27' of git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth (2025-06-30 18:58:47 -0700)

are available in the Git repository at:

  https://github.com/OpenVPN/ovpn-net-next tags/ovpn-net-20250703

for you to fetch changes up to 3d75827928ed01b548eb148f3292092bb07c6bcc:

  ovpn: reset GSO metadata after decapsulation (2025-07-03 13:22:15 +0200)

----------------------------------------------------------------
This bugfix batch includes the following changes:
* properly propagate sk mark to skb->mark field
* reject read-only OVPN_A_PEER_LOCAL_PORT in PEER_NEW/SET
* reset GSO state when moving skb from transport to tunnel layer

----------------------------------------------------------------
Antonio Quartulli (1):
      ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

Ralf Lici (2):
      ovpn: propagate socket mark to skb in UDP
      ovpn: reset GSO metadata after decapsulation

 drivers/net/ovpn/io.c      |  7 +++++++
 drivers/net/ovpn/netlink.c | 11 +++++++++++
 drivers/net/ovpn/udp.c     |  1 +
 3 files changed, 19 insertions(+)

[PATCH net 1/3] ovpn: propagate socket mark to skb in UDP

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

OpenVPN allows users to configure a FW mark on sockets used to
communicate with other peers. The mark is set by means of the
`SO_MARK` Linux socket option.

However, in the ovpn UDP code path, the socket's `sk_mark` value is
currently ignored and it is not propagated to outgoing `skbs`.

This commit ensures proper inheritance of the field by setting
`skb->mark` to `sk->sk_mark` before handing the `skb` to the network
stack for transmission.

Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Link: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31877.html
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/udp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index bff00946eae2..60435a21f29c 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -344,6 +344,7 @@ void ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
 	int ret;
 
 	skb->dev = peer->ovpn->dev;
+	skb->mark = READ_ONCE(sk->sk_mark);
 	/* no checksum performed at this layer */
 	skb->ip_summed = CHECKSUM_NONE;
 
-- 
2.49.0

[PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Antonio Quartulli]

The OVPN_A_PEER_LOCAL_PORT is designed to be a read-only attribute
that ovpn sends back to userspace to show the local port being used
to talk to that specific peer.

However, we forgot to reject it when parsing CMD_PEER_NEW/SET messages.
This is not a critical issue because the incoming value is just
ignored, but it may fool userspace which expects some change in
behaviour.

Explicitly error out and send back a message if OVPN_A_PEER_LOCAL_PORT
is specified in a CMD_PEER_NEW/SET message.

Reported-by: Ralf Lici <ralf@mandelbit.com>
Closes: https://github.com/OpenVPN/ovpn-net-next/issues/19
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/netlink.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index a4ec53def46e..17d23d01c6d8 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -224,6 +224,17 @@ static int ovpn_nl_peer_precheck(struct ovpn_priv *ovpn,
 		return -EINVAL;
 	}
 
+	/* the local port cannot be set by userspace because the socket
+	 * being passed is already bound to one.
+	 * OVPN_A_PEER_LOCAL_PORT is for sending peer status only (check
+	 * ovpn_nl_send_peer())
+	 */
+	if (attrs[OVPN_A_PEER_LOCAL_PORT]) {
+		NL_SET_ERR_MSG_MOD(info->extack,
+				   "cannot specify local port (socket is bound already)");
+		return -EINVAL;
+	}
+
 	/* check that local and remote address families are the same even
 	 * after parsing v4mapped IPv6 addresses.
 	 * (if addresses are not provided, family will be AF_UNSPEC and
-- 
2.49.0

[PATCH net 3/3] ovpn: reset GSO metadata after decapsulation

[Antonio Quartulli]

From: Ralf Lici <ralf@mandelbit.com>

The ovpn_netdev_write() function is responsible for injecting
decapsulated and decrypted packets back into the local network stack.

Prior to this patch, the skb could retain GSO metadata from the outer,
encrypted tunnel packet. This original GSO metadata, relevant to the
sender's transport context, becomes invalid and misleading for the
tunnel/data path once the inner packet is exposed.

Leaving this stale metadata intact causes internal GSO validation checks
further down the kernel's network stack (validate_xmit_skb()) to fail,
leading to packet drops. The reasons for these failures vary by
protocol, for example:
- for ICMP, no offload handler is registered;
- for TCP and UDP, the respective offload handlers return errors when
  comparing skb->len to the outdated skb_shinfo(skb)->gso_size.

By calling skb_gso_reset(skb) we ensure the inner packet is presented to
gro_cells_receive() with a clean state, correctly indicating it is an
individual packet from the perspective of the local stack.

This change eliminates the "Driver has suspect GRO implementation, TCP
performance may be compromised" warning and improves overall TCP
performance by allowing GSO/GRO to function as intended on the
decapsulated traffic.

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Reported-by: Gert Doering <gert@greenie.muc.de>
Closes: https://github.com/OpenVPN/ovpn-net-next/issues/4
Tested-by: Gert Doering <gert@greenie.muc.de>
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index ebf1e849506b..3e9e7f8444b3 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -62,6 +62,13 @@ static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
 	unsigned int pkt_len;
 	int ret;
 
+	/*
+	 * GSO state from the transport layer is not valid for the tunnel/data
+	 * path. Reset all GSO fields to prevent any further GSO processing
+	 * from entering an inconsistent state.
+	 */
+	skb_gso_reset(skb);
+
 	/* we can't guarantee the packet wasn't corrupted before entering the
 	 * VPN, therefore we give other layers a chance to check that
 	 */
-- 
2.49.0

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Sabrina Dubroca]

2025-07-03, 13:45:11 +0200, Antonio Quartulli wrote:
> The OVPN_A_PEER_LOCAL_PORT is designed to be a read-only attribute
> that ovpn sends back to userspace to show the local port being used
> to talk to that specific peer.

Seems like we'd want NLA_REJECT in the nla_policy instead of
NLA_POLICY_MIN, but my quick grepping in ynl and specs doesn't show
anything like that. Donald/Jakub, does it already exist? If not, does
it seem possible to extend the specs and ynl with something like:

name: local-port
type: reject(u16)

or maybe:

name: local-port
type: u16
checks:
  reject: true

(or maybe "read-only", and I don't know if yaml tolerates
"checks:\n reject" without the ": true". we can bikeshed the syntax
later :))


> However, we forgot to reject it when parsing CMD_PEER_NEW/SET messages.

Right :( Also a bunch of others: OVPN_A_PEER_SOCKET_NETNSID, all the
packets/bytes stats.

> This is not a critical issue because the incoming value is just
> ignored, but it may fool userspace which expects some change in
> behaviour.
> 
> Explicitly error out and send back a message if OVPN_A_PEER_LOCAL_PORT
> is specified in a CMD_PEER_NEW/SET message.
> 
> Reported-by: Ralf Lici <ralf@mandelbit.com>
> Closes: https://github.com/OpenVPN/ovpn-net-next/issues/19
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

Either way I'm ok with this patch.

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCH net 1/3] ovpn: propagate socket mark to skb in UDP

[Sabrina Dubroca]

2025-07-03, 13:45:10 +0200, Antonio Quartulli wrote:
> From: Ralf Lici <ralf@mandelbit.com>
> 
> OpenVPN allows users to configure a FW mark on sockets used to
> communicate with other peers. The mark is set by means of the
> `SO_MARK` Linux socket option.
> 
> However, in the ovpn UDP code path, the socket's `sk_mark` value is
> currently ignored and it is not propagated to outgoing `skbs`.

Probably worth adding a small selftest? (not necessarily as part of
this series, though I think it would be ideal to have the fix and the
corresponding test together)

> This commit ensures proper inheritance of the field by setting
> `skb->mark` to `sk->sk_mark` before handing the `skb` to the network
> stack for transmission.
> 
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> Link: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31877.html
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>

A Fixes tag would be welcome for all those bugfixes. For this patch:

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")


With this:
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Jakub Kicinski]

On Thu, 3 Jul 2025 15:07:51 +0200 Sabrina Dubroca wrote:
> > The OVPN_A_PEER_LOCAL_PORT is designed to be a read-only attribute
> > that ovpn sends back to userspace to show the local port being used
> > to talk to that specific peer.  
> 
> Seems like we'd want NLA_REJECT in the nla_policy instead of
> NLA_POLICY_MIN, but my quick grepping in ynl and specs doesn't show
> anything like that. Donald/Jakub, does it already exist? If not, does
> it seem possible to extend the specs and ynl with something like:
> 
> name: local-port
> type: reject(u16)
> 
> or maybe:
> 
> name: local-port
> type: u16
> checks:
>   reject: true

There's no way to explicitly reject, because we expect that only what's
needed will be listed (IOW we depend on NLA_UNSPEC rather than
NLA_REJECT). It gets complicated at times but I think it should work
here. Key mechanism is to define subsets of the nests:

diff --git a/Documentation/netlink/specs/ovpn.yaml b/Documentation/netlink/specs/ovpn.yaml
index 17e5e9b7f5a5..15309201e7ca 100644
--- a/Documentation/netlink/specs/ovpn.yaml
+++ b/Documentation/netlink/specs/ovpn.yaml
@@ -160,6 +160,36 @@ doc: Netlink protocol to control OpenVPN network devices
         name: link-tx-packets
         type: uint
         doc: Number of packets transmitted at the transport level
+  -
+    name: peer-input
+    subset-of: peer
+    attributes:
+      -
+        name: id
+      -
+        name: remote-ipv4
+      -
+        name: remote-ipv6
+      -
+        name: remote-ipv6-scope-id
+      -
+        name: remote-port
+      -
+        name: socket
+      -
+        name: socket-netnsid
+      -
+        name: vpn-ipv4
+      -
+        name: vpn-ipv6
+      -
+        name: local-ipv4
+      -
+        name: local-ipv6
+      -
+        name: keepalive-interval
+      -
+        name: keepalive-timeout
   -
     name: keyconf
     attributes:
@@ -235,12 +265,21 @@ doc: Netlink protocol to control OpenVPN network devices
         type: nest
         doc: Peer specific cipher configuration
         nested-attributes: keyconf
+  -
+    name: ovpn-peer-input
+    subset-of: ovpn
+    attributes:
+      -
+        name: ifindex
+      -
+        name: peer
+        nested-attributes: peer-input
 
 operations:
   list:
     -
       name: peer-new
-      attribute-set: ovpn
+      attribute-set: ovpn-peer-input
       flags: [admin-perm]
       doc: Add a remote peer
       do:
@@ -252,7 +291,7 @@ doc: Netlink protocol to control OpenVPN network devices
             - peer
     -
       name: peer-set
-      attribute-set: ovpn
+      attribute-set: ovpn-peer-input
       flags: [admin-perm]
       doc: modify a remote peer
       do:

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Sabrina Dubroca]

2025-07-07, 14:48:28 -0700, Jakub Kicinski wrote:
> On Thu, 3 Jul 2025 15:07:51 +0200 Sabrina Dubroca wrote:
> > > The OVPN_A_PEER_LOCAL_PORT is designed to be a read-only attribute
> > > that ovpn sends back to userspace to show the local port being used
> > > to talk to that specific peer.  
> > 
> > Seems like we'd want NLA_REJECT in the nla_policy instead of
> > NLA_POLICY_MIN, but my quick grepping in ynl and specs doesn't show
> > anything like that. Donald/Jakub, does it already exist? If not, does
> > it seem possible to extend the specs and ynl with something like:
> > 
> > name: local-port
> > type: reject(u16)
> > 
> > or maybe:
> > 
> > name: local-port
> > type: u16
> > checks:
> >   reject: true
> 
> There's no way to explicitly reject, because we expect that only what's
> needed will be listed (IOW we depend on NLA_UNSPEC rather than
> NLA_REJECT). It gets complicated at times but I think it should work
> here. Key mechanism is to define subsets of the nests:

Ok, I see. It's a bit verbose, especially with the nest, but adding a
reject here and there as I was suggesting wouldn't work for per-op
policies.


In ovpn we should also reject attributes from GET and DEL that aren't
currently used to match the peer we want to get/delete (ie everything
except PEER_ID), while still being able to parse all possible peer
attributes from the kernel's reply (only for GET). So I guess we'd
want a different variant of the nested attribute "peer" for the
request and reply here:

    -
      name: peer-get
      attribute-set: ovpn
      flags: [admin-perm]
      doc: Retrieve data about existing remote peers (or a specific one)
      do:
        pre: ovpn-nl-pre-doit
        post: ovpn-nl-post-doit
        request:
          attributes:
            - ifindex
            - peer
        reply:
          attributes:
            - peer
      dump:
        request:
          attributes:
            - ifindex
        reply:
          attributes:
            - peer


-- 
Sabrina

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Jakub Kicinski]

On Tue, 8 Jul 2025 12:20:09 +0200 Sabrina Dubroca wrote:
> Ok, I see. It's a bit verbose, especially with the nest, but adding a
> reject here and there as I was suggesting wouldn't work for per-op
> policies.

Right, it's a tricky problem to solve :(
Really, the best time to address it is when family is designed.
Even folks quite familiar with netlink make the mistake of treating
nesting as a cute way of grouping related attributes.
It is really, really counter productive to use it like that, nesting
has major drawbacks.
ethtool nesting may seem "inverted", but it's a good example of nesting
used _correctly_.

> In ovpn we should also reject attributes from GET and DEL that aren't
> currently used to match the peer we want to get/delete (ie everything
> except PEER_ID), while still being able to parse all possible peer
> attributes from the kernel's reply (only for GET). So I guess we'd
> want a different variant of the nested attribute "peer" for the
> request and reply here:

Yes, that's hard to the point of probably not being worth fixing 
at the spec level? :( We could so something like:

--- a/Documentation/netlink/specs/ovpn.yaml
+++ b/Documentation/netlink/specs/ovpn.yaml
@@ -265,6 +265,11 @@ doc: Netlink protocol to control OpenVPN network devices
         type: nest
         doc: Peer specific cipher configuration
         nested-attributes: keyconf
+      -
+        name: peer-input
+        type: nest
+        nested-attributes: peer-input
+        value: 2
   -
     name: ovpn-peer-input
     subset-of: ovpn

but the codegen today will output this "fake" attribute into the uAPI
which we don't need.

In any case. I think what I suggested is slightly better than
opencoding, even if verbose :) So I set the patches to Changes
Requested..

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Antonio Quartulli]

Hi Jakub,

I was on vacation last week and I am processing your emails now..

On 08/07/2025 16:47, Jakub Kicinski wrote:
> In any case. I think what I suggested is slightly better than
> opencoding, even if verbose :) So I set the patches to Changes
> Requested..

So you'd want to go with what you suggested on July 7th?
I.e. using subset-of and defining 'peer-input'/'ovpn-peer-input'.
Did I get it right?

As Sabrina pointed out, I'll also define a subset for PEER_DEL/GET, 
where we only need the PEER_ID.


Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Jakub Kicinski]

On Sun, 13 Jul 2025 22:04:49 +0200 Antonio Quartulli wrote:
> On 08/07/2025 16:47, Jakub Kicinski wrote:
> > In any case. I think what I suggested is slightly better than
> > opencoding, even if verbose :) So I set the patches to Changes
> > Requested..  
> 
> So you'd want to go with what you suggested on July 7th?
> I.e. using subset-of and defining 'peer-input'/'ovpn-peer-input'.
> Did I get it right?

Yes, please. It will improve the generated userspace C code, too.

> As Sabrina pointed out, I'll also define a subset for PEER_DEL/GET, 
> where we only need the PEER_ID.

GET may be hard but you can try.

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Antonio Quartulli]

On 08/07/2025 12:20, Sabrina Dubroca wrote:
[...]
> In ovpn we should also reject attributes from GET and DEL that aren't
> currently used to match the peer we want to get/delete (ie everything
> except PEER_ID), while still being able to parse all possible peer
> attributes from the kernel's reply (only for GET). So I guess we'd
> want a different variant of the nested attribute "peer" for the
> request and reply here:
> 
>      -
>        name: peer-get
>        attribute-set: ovpn
>        flags: [admin-perm]
>        doc: Retrieve data about existing remote peers (or a specific one)
>        do:
>          pre: ovpn-nl-pre-doit
>          post: ovpn-nl-post-doit
>          request:
>            attributes:
>              - ifindex
>              - peer
>          reply:
>            attributes:
>              - peer
>        dump:
>          request:
>            attributes:
>              - ifindex
>          reply:
>            attributes:
>              - peer
> 
> 

As Jakub predicted, I am hitting a problem with PEER_GET: the 
attribute-set is one for the entire op, therefore I can't specify two 
different sets for request and reply.

I presume I need to leave PEER_GET on the main 'ovpn' set and then 
opencode the restriction of having only the ID in the request.

Similarly goes for KEY_GET.

Sabrina, Jakub, does it make sense to you?


Regards,


-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Jakub Kicinski]

On Tue, 15 Jul 2025 16:36:40 +0200 Antonio Quartulli wrote:
> As Jakub predicted, I am hitting a problem with PEER_GET: the 
> attribute-set is one for the entire op, therefore I can't specify two 
> different sets for request and reply.
> 
> I presume I need to leave PEER_GET on the main 'ovpn' set and then 
> opencode the restriction of having only the ID in the request.
> 
> Similarly goes for KEY_GET.
> 
> Sabrina, Jakub, does it make sense to you?

Yes :( Sorry for the mixed solution but I think using the spec to its
full capabilities is worthwhile, even if it doesn't cover all the needs.

Re: [PATCH net 2/3] ovpn: explicitly reject netlink attr PEER_LOCAL_PORT in CMD_PEER_NEW/SET

[Antonio Quartulli]

On 15/07/2025 17:06, Jakub Kicinski wrote:
> On Tue, 15 Jul 2025 16:36:40 +0200 Antonio Quartulli wrote:
>> As Jakub predicted, I am hitting a problem with PEER_GET: the
>> attribute-set is one for the entire op, therefore I can't specify two
>> different sets for request and reply.
>>
>> I presume I need to leave PEER_GET on the main 'ovpn' set and then
>> opencode the restriction of having only the ID in the request.
>>
>> Similarly goes for KEY_GET.
>>
>> Sabrina, Jakub, does it make sense to you?
> 
> Yes :( Sorry for the mixed solution but I think using the spec to its
> full capabilities is worthwhile, even if it doesn't cover all the needs.

I totally agree - at least we can limit the opencoding only to a few ops.

Thanks for confirming - I'll go with the mixed approach.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.