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,