[PATCH net-next 00/10] pull request for net-next: ovpn 2025-05-09

[PATCH net-next 00/10] pull request for net-next: ovpn 2025-05-09

[Antonio Quartulli]

Hi Jakub,

here is my first pull request for the ovpn kernel module!

As you can see in the patches, we have various tags from
Gert Döring, the main maintainer of openvpn userspace,
who was eager to test and report malfunctionings.
He reported bugs, stared at fixes and tested them, hence
the Reported/Acked/Tested-by tags. If you think such
combination of tags is not truly appropriate, please let
me know.

Please pull or let me know if something should be changed.

Thanks a lot,
Antonio


The following changes since commit 3e52667a9c328b3d1a1ddbbb6b8fbf63a217bda3:

  Merge branch 'lan78xx-phylink-prep' (2025-05-07 12:57:06 +0100)

are available in the Git repository at:

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

for you to fetch changes up to 476148af4e28f498bf769eeed81ef99728544c56:

  ovpn: ensure sk is still valid during cleanup (2025-05-09 15:37:31 +0200)

----------------------------------------------------------------
This patchset contains the following:
- update MAINTAINERS entry for ovpn
- extend selftest with more cases
- fix ndo_start_xmit return value on error
- set ignore_df flag for IPv6 packets
- drop useless reg_state check in keepalive worker
- avoid crash during concurrent peer timeout and iface deletion

----------------------------------------------------------------
Antonio Quartulli (10):
      MAINTAINERS: add Sabrina as official reviewer for ovpn
      MAINTAINERS: update git URL for ovpn
      ovpn: set skb->ignore_df = 1 before sending IPv6 packets out
      ovpn: don't drop skb's dst when xmitting packet
      selftest/net/ovpn: fix crash in case of getaddrinfo() failure
      ovpn: fix ndo_start_xmit return value on error
      selftest/net/ovpn: extend coverage with more test cases
      ovpn: drop useless reg_state check in keepalive worker
      ovpn: improve 'no route to host' debug message
      ovpn: ensure sk is still valid during cleanup

 MAINTAINERS                                    |  3 ++-
 drivers/net/ovpn/io.c                          | 18 +++++++++++++++---
 drivers/net/ovpn/main.c                        |  5 +++++
 drivers/net/ovpn/peer.c                        |  5 ++---
 drivers/net/ovpn/socket.c                      | 21 ++++++++++++---------
 drivers/net/ovpn/udp.c                         | 10 ++++++++++
 tools/testing/selftests/net/ovpn/Makefile      |  1 +
 tools/testing/selftests/net/ovpn/common.sh     | 18 +++++++++++++++++-
 tools/testing/selftests/net/ovpn/ovpn-cli.c    | 19 +++++++++++++------
 tools/testing/selftests/net/ovpn/test.sh       |  6 +++++-
 tools/testing/selftests/net/ovpn/udp_peers.txt | 11 ++++++-----
 11 files changed, 88 insertions(+), 29 deletions(-)

[PATCH net-next 01/10] MAINTAINERS: add Sabrina as official reviewer for ovpn

[Antonio Quartulli]

Sabrina put quite some effort in reviewing the ovpn module
during its official submission to netdev.
For this reason she obtain extensive knowledge of the module
architecture and implementation.

Make her an official reviewer, so that I can be supported
in reviewing and acking new patches.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
Acked-by: Sabrina Dubroca <sd@queasysnail.net>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5c31814c9687..f107479f1af1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18196,6 +18196,7 @@ F:	drivers/irqchip/irq-or1k-*
 
 OPENVPN DATA CHANNEL OFFLOAD
 M:	Antonio Quartulli <antonio@openvpn.net>
+R:	Sabrina Dubroca <sd@queasysnail.net>
 L:	openvpn-devel@lists.sourceforge.net (subscribers-only)
 L:	netdev@vger.kernel.org
 S:	Supported
-- 
2.49.0

Re: [PATCH net-next 01/10] MAINTAINERS: add Sabrina as official reviewer for ovpn

[Andrew Lunn]

On Fri, May 09, 2025 at 04:26:11PM +0200, Antonio Quartulli wrote:
> Sabrina put quite some effort in reviewing the ovpn module
> during its official submission to netdev.
> For this reason she obtain extensive knowledge of the module
> architecture and implementation.
> 
> Make her an official reviewer, so that I can be supported
> in reviewing and acking new patches.
> 
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> Acked-by: Sabrina Dubroca <sd@queasysnail.net>

Agreed. She deserves the credit.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 01/10] MAINTAINERS: add Sabrina as official reviewer for ovpn

[Antonio Quartulli]

On 09/05/2025 16:34, Andrew Lunn wrote:
> On Fri, May 09, 2025 at 04:26:11PM +0200, Antonio Quartulli wrote:
>> Sabrina put quite some effort in reviewing the ovpn module
>> during its official submission to netdev.
>> For this reason she obtain extensive knowledge of the module
>> architecture and implementation.
>>
>> Make her an official reviewer, so that I can be supported
>> in reviewing and acking new patches.
>>
>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>> Acked-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> Agreed. She deserves the credit.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Hi Jakub,

should I edit the patch in my tree and append the Reviewed-by Andrew?

While at it, should I add the "Link:" tag referencing patches on the 
openvpn mailing list?

I can keep the same git-tag name so that the pull request does not need 
to be resent.

Cheers,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net-next 01/10] MAINTAINERS: add Sabrina as official reviewer for ovpn

[Jakub Kicinski]

On Mon, 12 May 2025 10:22:24 +0200 Antonio Quartulli wrote:
> should I edit the patch in my tree and append the Reviewed-by Andrew?

Not needed. Not being able to add review tags is just one of those
things we have to accept.

> While at it, should I add the "Link:" tag referencing patches on the 
> openvpn mailing list?

For the future patches this would be helpful.

[PATCH net-next 02/10] MAINTAINERS: update git URL for ovpn

[Antonio Quartulli]

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index f107479f1af1..04c58e3ee47d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18200,7 +18200,7 @@ R:	Sabrina Dubroca <sd@queasysnail.net>
 L:	openvpn-devel@lists.sourceforge.net (subscribers-only)
 L:	netdev@vger.kernel.org
 S:	Supported
-T:	git https://github.com/OpenVPN/linux-kernel-ovpn.git
+T:	git https://github.com/OpenVPN/ovpn-net-next.git
 F:	Documentation/netlink/specs/ovpn.yaml
 F:	drivers/net/ovpn/
 F:	include/uapi/linux/ovpn.h
-- 
2.49.0

[PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Antonio Quartulli]

IPv6 user packets (sent over the tunnel) may be larger than
the outgoing interface MTU after encapsulation.
When this happens ovpn should allow the kernel to fragment
them because they are "locally generated".

