netdev - Re: [PATCH net-next v5 11/25] ovpn: implement basic RX path (UDP)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <937bd3bf-e6c6-4b10-905f-932d66c7a602@openvpn.net>
Date: Tue, 9 Jul 2024 00:09:34 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, ryazanov.s.a@...il.com,
 pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 11/25] ovpn: implement basic RX path (UDP)

Hi,

On 08/07/2024 18:11, Sabrina Dubroca wrote:
> 2024-06-27, 15:08:29 +0200, Antonio Quartulli wrote:
>> +static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
> [...]
>> +	/* cause packet to be "received" by the interface */
>> +	if (likely(gro_cells_receive(&peer->ovpn->gro_cells,
>> +				     skb) == NET_RX_SUCCESS))
>> +		/* update RX stats with the size of decrypted packet */
>> +		dev_sw_netstats_rx_add(peer->ovpn->dev, skb->len);
>> +	else
>> +		dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> 
> Not needed AFAICT, gro_cells_receive already does
> dev_core_stats_rx_dropped_inc(skb->dev) when it drops the packet.

You're right! This should be removed.

> 
>> +}
>> +
>> +static void ovpn_decrypt_post(struct sk_buff *skb, int ret)
>> +{
>> +	struct ovpn_peer *peer = ovpn_skb_cb(skb)->peer;
>> +
>> +	if (unlikely(ret < 0))
>> +		goto drop;
>> +
>> +	ovpn_netdev_write(peer, skb);
>> +	/* skb is passed to upper layer - don't free it */
>> +	skb = NULL;
>> +drop:
> 
> I really find this "common" return code confusing. The only thing the
> two cases have in common is dropping the peer reference.

I believe it's just a matter of perspective: I read it as "exit path" 
and if something went wrong I just do some extra actions.

However, I am perfectly fine splitting success/failure paths if the 
common code is little like in this case.

Thanks!

> 
>> +	if (unlikely(skb))
>> +		dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
>> +	kfree_skb(skb);
>> +	ovpn_peer_put(peer);
>> +}
> 

-- 
Antonio Quartulli
OpenVPN Inc.