Re: [PATCH net-next v24 16/23] ovpn: implement keepalive mechanism

[Antonio Quartulli]

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.