To achieve the above, we must set skb->ignore_df = 1
so that ip6_fragment() can be made aware of this decision.

Failing to do so will result in ip6_fragment() dropping
the packet thinking it was "routed".

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Reported-by: Gert Doering <gert@greenie.muc.de>
Tested-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Gert Doering <gert@greenie.muc.de> # as primary user
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/udp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index c9e189056f33..aef8c0406ec9 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -262,6 +262,16 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 	dst_cache_set_ip6(cache, dst, &fl.saddr);
 
 transmit:
+	/* user IPv6 packets may be larger than the transport interface
+	 * MTU (after encapsulation), however, since they are locally
+	 * generated we should ensure they get fragmented.
+	 * Setting the ignore_df flag to 1 will instruct ip6_fragment() to
+	 * fragment packets if needed.
+	 *
+	 * NOTE: this is not needed for IPv4 because we pass df=0 to
+	 * udp_tunnel_xmit_skb()
+	 */
+	skb->ignore_df = 1;
 	udp_tunnel6_xmit_skb(dst, sk, skb, skb->dev, &fl.saddr, &fl.daddr, 0,
 			     ip6_dst_hoplimit(dst), 0, fl.fl6_sport,
 			     fl.fl6_dport, udp_get_no_check6_tx(sk));
-- 
2.49.0

Re: [PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Paolo Abeni]

On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> IPv6 user packets (sent over the tunnel) may be larger than
> the outgoing interface MTU after encapsulation.
> When this happens ovpn should allow the kernel to fragment
> them because they are "locally generated".
> 
> To achieve the above, we must set skb->ignore_df = 1
> so that ip6_fragment() can be made aware of this decision.

Why the above applies only to IPv6? AFAICS the same could happen even
for IPv4.

/P

Re: [PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Gert Doering]

Hi,

On Tue, May 13, 2025 at 09:37:33AM +0200, Paolo Abeni wrote:
> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> > IPv6 user packets (sent over the tunnel) may be larger than
> > the outgoing interface MTU after encapsulation.
> > When this happens ovpn should allow the kernel to fragment
> > them because they are "locally generated".
> > 
> > To achieve the above, we must set skb->ignore_df = 1
> > so that ip6_fragment() can be made aware of this decision.
> 
> Why the above applies only to IPv6? AFAICS the same could happen even
> for IPv4.

Without having a detailed understanding of the kernel code paths (I'm
the userspace developer that breaks this stuff from the outside), the
v4/v6 logic seems different enough - possibly based on differences in
fragment handling in the relevant RFCs - that this is not a problem.

I am testing "3000 byte IPv4 and IPv6 packets over OpenVPN over IPv4 and
IPv6", intentionally creating both inside and outside fragmentation,
and without this patch "... over IPv6" was broken, while "... over IPv4"
worked all the time.  With the patch, all 4 combinations pass.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@greenie.muc.de

Re: [PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Antonio Quartulli]

On 13/05/2025 09:37, Paolo Abeni wrote:
> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
>> IPv6 user packets (sent over the tunnel) may be larger than
>> the outgoing interface MTU after encapsulation.
>> When this happens ovpn should allow the kernel to fragment
>> them because they are "locally generated".
>>
>> To achieve the above, we must set skb->ignore_df = 1
>> so that ip6_fragment() can be made aware of this decision.
> 
> Why the above applies only to IPv6? AFAICS the same could happen even
> for IPv4.

For IPv4 we have the 'df=0' param that is passed to 
udp_tunnel_xmit_skb(), which basically leads to the same result.

Originally (in some old version of the original ovpn submission) I had 
skb->ignore_df=1 in the common path, but then it was removed when 
Sabrina highlighted the df param.

However, we overlooked that there is no such param/logic for IPv6, hence 
we need the explicit ignore_df=1 for IPv6.

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Paolo Abeni]

On 5/13/25 9:51 AM, Antonio Quartulli wrote:
> On 13/05/2025 09:37, Paolo Abeni wrote:
>> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
>>> IPv6 user packets (sent over the tunnel) may be larger than
>>> the outgoing interface MTU after encapsulation.
>>> When this happens ovpn should allow the kernel to fragment
>>> them because they are "locally generated".
>>>
>>> To achieve the above, we must set skb->ignore_df = 1
>>> so that ip6_fragment() can be made aware of this decision.
>>
>> Why the above applies only to IPv6? AFAICS the same could happen even
>> for IPv4.
> 
> For IPv4 we have the 'df=0' param that is passed to 
> udp_tunnel_xmit_skb(), which basically leads to the same result.

You need to include (an expanded/more describing version of) the above
in the commit message.

/P

Re: [PATCH net-next 03/10] ovpn: set skb->ignore_df = 1 before sending IPv6 packets out

[Antonio Quartulli]

On 13/05/2025 10:51, Paolo Abeni wrote:
> On 5/13/25 9:51 AM, Antonio Quartulli wrote:
>> On 13/05/2025 09:37, Paolo Abeni wrote:
>>> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
>>>> IPv6 user packets (sent over the tunnel) may be larger than
>>>> the outgoing interface MTU after encapsulation.
>>>> When this happens ovpn should allow the kernel to fragment
>>>> them because they are "locally generated".
>>>>
>>>> To achieve the above, we must set skb->ignore_df = 1
>>>> so that ip6_fragment() can be made aware of this decision.
>>>
>>> Why the above applies only to IPv6? AFAICS the same could happen even
>>> for IPv4.
>>
>> For IPv4 we have the 'df=0' param that is passed to
>> udp_tunnel_xmit_skb(), which basically leads to the same result.
> 
> You need to include (an expanded/more describing version of) the above
> in the commit message.

Will do!
Thanks a lot.

Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net-next 04/10] ovpn: don't drop skb's dst when xmitting packet

[Antonio Quartulli]

When routing a packet to a LAN behind a peer, ovpn needs to
inspect the route entry that brought the packet there in the
first place.

If this packet is truly routable, the route entry provides the
GW to be used when looking up the VPN peer to send the packet to.

However, the route entry is currently dropped before entering
the ovpn xmit function, because the IFF_XMIT_DST_RELEASE priv_flag
is enabled by default.

Clear the IFF_XMIT_DST_RELEASE flag during interface setup to allow
the route entry (skb's dst) to survive and thus be inspected
by the ovpn routing logic.

Fixes: a3aaef8cd173 ("ovpn: implement peer lookup logic")
Reported-by: Gert Doering <gert@greenie.muc.de>
Tested-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Gert Doering <gert@greenie.muc.de> # as a primary user
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c   | 2 ++
 drivers/net/ovpn/main.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index dd8a8055d967..7e4b89484c9d 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -398,6 +398,8 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
 				    netdev_name(ovpn->dev));
 		goto drop;
 	}
