[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZoXEosCwp6-WR7wb@hog>
Date: Wed, 3 Jul 2024 23:37:38 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.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
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
Powered by blists - more mailing lists