netdev - Re: [PATCH net-next v6 10/25] ovpn: implement basic TX 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: <ZtH7HWxCn0Qyk3wU@hog>
Date: Fri, 30 Aug 2024 19:02:21 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.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)

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?

> +		}
> +
> +	/* 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)

> +			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.

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

-- 
Sabrina