+	/* dst was needed for peer selection - it can now be dropped */
+	skb_dst_drop(skb);
 
 	ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len);
 	ovpn_send(ovpn, skb_list.next, peer);
diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
index 0acb0934c1be..dcc094bf3ade 100644
--- a/drivers/net/ovpn/main.c
+++ b/drivers/net/ovpn/main.c
@@ -157,6 +157,11 @@ static void ovpn_setup(struct net_device *dev)
 	dev->type = ARPHRD_NONE;
 	dev->flags = IFF_POINTOPOINT | IFF_NOARP;
 	dev->priv_flags |= IFF_NO_QUEUE;
+	/* when routing packets to a LAN behind a client, we rely on the
+	 * route entry that originally brought the packet into ovpn, so
+	 * don't release it
+	 */
+	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
 
 	dev->lltx = true;
 	dev->features |= feat;
-- 
2.49.0

Re: [PATCH net-next 04/10] ovpn: don't drop skb's dst when xmitting packet

[Paolo Abeni]

On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> When routing a packet to a LAN behind a peer, ovpn needs to
> inspect the route entry that brought the packet there in the
> first place.
> 
> If this packet is truly routable, the route entry provides the
> GW to be used when looking up the VPN peer to send the packet to.
> 
> However, the route entry is currently dropped before entering
> the ovpn xmit function, because the IFF_XMIT_DST_RELEASE priv_flag
> is enabled by default.
> 
> Clear the IFF_XMIT_DST_RELEASE flag during interface setup to allow
> the route entry (skb's dst) to survive and thus be inspected
> by the ovpn routing logic.
> 
> Fixes: a3aaef8cd173 ("ovpn: implement peer lookup logic")
> Reported-by: Gert Doering <gert@greenie.muc.de>
> Tested-by: Gert Doering <gert@greenie.muc.de>
> Acked-by: Gert Doering <gert@greenie.muc.de> # as a primary user
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> ---
>  drivers/net/ovpn/io.c   | 2 ++
>  drivers/net/ovpn/main.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index dd8a8055d967..7e4b89484c9d 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -398,6 +398,8 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  				    netdev_name(ovpn->dev));
>  		goto drop;
>  	}
> +	/* dst was needed for peer selection - it can now be dropped */
> +	skb_dst_drop(skb);
>  
>  	ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len);
>  	ovpn_send(ovpn, skb_list.next, peer);
> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> index 0acb0934c1be..dcc094bf3ade 100644
> --- a/drivers/net/ovpn/main.c
> +++ b/drivers/net/ovpn/main.c
> @@ -157,6 +157,11 @@ static void ovpn_setup(struct net_device *dev)
>  	dev->type = ARPHRD_NONE;
>  	dev->flags = IFF_POINTOPOINT | IFF_NOARP;
>  	dev->priv_flags |= IFF_NO_QUEUE;
> +	/* when routing packets to a LAN behind a client, we rely on the
> +	 * route entry that originally brought the packet into ovpn, so
> +	 * don't release it
> +	 */
> +	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;

See commit 0287587884b15041203b3a362d485e1ab1f24445; the above should be

	netif_keep_dst(dev);

and no need to additional comment, as the helper nails it.

Thanks,

Paolo

Re: [PATCH net-next 04/10] ovpn: don't drop skb's dst when xmitting packet

[Antonio Quartulli]

On 13/05/2025 09:45, Paolo Abeni wrote:
[...]

>> +	/* when routing packets to a LAN behind a client, we rely on the
>> +	 * route entry that originally brought the packet into ovpn, so
>> +	 * don't release it
>> +	 */
>> +	dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> 
> See commit 0287587884b15041203b3a362d485e1ab1f24445; the above should be
> 
> 	netif_keep_dst(dev);
> 
> and no need to additional comment, as the helper nails it.

Thanks! I missed it.

Regards,

> 
> Thanks,
> 
> Paolo
> 

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net-next 05/10] selftest/net/ovpn: fix crash in case of getaddrinfo() failure

[Antonio Quartulli]

getaddrinfo() may fail with error code different from EAI_FAIL
or EAI_NONAME, however in this case we still try to free the
results object, thus leading to a crash.

