netdev - Re: [PATCH net-next v5 08/25] ovpn: introduce the ovpn

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: <69443dab-1eaa-4754-8973-750f653ef716@openvpn.net>
Date: Thu, 4 Jul 2024 00:16:54 +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 08/25] ovpn: introduce the ovpn_peer object

On 03/07/2024 23:37, Sabrina Dubroca wrote:
> 2024-06-27, 15:08:26 +0200, Antonio Quartulli wrote:
>> +/**
>> + * struct ovpn_sockaddr - basic transport layer address
>> + * @in4: IPv4 address
>> + * @in6: IPv6 address
>> + */
>> +struct ovpn_sockaddr {
>> +	union {
>> +		struct sockaddr_in in4;
>> +		struct sockaddr_in6 in6;
>> +	};
>> +};
> 
> nit: wrapping the anonymous union in a struct that contains nothing
> else is not that useful.

yeah, I guess I can just turn ovpn_sockaddr in a union.

> 
> 
>> +/**
>> + * struct ovpn_bind - remote peer binding
>> + * @sa: the remote peer sockaddress
>> + * @local: local endpoint used to talk to the peer
>> + * @local.ipv4: local IPv4 used to talk to the peer
>> + * @local.ipv6: local IPv6 used to talk to the peer
>> + * @rcu: used to schedule RCU cleanup job
>> + */
>> +struct ovpn_bind {
>> +	struct ovpn_sockaddr sa;  /* remote sockaddr */
> 
> nit: then maybe call it "peer" or "remote" instead of sa?

yap, makes sense. I will call it "remote".

> 
>> +	union {
>> +		struct in_addr ipv4;
>> +		struct in6_addr ipv6;
>> +	} local;
>> +
>> +	struct rcu_head rcu;
>> +};
>> +
> 
> [...]
>> +struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id)
>> +{
>> +	struct ovpn_peer *peer;
>> +	int ret;
>> +
>> +	/* alloc and init peer object */
>> +	peer = kzalloc(sizeof(*peer), GFP_KERNEL);
>> +	if (!peer)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	peer->id = id;
>> +	peer->halt = false;
>> +	peer->ovpn = ovpn;
>> +
>> +	peer->vpn_addrs.ipv4.s_addr = htonl(INADDR_ANY);
>> +	peer->vpn_addrs.ipv6 = in6addr_any;
>> +
>> +	RCU_INIT_POINTER(peer->bind, NULL);
>> +	spin_lock_init(&peer->lock);
>> +	kref_init(&peer->refcount);
>> +
>> +	ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
>> +	if (ret < 0) {
>> +		netdev_err(ovpn->dev, "%s: cannot initialize dst cache\n",
>> +			   __func__);
>> +		kfree(peer);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	netdev_hold(ovpn->dev, NULL, GFP_KERNEL);
> 
> It would be good to add a tracker to help debug refcount issues.

Ok, will do!

> 
> 
>> +
>> +	return peer;
>> +}
>> +
>> +#define ovpn_peer_index(_tbl, _key, _key_len)		\
>> +	(jhash(_key, _key_len, 0) % HASH_SIZE(_tbl))	\
> 
> nit: not used in this patch, and even removed by patch 16 as you
> convert from index to buckets (that conversion should be squashed into
> patch 15)

You're correct. Will merge all these pieces in patch 15.

> 
>> +/**
>> + * ovpn_peer_transp_match - check if sockaddr and peer binding match
>> + * @peer: the peer to get the binding from
>> + * @ss: the sockaddr to match
>> + *
>> + * Return: true if sockaddr and binding match or false otherwise
>> + */
>> +static bool ovpn_peer_transp_match(const struct ovpn_peer *peer,
>> +				   const struct sockaddr_storage *ss)
>> +{
> 
> AFAICT ovpn_peer_transp_match is only called with ss from
> ovpn_peer_skb_to_sockaddr, so it's pretty much ovpn_bind_skb_src_match
> but using peer->bind. You can probably avoid the code duplication
> (ovpn_peer_transp_match and ovpn_bind_skb_src_match are very similar).
> 

mhh it is not called in ovpn_peer_skb_to_sockaddr, but I guess your 
comment still applies: ovpn_peer_transp_match and 
ovpn_bind_skb_src_match are very similar.

However in one we have a sockaddr_storage while in the other we have an 
skb. How do we combine the two?
The only way I see is to create an ss out of the skb and then always use
ovpn_peer_transp_match. Is this what you were alluding to?


Thanks!

-- 
Antonio Quartulli
OpenVPN Inc.