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

[Sabrina Dubroca <sd@queasysnail.net>]

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.


> +/**
> + * 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?

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


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

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

-- 
Sabrina