Fix this by bailing out on any possible error.

Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 tools/testing/selftests/net/ovpn/ovpn-cli.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
index 69e41fc07fbc..c6372a1b4728 100644
--- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
+++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
@@ -1753,8 +1753,11 @@ static int ovpn_parse_remote(struct ovpn_ctx *ovpn, const char *host,
 
 	if (host) {
 		ret = getaddrinfo(host, service, &hints, &result);
-		if (ret == EAI_NONAME || ret == EAI_FAIL)
+		if (ret) {
+			fprintf(stderr, "getaddrinfo on remote error: %s\n",
+				gai_strerror(ret));
 			return -1;
+		}
 
 		if (!(result->ai_family == AF_INET &&
 		      result->ai_addrlen == sizeof(struct sockaddr_in)) &&
@@ -1769,8 +1772,11 @@ static int ovpn_parse_remote(struct ovpn_ctx *ovpn, const char *host,
 
 	if (vpnip) {
 		ret = getaddrinfo(vpnip, NULL, &hints, &result);
-		if (ret == EAI_NONAME || ret == EAI_FAIL)
+		if (ret) {
+			fprintf(stderr, "getaddrinfo on vpnip error: %s\n",
+				gai_strerror(ret));
 			return -1;
+		}
 
 		if (!(result->ai_family == AF_INET &&
 		      result->ai_addrlen == sizeof(struct sockaddr_in)) &&
-- 
2.49.0

Re: [PATCH net-next 05/10] selftest/net/ovpn: fix crash in case of getaddrinfo() failure

[Paolo Abeni]

On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> getaddrinfo() may fail with error code different from EAI_FAIL
> or EAI_NONAME, however in this case we still try to free the
> results object, thus leading to a crash.
> 
> Fix this by bailing out on any possible error.
> 
> Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> ---
>  tools/testing/selftests/net/ovpn/ovpn-cli.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> index 69e41fc07fbc..c6372a1b4728 100644
> --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
> +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
> @@ -1753,8 +1753,11 @@ static int ovpn_parse_remote(struct ovpn_ctx *ovpn, const char *host,
>  
>  	if (host) {
>  		ret = getaddrinfo(host, service, &hints, &result);
> -		if (ret == EAI_NONAME || ret == EAI_FAIL)
> +		if (ret) {
> +			fprintf(stderr, "getaddrinfo on remote error: %s\n",
> +				gai_strerror(ret));
>  			return -1;

Side note: you could instead use the libcall error(), even if at this
point it would be a quite largish self-test refactor.

/P

Re: [PATCH net-next 05/10] selftest/net/ovpn: fix crash in case of getaddrinfo() failure

[Antonio Quartulli]

On 13/05/2025 09:48, Paolo Abeni wrote:
> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
>> getaddrinfo() may fail with error code different from EAI_FAIL
>> or EAI_NONAME, however in this case we still try to free the
>> results object, thus leading to a crash.
>>
>> Fix this by bailing out on any possible error.
>>
>> Fixes: 959bc330a439 ("testing/selftests: add test tool and scripts for ovpn module")
>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>> ---
>>   tools/testing/selftests/net/ovpn/ovpn-cli.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> index 69e41fc07fbc..c6372a1b4728 100644
>> --- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> +++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
>> @@ -1753,8 +1753,11 @@ static int ovpn_parse_remote(struct ovpn_ctx *ovpn, const char *host,
>>   
>>   	if (host) {
>>   		ret = getaddrinfo(host, service, &hints, &result);
>> -		if (ret == EAI_NONAME || ret == EAI_FAIL)
>> +		if (ret) {
>> +			fprintf(stderr, "getaddrinfo on remote error: %s\n",
>> +				gai_strerror(ret));
>>   			return -1;
> 
> Side note: you could instead use the libcall error(), even if at this
> point it would be a quite largish self-test refactor.

Thanks for the hint!
I'll add this to my todo list and I'll do a full ovpn-cli.c revamp with 
error().

Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net-next 06/10] ovpn: fix ndo_start_xmit return value on error

[Antonio Quartulli]

ndo_start_xmit is basically expected to always return NETDEV_TX_OK.
However, in case of error, it was currently returning NET_XMIT_DROP,
which is not a valid netdev_tx_t return value, leading to
misinterpretation.

Change ndo_start_xmit to always return NETDEV_TX_OK to signal back
to the caller that the packet was handled (even if dropped).

Effects of this bug can be seen when sending IPv6 packets having
no peer to forward them to:

  $ ip netns exec ovpn-server oping -c20 fd00:abcd:220:201::1
  PING fd00:abcd:220:201::1 (fd00:abcd:220:201::1) 56 bytes of data.00:abcd:220:201 :1
  ping_send failed: No buffer space available
  ping_sendto: No buffer space available
  ping_send failed: No buffer space available
  ping_sendto: No buffer space available
  ...

Fixes: c2d950c4672a ("ovpn: add basic interface creation/destruction/management routines")
Reported-by: Gert Doering <gert@greenie.muc.de>
Tested-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Gert Doering <gert@greenie.muc.de>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 7e4b89484c9d..43f428ac112e 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -410,7 +410,7 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev_dstats_tx_dropped(ovpn->dev);
 	skb_tx_error(skb);
 	kfree_skb_list(skb);
-	return NET_XMIT_DROP;
+	return NETDEV_TX_OK;
 }
 
 /**
-- 
2.49.0

[PATCH net-next 07/10] selftest/net/ovpn: extend coverage with more test cases

[Antonio Quartulli]

To increase code coverage, extend the ovpn selftests with the following
cases:
* connect UDP peers using a mix of IPv6 and IPv4 at the transport layer
* run full test with tunnel MTU equal to transport MTU (exercising
  IP layer fragmentation)
* ping "LAN IP" served by VPN peer ("LAN behind a client" test case)

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 tools/testing/selftests/net/ovpn/Makefile      |  1 +
 tools/testing/selftests/net/ovpn/common.sh     | 18 +++++++++++++++++-
 tools/testing/selftests/net/ovpn/ovpn-cli.c    |  9 +++++----
 tools/testing/selftests/net/ovpn/test.sh       |  6 +++++-
 tools/testing/selftests/net/ovpn/udp_peers.txt | 11 ++++++-----
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/ovpn/Makefile b/tools/testing/selftests/net/ovpn/Makefile
index 2d102878cb6d..e0926d76b4c8 100644
--- a/tools/testing/selftests/net/ovpn/Makefile
+++ b/tools/testing/selftests/net/ovpn/Makefile
@@ -20,6 +20,7 @@ LDLIBS += $(VAR_LDLIBS)
 TEST_FILES = common.sh
 
 TEST_PROGS = test.sh \
+	test-large-mtu.sh \
 	test-chachapoly.sh \
 	test-tcp.sh \
 	test-float.sh \
diff --git a/tools/testing/selftests/net/ovpn/common.sh b/tools/testing/selftests/net/ovpn/common.sh
index 7502292a1ee0..88869c675d03 100644
--- a/tools/testing/selftests/net/ovpn/common.sh
+++ b/tools/testing/selftests/net/ovpn/common.sh
@@ -11,6 +11,8 @@ ALG=${ALG:-aes}
 PROTO=${PROTO:-UDP}
 FLOAT=${FLOAT:-0}
 
+LAN_IP="11.11.11.11"
+
 create_ns() {
 	ip netns add peer${1}
 }
@@ -24,15 +26,25 @@ setup_ns() {
 			ip link add veth${p} netns peer0 type veth peer name veth${p} netns peer${p}
 
 			ip -n peer0 addr add 10.10.${p}.1/24 dev veth${p}
+			ip -n peer0 addr add fd00:0:0:${p}::1/64 dev veth${p}
 			ip -n peer0 link set veth${p} up
 
 			ip -n peer${p} addr add 10.10.${p}.2/24 dev veth${p}
+			ip -n peer${p} addr add fd00:0:0:${p}::2/64 dev veth${p}
 			ip -n peer${p} link set veth${p} up
 		done
 	fi
 
 	ip netns exec peer${1} ${OVPN_CLI} new_iface tun${1} $MODE
 	ip -n peer${1} addr add ${2} dev tun${1}
+	# add a secondary IP to peer 1, to test a LAN behind a client
+	if [ ${1} -eq 1 -a -n "${LAN_IP}" ]; then
+		ip -n peer${1} addr add ${LAN_IP} dev tun${1}
+		ip -n peer0 route add ${LAN_IP} via $(echo ${2} |sed -e s'!/.*!!') dev tun0
+	fi
+	if [ -n "${3}" ]; then
+		ip -n peer${1} link set mtu ${3} dev tun${1}
+	fi
 	ip -n peer${1} link set tun${1} up
 }
 
@@ -46,7 +58,11 @@ add_peer() {
 					data64.key
 			done
 		else
-			ip netns exec peer${1} ${OVPN_CLI} new_peer tun${1} ${1} 1 10.10.${1}.1 1
+			RADDR=$(awk "NR == ${1} {print \$2}" ${UDP_PEERS_FILE})
+			RPORT=$(awk "NR == ${1} {print \$3}" ${UDP_PEERS_FILE})
+			LPORT=$(awk "NR == ${1} {print \$5}" ${UDP_PEERS_FILE})
+			ip netns exec peer${1} ${OVPN_CLI} new_peer tun${1} ${1} ${LPORT} \
+				${RADDR} ${RPORT}
 			ip netns exec peer${1} ${OVPN_CLI} new_key tun${1} ${1} 1 0 ${ALG} 1 \
 				data64.key
 		fi
diff --git a/tools/testing/selftests/net/ovpn/ovpn-cli.c b/tools/testing/selftests/net/ovpn/ovpn-cli.c
index c6372a1b4728..de9c26f98b2e 100644
--- a/tools/testing/selftests/net/ovpn/ovpn-cli.c
+++ b/tools/testing/selftests/net/ovpn/ovpn-cli.c
@@ -1934,7 +1934,8 @@ static void ovpn_waitbg(void)
 
 static int ovpn_run_cmd(struct ovpn_ctx *ovpn)
 {
-	char peer_id[10], vpnip[INET6_ADDRSTRLEN], raddr[128], rport[10];
+	char peer_id[10], vpnip[INET6_ADDRSTRLEN], laddr[128], lport[10];
+	char raddr[128], rport[10];
 	int n, ret;
 	FILE *fp;
 
@@ -2050,8 +2051,8 @@ static int ovpn_run_cmd(struct ovpn_ctx *ovpn)
 			return -1;
 		}
 
-		while ((n = fscanf(fp, "%s %s %s %s\n", peer_id, raddr, rport,
-				   vpnip)) == 4) {
+		while ((n = fscanf(fp, "%s %s %s %s %s %s\n", peer_id, laddr,
+				   lport, raddr, rport, vpnip)) == 6) {
 			struct ovpn_ctx peer_ctx = { 0 };
 
 			peer_ctx.ifindex = ovpn->ifindex;
@@ -2355,7 +2356,7 @@ int main(int argc, char *argv[])
 	}
 
 	memset(&ovpn, 0, sizeof(ovpn));
-	ovpn.sa_family = AF_INET;
+	ovpn.sa_family = AF_UNSPEC;
 	ovpn.cipher = OVPN_CIPHER_ALG_NONE;
 
 	ovpn.cmd = ovpn_parse_cmd(argv[1]);
diff --git a/tools/testing/selftests/net/ovpn/test.sh b/tools/testing/selftests/net/ovpn/test.sh
index 7b62897b0240..e8acdc303307 100755
--- a/tools/testing/selftests/net/ovpn/test.sh
+++ b/tools/testing/selftests/net/ovpn/test.sh
@@ -18,7 +18,7 @@ for p in $(seq 0 ${NUM_PEERS}); do
 done
 
 for p in $(seq 0 ${NUM_PEERS}); do
-	setup_ns ${p} 5.5.5.$((${p} + 1))/24
+	setup_ns ${p} 5.5.5.$((${p} + 1))/24 ${MTU}
 done
 
 for p in $(seq 0 ${NUM_PEERS}); do
@@ -34,8 +34,12 @@ sleep 1
 
 for p in $(seq 1 ${NUM_PEERS}); do
 	ip netns exec peer0 ping -qfc 500 -w 3 5.5.5.$((${p} + 1))
+	ip netns exec peer0 ping -qfc 500 -s 3000 -w 3 5.5.5.$((${p} + 1))
 done
 
+# ping LAN behind client 1
+ip netns exec peer0 ping -qfc 500 -w 3 ${LAN_IP}
+
 if [ "$FLOAT" == "1" ]; then
 	# make clients float..
 	for p in $(seq 1 ${NUM_PEERS}); do
diff --git a/tools/testing/selftests/net/ovpn/udp_peers.txt b/tools/testing/selftests/net/ovpn/udp_peers.txt
index 32f14bd9347a..e9773ddf875c 100644
--- a/tools/testing/selftests/net/ovpn/udp_peers.txt
+++ b/tools/testing/selftests/net/ovpn/udp_peers.txt
@@ -1,5 +1,6 @@
-1 10.10.1.2 1 5.5.5.2
-2 10.10.2.2 1 5.5.5.3
-3 10.10.3.2 1 5.5.5.4
-4 10.10.4.2 1 5.5.5.5
-5 10.10.5.2 1 5.5.5.6
+1 10.10.1.1 1 10.10.1.2 1 5.5.5.2
+2 10.10.2.1 1 10.10.2.2 1 5.5.5.3
+3 10.10.3.1 1 10.10.3.2 1 5.5.5.4
+4 fd00:0:0:4::1 1 fd00:0:0:4::2 1 5.5.5.5
+5 fd00:0:0:5::1 1 fd00:0:0:5::2 1 5.5.5.6
+6 fd00:0:0:6::1 1 fd00:0:0:6::2 1 5.5.5.7
-- 
2.49.0

[PATCH net-next 08/10] ovpn: drop useless reg_state check in keepalive worker

[Antonio Quartulli]

The keepalive worker is cancelled before calling
unregister_netdevice_queue(), therefore it will never
hit a situation where the reg_state can be different
than NETDEV_REGISTERED.

For this reason, checking reg_state is useless and the
condition can be removed.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/peer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a37f89fffb02..24eb9d81429e 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -1353,8 +1353,7 @@ void ovpn_peer_keepalive_work(struct work_struct *work)
 	}
 
 	/* prevent rearming if the interface is being destroyed */
-	if (next_run > 0 &&
-	    READ_ONCE(ovpn->dev->reg_state) == NETREG_REGISTERED) {
+	if (next_run > 0) {
 		netdev_dbg(ovpn->dev,
 			   "scheduling keepalive work: now=%llu next_run=%llu delta=%llu\n",
 			   next_run, now, next_run - now);
-- 
2.49.0

[PATCH net-next 09/10] ovpn: improve 'no route to host' debug message

[Antonio Quartulli]

When debugging a 'no route to host' error it can be beneficial
to know the address of the unreachable destination.
Print it along the debugging text.

While at it, add a missing parenthesis in another debugging
message.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c   | 14 ++++++++++++--
 drivers/net/ovpn/peer.c |  2 +-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 43f428ac112e..10d8afecec55 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -394,8 +394,18 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* 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",
-				    netdev_name(ovpn->dev));
+		switch (skb->protocol) {
+		case htons(ETH_P_IP):
+			net_dbg_ratelimited("%s: no peer to send data to dst=%pI4\n",
+					    netdev_name(ovpn->dev),
+					    &ip_hdr(skb)->daddr);
+			break;
+		case htons(ETH_P_IPV6):
+			net_dbg_ratelimited("%s: no peer to send data to dst=%pI6c\n",
+					    netdev_name(ovpn->dev),
+					    &ipv6_hdr(skb)->daddr);
+			break;
+		}
 		goto drop;
 	}
 	/* dst was needed for peer selection - it can now be dropped */
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index 24eb9d81429e..a1fd27b9c038 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -258,7 +258,7 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
 		 */
 		if (unlikely(!ipv6_addr_equal(&bind->local.ipv6,
 					      &ipv6_hdr(skb)->daddr))) {
-			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c\n",
+			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c)\n",
 					    netdev_name(peer->ovpn->dev),
 					    peer->id, &bind->local.ipv6,
 					    &ipv6_hdr(skb)->daddr);
-- 
2.49.0

Re: [PATCH net-next 09/10] ovpn: improve 'no route to host' debug message

[Paolo Abeni]

On 5/9/25 4:26 PM, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> index 24eb9d81429e..a1fd27b9c038 100644
> --- a/drivers/net/ovpn/peer.c
> +++ b/drivers/net/ovpn/peer.c
> @@ -258,7 +258,7 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
>  		 */
>  		if (unlikely(!ipv6_addr_equal(&bind->local.ipv6,
>  					      &ipv6_hdr(skb)->daddr))) {
> -			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c\n",
> +			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c)\n",
>  					    netdev_name(peer->ovpn->dev),
>  					    peer->id, &bind->local.ipv6,
>  					    &ipv6_hdr(skb)->daddr);

Since you have to repost it's better to move this chunk to a separate
patch, as it's unrelated to the previous one - or at very least mention
it explicitly in the commit message.

/P

Re: [PATCH net-next 09/10] ovpn: improve 'no route to host' debug message

[Antonio Quartulli]

On 13/05/2025 09:53, Paolo Abeni wrote:
> On 5/9/25 4:26 PM, Antonio Quartulli wrote:
>> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
>> index 24eb9d81429e..a1fd27b9c038 100644
>> --- a/drivers/net/ovpn/peer.c
>> +++ b/drivers/net/ovpn/peer.c
>> @@ -258,7 +258,7 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
>>   		 */
>>   		if (unlikely(!ipv6_addr_equal(&bind->local.ipv6,
>>   					      &ipv6_hdr(skb)->daddr))) {
>> -			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c\n",
>> +			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c)\n",
>>   					    netdev_name(peer->ovpn->dev),
>>   					    peer->id, &bind->local.ipv6,
>>   					    &ipv6_hdr(skb)->daddr);
> 
> Since you have to repost it's better to move this chunk to a separate
> patch, as it's unrelated to the previous one - or at very least mention
> it explicitly in the commit message.

Yeah, the line:
"While at it, add a missing parenthesis in another debugging
message."

was too vague :)

I'll make it more explicit.

Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

In case of UDP peer timeout, an openvpn client (userspace)
performs the following actions:
1. receives the peer deletion notification (reason=timeout)
2. closes the socket

Upon 1. we have the following:
- ovpn_peer_keepalive_work()
 - ovpn_socket_release()
  - synchronize_rcu()
At this point, 2. gets a chance to complete and ovpn_sock->sock->sk
becomes NULL. ovpn_socket_release() will then attempt dereferencing it,
resulting in the following crash log:

Oops: general protection fault, probably for non-canonical address 0xdffffc0000000077: 0000 [#1] SMP KASAN
KASAN: null-ptr-deref in range [0x00000000000003b8-0x00000000000003bf]
CPU: 12 UID: 0 PID: 162 Comm: kworker/12:1 Tainted: G           O        6.15.0-rc2-00635-g521139ac3840 #272 PREEMPT(full)
Tainted: [O]=OOT_MODULE
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-20240910_120124-localhost 04/01/2014
Workqueue: events ovpn_peer_keepalive_work [ovpn]
RIP: 0010:ovpn_socket_release+0x23c/0x500 [ovpn]
Code: ea 03 80 3c 02 00 0f 85 71 02 00 00 48 b8 00 00 00 00 00 fc ff df 4d 8b 64 24 18 49 8d bc 24 be 03 00 00 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 01 38 d0 7c 08 84 d2 0f 85 30
RSP: 0018:ffffc90000c9fb18 EFLAGS: 00010217
RAX: dffffc0000000000 RBX: ffff8881148d7940 RCX: ffffffff817787bb
RDX: 0000000000000077 RSI: 0000000000000008 RDI: 00000000000003be
RBP: ffffc90000c9fb30 R08: 0000000000000000 R09: fffffbfff0d3e840
R10: ffffffff869f4207 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888115eb9300 R14: ffffc90000c9fbc8 R15: 000000000000000c
FS:  0000000000000000(0000) GS:ffff8882b0151000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f37266b6114 CR3: 00000000054a8000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 unlock_ovpn+0x8b/0xe0 [ovpn]
 ovpn_peer_keepalive_work+0xe3/0x540 [ovpn]
 ? ovpn_peers_free+0x780/0x780 [ovpn]
 ? lock_acquire+0x56/0x70
 ? process_one_work+0x888/0x1740
 process_one_work+0x933/0x1740
 ? pwq_dec_nr_in_flight+0x10b0/0x10b0
 ? move_linked_works+0x12d/0x2c0
 ? assign_work+0x163/0x270
 worker_thread+0x4d6/0xd90
 ? preempt_count_sub+0x4c/0x70
 ? process_one_work+0x1740/0x1740
 kthread+0x36c/0x710
 ? trace_preempt_on+0x8c/0x1e0
 ? kthread_is_per_cpu+0xc0/0xc0
 ? preempt_count_sub+0x4c/0x70
 ? _raw_spin_unlock_irq+0x36/0x60
 ? calculate_sigpending+0x7b/0xa0
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork+0x3a/0x80
 ? kthread_is_per_cpu+0xc0/0xc0
 ret_from_fork_asm+0x11/0x20
 </TASK>
Modules linked in: ovpn(O)

Reason for accessing sk is ithat we need to retrieve its
protocol and continue the cleanup routine accordingly.

Fix the crash by grabbing a reference to sk before proceeding
with the cleanup. If the refcounter has reached zero, we
know that the socket is being destroyed and thus we skip
the cleanup in ovpn_socket_release().

Fixes: 11851cbd60ea ("ovpn: implement TCP transport")
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Dumazet <edumazet@google.com>
Reported-by: Qingfang Deng <dqfext@gmail.com>
Tested-By: Gert Doering <gert@greenie.muc.de>
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/socket.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ovpn/socket.c b/drivers/net/ovpn/socket.c
index a83cbab72591..66a2ecbc483b 100644
--- a/drivers/net/ovpn/socket.c
+++ b/drivers/net/ovpn/socket.c
@@ -66,6 +66,7 @@ static bool ovpn_socket_put(struct ovpn_peer *peer, struct ovpn_socket *sock)
 void ovpn_socket_release(struct ovpn_peer *peer)
 {
 	struct ovpn_socket *sock;
+	struct sock *sk;
 	bool released;
 
 	might_sleep();
@@ -75,13 +76,14 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	if (!sock)
 		return;
 
-	/* sanity check: we should not end up here if the socket
-	 * was already closed
+	/* sock->sk may be released concurrently, therefore we
+	 * first attempt grabbing a reference.
+	 * if sock->sk is NULL it means it is already being
+	 * destroyed and we don't need any further cleanup
 	 */
-	if (!sock->sock->sk) {
-		DEBUG_NET_WARN_ON_ONCE(1);
+	sk = sock->sock->sk;
+	if (!sk || !refcount_inc_not_zero(&sk->sk_refcnt))
 		return;
-	}
 
 	/* Drop the reference while holding the sock lock to avoid
 	 * concurrent ovpn_socket_new call to mess up with a partially
@@ -90,18 +92,18 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 	 * Holding the lock ensures that a socket with refcnt 0 is fully
 	 * detached before it can be picked by a concurrent reader.
 	 */
-	lock_sock(sock->sock->sk);
+	lock_sock(sk);
 	released = ovpn_socket_put(peer, sock);
-	release_sock(sock->sock->sk);
+	release_sock(sk);
 
 	/* align all readers with sk_user_data being NULL */
 	synchronize_rcu();
 
 	/* following cleanup should happen with lock released */
 	if (released) {
-		if (sock->sock->sk->sk_protocol == IPPROTO_UDP) {
+		if (sk->sk_protocol == IPPROTO_UDP) {
 			netdev_put(sock->ovpn->dev, &sock->dev_tracker);
-		} else if (sock->sock->sk->sk_protocol == IPPROTO_TCP) {
+		} else if (sk->sk_protocol == IPPROTO_TCP) {
 			/* wait for TCP jobs to terminate */
 			ovpn_tcp_socket_wait_finish(sock);
 			ovpn_peer_put(sock->peer);
@@ -111,6 +113,7 @@ void ovpn_socket_release(struct ovpn_peer *peer)
 		 */
 		kfree(sock);
 	}
+	sock_put(sk);
 }
 
 static bool ovpn_socket_hold(struct ovpn_socket *sock)
-- 
2.49.0

Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

[Jakub Kicinski]

On Fri,  9 May 2025 16:26:20 +0200 Antonio Quartulli wrote:
> In case of UDP peer timeout, an openvpn client (userspace)
> performs the following actions:
> 1. receives the peer deletion notification (reason=timeout)
> 2. closes the socket
> 
> Upon 1. we have the following:
> - ovpn_peer_keepalive_work()
>  - ovpn_socket_release()
>   - synchronize_rcu()
> At this point, 2. gets a chance to complete and ovpn_sock->sock->sk
> becomes NULL. ovpn_socket_release() will then attempt dereferencing it,
> resulting in the following crash log:

What runs where is a bit unclear to me. Specifically I'm not sure what
runs the code under the "if (released)" branch of ovpn_socket_release()
if the user closes the socket. Because you now return without a WARN().

> @@ -75,13 +76,14 @@ void ovpn_socket_release(struct ovpn_peer *peer)
>  	if (!sock)
>  		return;
>  
> -	/* sanity check: we should not end up here if the socket
> -	 * was already closed
> +	/* sock->sk may be released concurrently, therefore we
> +	 * first attempt grabbing a reference.
> +	 * if sock->sk is NULL it means it is already being
> +	 * destroyed and we don't need any further cleanup
>  	 */
> -	if (!sock->sock->sk) {
> -		DEBUG_NET_WARN_ON_ONCE(1);
> +	sk = sock->sock->sk;
> +	if (!sk || !refcount_inc_not_zero(&sk->sk_refcnt))

How is sk protected from getting reused here?
refcount_inc_not_zero() still needs the underlying object to be allocated.
I don't see any locking here, and code says this function may sleep so 
it can't be called under RCU, either.

Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

[Paolo Abeni]

On 5/13/25 3:37 AM, Jakub Kicinski wrote:
> On Fri,  9 May 2025 16:26:20 +0200 Antonio Quartulli wrote:
>> In case of UDP peer timeout, an openvpn client (userspace)
>> performs the following actions:
>> 1. receives the peer deletion notification (reason=timeout)
>> 2. closes the socket
>>
>> Upon 1. we have the following:
>> - ovpn_peer_keepalive_work()
>>  - ovpn_socket_release()
>>   - synchronize_rcu()
>> At this point, 2. gets a chance to complete and ovpn_sock->sock->sk
>> becomes NULL. ovpn_socket_release() will then attempt dereferencing it,
>> resulting in the following crash log:
> 
> What runs where is a bit unclear to me. Specifically I'm not sure what
> runs the code under the "if (released)" branch of ovpn_socket_release()
> if the user closes the socket. Because you now return without a WARN().
> 
>> @@ -75,13 +76,14 @@ void ovpn_socket_release(struct ovpn_peer *peer)
>>  	if (!sock)
>>  		return;
>>  
>> -	/* sanity check: we should not end up here if the socket
>> -	 * was already closed
>> +	/* sock->sk may be released concurrently, therefore we
>> +	 * first attempt grabbing a reference.
>> +	 * if sock->sk is NULL it means it is already being
>> +	 * destroyed and we don't need any further cleanup
>>  	 */
>> -	if (!sock->sock->sk) {
>> -		DEBUG_NET_WARN_ON_ONCE(1);
>> +	sk = sock->sock->sk;
>> +	if (!sk || !refcount_inc_not_zero(&sk->sk_refcnt))
> 
> How is sk protected from getting reused here?
> refcount_inc_not_zero() still needs the underlying object to be allocated.
> I don't see any locking here, and code says this function may sleep so 
> it can't be called under RCU, either.

I agree this still looks racy. When the socket close runs, nobody else
should have access/reference to the 'struct socket'. I'm under the
impression that ovpn_socket should acquire references to the underlying
fd instead of keeping its own refcount.

Side note: the ovpn_socket refcount release/detach path looks wrong, at
least in case of an UDP socket, as ovpn_udp_socket_detach() calls
setup_udp_tunnel_sock() which in turns will try to _increment_ various
core counters, instead of decreasing them (i.e. udp_encap_enable should
be wrongly accounted after that call).

/P

Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

On 13/05/2025 10:21, Paolo Abeni wrote:
> 
> 
> On 5/13/25 3:37 AM, Jakub Kicinski wrote:
>> On Fri,  9 May 2025 16:26:20 +0200 Antonio Quartulli wrote:
>>> In case of UDP peer timeout, an openvpn client (userspace)
>>> performs the following actions:
>>> 1. receives the peer deletion notification (reason=timeout)
>>> 2. closes the socket
>>>
>>> Upon 1. we have the following:
>>> - ovpn_peer_keepalive_work()
>>>   - ovpn_socket_release()
>>>    - synchronize_rcu()
>>> At this point, 2. gets a chance to complete and ovpn_sock->sock->sk
>>> becomes NULL. ovpn_socket_release() will then attempt dereferencing it,
>>> resulting in the following crash log:
>>
>> What runs where is a bit unclear to me. Specifically I'm not sure what
>> runs the code under the "if (released)" branch of ovpn_socket_release()
>> if the user closes the socket. Because you now return without a WARN().
>>
>>> @@ -75,13 +76,14 @@ void ovpn_socket_release(struct ovpn_peer *peer)
>>>   	if (!sock)
>>>   		return;
>>>   
>>> -	/* sanity check: we should not end up here if the socket
>>> -	 * was already closed
>>> +	/* sock->sk may be released concurrently, therefore we
>>> +	 * first attempt grabbing a reference.
>>> +	 * if sock->sk is NULL it means it is already being
>>> +	 * destroyed and we don't need any further cleanup
>>>   	 */
>>> -	if (!sock->sock->sk) {
>>> -		DEBUG_NET_WARN_ON_ONCE(1);
>>> +	sk = sock->sock->sk;
>>> +	if (!sk || !refcount_inc_not_zero(&sk->sk_refcnt))
>>
>> How is sk protected from getting reused here?
>> refcount_inc_not_zero() still needs the underlying object to be allocated.
>> I don't see any locking here, and code says this function may sleep so
>> it can't be called under RCU, either.
> 
> I agree this still looks racy. When the socket close runs, nobody else
> should have access/reference to the 'struct socket'. I'm under the
> impression that ovpn_socket should acquire references to the underlying
> fd instead of keeping its own refcount.

This is what we were originally doing, but since the socket is not a 
"kernel socket", increasing the refcount was preventing us from 
understanding when the socket was supposed to be destroyed (because ovpn 
itself was still holding a ref).
Hence we switched to this model where we get notified about the socket 
going away via close()/destroy() call.


I think ovpn_socket should coordinate access to its sock member and 
nullify it during destroy (which is invoked by sk_common_release()).
At that point no other part of the code will have a chance to access it.

I am gonna play with this idea right now.


> 
> Side note: the ovpn_socket refcount release/detach path looks wrong, at
> least in case of an UDP socket, as ovpn_udp_socket_detach() calls
> setup_udp_tunnel_sock() which in turns will try to _increment_ various
> core counters, instead of decreasing them (i.e. udp_encap_enable should
> be wrongly accounted after that call).

You're right.
I had the impression I needed to "undo" the setup.
I see now that the encap key is decremented in the UDP sock destroy, 
right after having called my implementation of .destroy().

I'll drop the call to setup_udp_tunnel_sock() with empty config then.

Regards,

> 
> /P
> 

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

[Antonio Quartulli]

On 13/05/2025 11:19, Antonio Quartulli wrote:
[...]

>> Side note: the ovpn_socket refcount release/detach path looks wrong, at
>> least in case of an UDP socket, as ovpn_udp_socket_detach() calls
>> setup_udp_tunnel_sock() which in turns will try to _increment_ various
>> core counters, instead of decreasing them (i.e. udp_encap_enable should
>> be wrongly accounted after that call).
> 
> You're right.
> I had the impression I needed to "undo" the setup.
> I see now that the encap key is decremented in the UDP sock destroy, 
> right after having called my implementation of .destroy().
> 
> I'll drop the call to setup_udp_tunnel_sock() with empty config then.

Paolo, the reason for calling setup_udp_tunnel_sock() with an empty 
config was to "reset" its encap state (for udp_tunnel).

Technically a UDP socket could go back to being a pure userspace socket 
only (i.e. when all peers using that socket have been deleted), 
therefore I wanted to make sure that the kernel would not try to 
intercept its packets anymore.

I understand setup_udp_tunnel_sock(cfg={}) is not correct, but what are 
my options?

1) I could nullify manually sk_user_data and udp_sk(sk)->encap_type;

2) I could implement something like "stop_udp_tunnel_sock()" that 
reverts what setup_udp_tunnel_sock() had done and call it.

Opinions?
I'd go with 2.

Thanks a lot!


-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net-next 00/10] pull request for net-next: ovpn 2025-05-09

[Andrew Lunn]

On Fri, May 09, 2025 at 04:26:10PM +0200, Antonio Quartulli wrote:
> Hi Jakub,
> 
> here is my first pull request for the ovpn kernel module!
> 
> As you can see in the patches, we have various tags from
> Gert Döring, the main maintainer of openvpn userspace,
> who was eager to test and report malfunctionings.
> He reported bugs, stared at fixes and tested them, hence
> the Reported/Acked/Tested-by tags. If you think such
> combination of tags is not truly appropriate, please let
> me know.

Ideally, all this discussion should have happened on the netdev, where
others can take part and learn more about how ovpn works. If this
happened in the open on some other list, please could you include a
link in the patches.

       Andrew

Re: [PATCH net-next 00/10] pull request for net-next: ovpn 2025-05-09

[Antonio Quartulli]

On 09/05/2025 16:40, Andrew Lunn wrote:
> On Fri, May 09, 2025 at 04:26:10PM +0200, Antonio Quartulli wrote:
>> Hi Jakub,
>>
>> here is my first pull request for the ovpn kernel module!
>>
>> As you can see in the patches, we have various tags from
>> Gert Döring, the main maintainer of openvpn userspace,
>> who was eager to test and report malfunctionings.
>> He reported bugs, stared at fixes and tested them, hence
>> the Reported/Acked/Tested-by tags. If you think such
>> combination of tags is not truly appropriate, please let
>> me know.
> 
> Ideally, all this discussion should have happened on the netdev, where
> others can take part and learn more about how ovpn works. If this
> happened in the open on some other list, please could you include a
> link in the patches.

Reporting happens often on our IRC channel (#openvpn-devel @ 
irc.libera.chat) and then we track bugs on GitHub[1].

After getting the bug report I sent bugfixes/patches to our public 
mailing list (the one listed in MAINTAINERS).
At that point Gert reported his findings (with his tags) in response to 
the patches.

I can add a "Link:" tag to each patch, where I provide the URL of the 
patch on the openvpn-devel mailing list (with following discussion).

I don't know if I should also include the URL to the GitHub issue?
If yes, which tag should I use?


Thanks a lot!


[1]https://github.com/OpenVPN/ovpn-net-next/issues

-- 
Antonio Quartulli
OpenVPN Inc.