[Openvpn-devel,ovpn-net-next] ovpn: fix ndo_start_xmit return value on error

Message ID 20250507124439.28161-1-a@unstable.cc
State New
Headers show
Series [Openvpn-devel,ovpn-net-next] ovpn: fix ndo_start_xmit return value on error | expand

Commit Message

Antonio Quartulli May 7, 2025, 12:44 p.m. UTC
From: Antonio Quartulli <antonio@openvpn.net>

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")
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
 drivers/net/ovpn/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gert Doering May 7, 2025, 2:02 p.m. UTC | #1
Hi,

On Wed, May 07, 2025 at 02:44:39PM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@openvpn.net>
> 
> 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")
> 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 dd8a8055d967..03728ce733a9 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -408,7 +408,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;
>  }

Tested, works :-) - so the previous behaviour was "oping was hanging
due to 'out of buffer space' error not being counted as 'packet sent'"
and now it's behaving as any regular network interface when there
is no ARP/IPv6 ND for that neighbour.  Explanation makes sense, too :-)

So

Acked-By: Gert Doering <gert@greenie.muc.de>

gert
Gert Doering May 7, 2025, 2:36 p.m. UTC | #2
Hi,

On Wed, May 07, 2025 at 04:02:54PM +0200, Gert Doering wrote:
> Tested, works :-) - so the previous behaviour was "oping was hanging
> due to 'out of buffer space' error not being counted as 'packet sent'"
> and now it's behaving as any regular network interface when there
> is no ARP/IPv6 ND for that neighbour.  Explanation makes sense, too :-)
> 
> So
> 
> Acked-By: Gert Doering <gert@greenie.muc.de>

Antonio says if I test this, I should state so...

Tested-By: Gert Doering <gert@greenie.muc.de>
Acked-By: Gert Doering <gert@greenie.muc.de>

here we go :-)

gert
Antonio Quartulli May 7, 2025, 7:40 p.m. UTC | #3
On 07/05/2025 16:36, Gert Doering wrote:
> Hi,
> 
> On Wed, May 07, 2025 at 04:02:54PM +0200, Gert Doering wrote:
>> Tested, works :-) - so the previous behaviour was "oping was hanging
>> due to 'out of buffer space' error not being counted as 'packet sent'"
>> and now it's behaving as any regular network interface when there
>> is no ARP/IPv6 ND for that neighbour.  Explanation makes sense, too :-)
>>
>> So
>>
>> Acked-By: Gert Doering <gert@greenie.muc.de>
> 
> Antonio says if I test this, I should state so...
> 
> Tested-By: Gert Doering <gert@greenie.muc.de>
> Acked-By: Gert Doering <gert@greenie.muc.de>
> 
> here we go :-)

Merged to main branch, commit id 2176a2102216

Thanks!

Patch

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index dd8a8055d967..03728ce733a9 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -408,7 +408,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;
 }
 
 /**