[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <39b4ca40-a4a8-447f-b2a7-7221437fad22@openvpn.net>
Date: Wed, 2 Apr 2025 14:11:43 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v24 16/23] ovpn: implement keepalive mechanism
On 01/04/2025 14:51, Sabrina Dubroca wrote:
> 2025-03-18, 02:40:51 +0100, Antonio Quartulli wrote:
>> @@ -124,6 +154,13 @@ void ovpn_decrypt_post(void *data, int ret)
>> goto drop;
>> }
>>
>> + if (ovpn_is_keepalive(skb)) {
>> + net_dbg_ratelimited("%s: ping received from peer %u\n",
>> + netdev_name(peer->ovpn->dev),
>> + peer->id);
>> + goto drop_nocount;
>> + }
>> +
>> net_info_ratelimited("%s: unsupported protocol received from peer %u\n",
>> netdev_name(peer->ovpn->dev), peer->id);
>> goto drop;
>> @@ -149,6 +186,7 @@ void ovpn_decrypt_post(void *data, int ret)
>> drop:
>> if (unlikely(skb))
>> dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
>> +drop_nocount:
>> if (likely(peer))
>> ovpn_peer_put(peer);
>> if (likely(ks))
>> kfree_skb(skb);
>> }
>
> Again a small thing: in the case of a keepalive message, it would also
> be nice to use consume_skb instead of kfree_skb. Quoting from the doc
> for consume_skb:
>
> * Functions identically to kfree_skb, but kfree_skb assumes that the frame
> * is being dropped after a failure and notes that
I agree! I always try to pay attention to when consume_skb() should be
used, but I must have missed this special case.
>
>
>
> Something like this maybe (not compiled):
>
> /* skb is passed to upper layer - don't free it */
> skb = NULL;
> drop:
> if (unlikely(skb))
> dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> kfree_skb(skb);
> skb = NULL;
> drop_nocount:
> if (likely(peer))
> ovpn_peer_put(peer);
> if (likely(ks))
> ovpn_crypto_key_slot_put(ks);
> consume_skb(skb);
>
>
>
Either that or I can call consume_skb(skb) and set skb = NULL before
jumping to drop_nocount (haven't fully checked if possible).
I'll see which version is better.
Thanks for pointing this out!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists