Message-ID: <4f241c3b-8256-494c-bb84-178e0ee12b72@openvpn.net>
Date: Mon, 2 Sep 2024 14:03:57 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 10/25] ovpn: implement basic TX path (UDP)



On 30/08/2024 19:02, Sabrina Dubroca wrote:
> Hi Antonio,
> 
> Thanks for the updated patchset. I'm going through it again.
> 
> 2024-08-27, 14:07:50 +0200, Antonio Quartulli wrote:
>> +/* send skb to connected peer, if any */
>> +static void ovpn_send(struct ovpn_struct *ovpn, struct sk_buff *skb,
>> +		      struct ovpn_peer *peer)
>> +{
>> +	struct sk_buff *curr, *next;
>> +
>> +	if (likely(!peer))
>> +		/* retrieve peer serving the destination IP of this packet */
>> +		peer = ovpn_peer_get_by_dst(ovpn, skb);
>> +	if (unlikely(!peer)) {
>> +		net_dbg_ratelimited("%s: no peer to send data to\n",
>> +				    ovpn->dev->name);
>> +		dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +		goto drop;
>> +	}
>> +
>> +	/* this might be a GSO-segmented skb list: process each skb
>> +	 * independently
>> +	 */
>> +	skb_list_walk_safe(skb, curr, next)
>> +		if (unlikely(!ovpn_encrypt_one(peer, curr))) {
>> +			dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +			kfree_skb(curr);
> 
> Is this a bit inconsistent with ovpn_net_xmit's behavior? There we
> drop the full list if we fail one skb_share_check, and here we only
> drop the single packet that failed and handle the rest? Or am I
> misreading this?

You're right, it's inconsistent.

In ovpn_send() each call to ovpn_encrypt_one() will result in the skb 
being sent, therefore, if we wanted, we could free only skbs we haven't 
sent yet.

Maybe it makes sense to always try sending the rest and assume that the 
upper layer will deal with the missing data.

I'll adjust ovpn_net_xmit() to drop only the failing skb and move on.

> 
>> +		}
>> +
>> +	/* skb passed over, no need to free */
>> +	skb = NULL;
>> +drop:
>> +	if (likely(peer))
>> +		ovpn_peer_put(peer);
>> +	kfree_skb_list(skb);
>> +}
>>   
>>   /* Send user data to the network
>>    */
>>   netdev_tx_t ovpn_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>> +	struct ovpn_struct *ovpn = netdev_priv(dev);
>> +	struct sk_buff *segments, *tmp, *curr, *next;
>> +	struct sk_buff_head skb_list;
>> +	__be16 proto;
>> +	int ret;
>> +
>> +	/* reset netfilter state */
>> +	nf_reset_ct(skb);
>> +
>> +	/* verify IP header size in network packet */
>> +	proto = ovpn_ip_check_protocol(skb);
>> +	if (unlikely(!proto || skb->protocol != proto)) {
>> +		net_err_ratelimited("%s: dropping malformed payload packet\n",
>> +				    dev->name);
>> +		dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +		goto drop;
>> +	}
>> +
>> +	if (skb_is_gso(skb)) {
>> +		segments = skb_gso_segment(skb, 0);
>> +		if (IS_ERR(segments)) {
>> +			ret = PTR_ERR(segments);
>> +			net_err_ratelimited("%s: cannot segment packet: %d\n",
>> +					    dev->name, ret);
>> +			dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +			goto drop;
>> +		}
>> +
>> +		consume_skb(skb);
>> +		skb = segments;
>> +	}
>> +
>> +	/* from this moment on, "skb" might be a list */
>> +
>> +	__skb_queue_head_init(&skb_list);
>> +	skb_list_walk_safe(skb, curr, next) {
>> +		skb_mark_not_on_list(curr);
>> +
>> +		tmp = skb_share_check(curr, GFP_ATOMIC);
>> +		if (unlikely(!tmp)) {
>> +			kfree_skb_list(next);
> 
> Those don't get counted as dropped, but the ones we've already handled
> (and put on skb_list) will be counted as dev_core_stats_tx_dropped_inc?
> (it probably doesn't matter that much, since if we'd dropped before/at
> skb_gso_segment we'd only count one drop)

Once I change this part, I'll ensure we count each single dropped packet.
The downside is that any failure before skb_gso_segment() is counted as 
1 drop, while anything later than that is counter per-segment.
I don't think we can do anything about it though.

> 
>> +			net_err_ratelimited("%s: skb_share_check failed\n",
>> +					    dev->name);
>> +			goto drop_list;
>> +		}
>> +
>> +		__skb_queue_tail(&skb_list, tmp);
>> +	}
>> +	skb_list.prev->next = NULL;
>> +
>> +	ovpn_send(ovpn, skb_list.next, NULL);
>> +
>> +	return NETDEV_TX_OK;
>> +
>> +drop_list:
>> +	skb_queue_walk_safe(&skb_list, curr, next) {
>> +		dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +		kfree_skb(curr);
>> +	}
>> +drop:
>>   	skb_tx_error(skb);
>> -	kfree_skb(skb);
>> +	kfree_skb_list(skb);
>>   	return NET_XMIT_DROP;
>>   }
> 
> 
> [...]
>> +void ovpn_udp_send_skb(struct ovpn_struct *ovpn, struct ovpn_peer *peer,
>> +		       struct sk_buff *skb)
>> +{
>> +	struct ovpn_bind *bind;
>> +	struct socket *sock;
>> +	int ret = -1;
>> +
>> +	skb->dev = ovpn->dev;
>> +	/* no checksum performed at this layer */
>> +	skb->ip_summed = CHECKSUM_NONE;
>> +
>> +	/* get socket info */
>> +	sock = peer->sock->sock;
>> +	if (unlikely(!sock)) {
>> +		net_warn_ratelimited("%s: no sock for remote peer\n", __func__);
>> +		goto out;
>> +	}
>> +
>> +	rcu_read_lock();
>> +	/* get binding */
>> +	bind = rcu_dereference(peer->bind);
>> +	if (unlikely(!bind)) {
>> +		net_warn_ratelimited("%s: no bind for remote peer\n", __func__);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/* crypto layer -> transport (UDP) */
>> +	ret = ovpn_udp_output(ovpn, bind, &peer->dst_cache, sock->sk, skb);
>> +
>> +out_unlock:
>> +	rcu_read_unlock();
>> +out:
>> +	if (unlikely(ret < 0)) {
>> +		dev_core_stats_tx_dropped_inc(ovpn->dev);
>> +		kfree_skb(skb);
>> +		return;
>> +	}
>> +
>> +	dev_sw_netstats_tx_add(ovpn->dev, 1, skb->len);
> 
> I don't think it's safe to access skb->len after calling
> udp_tunnel(6)_xmit_skb.

Absolutely right! Thanks!

> 
> For example, vxlan_xmit_one (drivers/net/vxlan/vxlan_core.c) has a
> similar counter and saves skb->len into pkt_len.

Yap, will do the same.

Cheers,

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists