Re: [PATCH net-next v3 15/24] ovpn: implement peer lookup logic

Sabrina Dubroca
2024-05-28T16:42:24Z

2024-05-06, 03:16:28 +0200, Antonio Quartulli wrote:
> +static struct in6_addr ovpn_nexthop_from_skb6(struct sk_buff *skb)
> +{
> + struct rt6_info *rt = (struct rt6_info *)skb_rtable(skb);

skb_rt6_info?

> +
> + if (!rt || !(rt->rt6i_flags & RTF_GATEWAY))
> + return ipv6_hdr(skb)->daddr;
> +
> + return rt->rt6i_gateway;
> +}
> +
> +/**
> + * ovpn_peer_get_by_vpn_addr4 - retrieve peer by its VPN IPv4 address
> + * @head: list head to search
> + * @addr: VPN IPv4 to use as search key
> + *
> + * Return: the peer if found or NULL otherwise

The doc for all those ovpn_peer_get_* functions could indicate that on
success, a reference on the peer is held.

[...]
> +static struct ovpn_peer *ovpn_peer_get_by_vpn_addr6(struct hlist_head *head,
> + struct in6_addr *addr)
> +{
> + struct ovpn_peer *tmp, *peer = NULL;
> + int i;
> +
> + rcu_read_lock();
> + hlist_for_each_entry_rcu(tmp, head, hash_entry_addr6) {
> + for (i = 0; i < 4; i++) {
> + if (addr->s6_addr32[i] !=
> + tmp->vpn_addrs.ipv6.s6_addr32[i])
> + continue;
> + }

ipv6_addr_equal

[...]
> + default:
> + return NULL;
> + }
> +
> + index = ovpn_peer_index(ovpn->peers.by_transp_addr, &ss, sa_len);
> + head = &ovpn->peers.by_transp_addr[index];

Maybe worth adding a get_bucket helper (with a better name :)) instead
of ovpn_peer_index, since all uses of ovpn_peer_index are followed by
a "head = TBL[index]" (or direct use in some hlist iterator), but the
index itself is not used later on, only the bucket.

> +
> + rcu_read_lock();
> + hlist_for_each_entry_rcu(tmp, head, hash_entry_transp_addr) {
> + found = ovpn_peer_transp_match(tmp, &ss);
> + if (!found)

nit: call ovpn_peer_transp_match directly and drop the found variable

> + continue;
> +
> + if (!ovpn_peer_hold(tmp))
> + continue;
> +
> + peer = tmp;
> + break;
> + }
> + rcu_read_unlock();
>
> return peer;
> }
> @@ -303,10 +427,28 @@ static struct ovpn_peer *ovpn_peer_get_by_id_p2p(struct ovpn_struct *ovpn,
>
> struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id)
> {
> - struct ovpn_peer *peer = NULL;
> + struct ovpn_peer *tmp, *peer = NULL;
> + struct hlist_head *head;
> + u32 index;
>
> if (ovpn->mode == OVPN_MODE_P2P)
> - peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id);
> + return ovpn_peer_get_by_id_p2p(ovpn, peer_id);
> +
> + index = ovpn_peer_index(ovpn->peers.by_id, &peer_id, sizeof(peer_id));
> + head = &ovpn->peers.by_id[index];
> +
> + rcu_read_lock();
> + hlist_for_each_entry_rcu(tmp, head, hash_entry_id) {
> + if (tmp->id != peer_id)
> + continue;
> +
> + if (!ovpn_peer_hold(tmp))
> + continue;

Can there ever be multiple peers with the same id? (ie, is it worth
continuing the loop if this fails? the same question probably applies
to ovpn_peer_get_by_transp_addr as well)

> + peer = tmp;
> + break;
> + }
> + rcu_read_unlock();
>
> return peer;
> }
> @@ -328,6 +470,11 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn,
> struct sk_buff *skb)
> {
> struct ovpn_peer *tmp, *peer = NULL;
> + struct hlist_head *head;
> + sa_family_t sa_fam;
> + struct in6_addr addr6;
> + __be32 addr4;
> + u32 index;
>
> /* in P2P mode, no matter the destination, packets are always sent to
> * the single peer listening on the other side
> @@ -338,15 +485,123 @@ struct ovpn_peer *ovpn_peer_get_by_dst(struct ovpn_struct *ovpn,
> if (likely(tmp && ovpn_peer_hold(tmp)))
> peer = tmp;
> rcu_read_unlock();
> + return peer;
> + }
> +
> + sa_fam = skb_protocol_to_family(skb);
> +
> + switch (sa_fam) {
> + case AF_INET:
> + addr4 = ovpn_nexthop_from_skb4(skb);
> + index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr4,
> + sizeof(addr4));
> + head = &ovpn->peers.by_vpn_addr[index];
> +
> + peer = ovpn_peer_get_by_vpn_addr4(head, &addr4);
> + break;
> + case AF_INET6:
> + addr6 = ovpn_nexthop_from_skb6(skb);
> + index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr6,
> + sizeof(addr6));
> + head = &ovpn->peers.by_vpn_addr[index];
> +
> + peer = ovpn_peer_get_by_vpn_addr6(head, &addr6);

The index -> head -> peer code is identical in get_by_dst and
get_by_src, it could be stuffed into ovpn_peer_get_by_vpn_addr{4,6}.

> + break;
> }
>
> return peer;
> }

[snip the _rt4 variant, comments apply to both]
> +/**
> + * ovpn_nexthop_from_rt6 - look up the IPv6 nexthop for the given destination

I'm a bit confused by this talk about "destination" when those two
functions are then used with the source address from the packet, from
a function called "get_by_src".

> + * @ovpn: the private data representing the current VPN session
> + * @dst: the destination to be looked up
> + *
> + * Looks up in the IPv6 system routing table the IO of the nexthop to be used

"the IO"?

> + * to reach the destination passed as argument. IF no nexthop can be found, the
> + * destination itself is returned as it probably has to be used as nexthop.
> + *
> + * Return: the IP of the next hop if found or the dst itself otherwise

"the dst" tends to refer to a dst_entry, maybe "or @dst otherwise"?
(though I'm not sure that's valid kdoc)

(also for ovpn_nexthop_from_rt4)

> + */
> +static struct in6_addr ovpn_nexthop_from_rt6(struct ovpn_struct *ovpn,
> + struct in6_addr dst)
> +{
> +#if IS_ENABLED(CONFIG_IPV6)
> + struct dst_entry *entry;
> + struct rt6_info *rt;
> + struct flowi6 fl = {
> + .daddr = dst,
> + };
> +
> + entry = ipv6_stub->ipv6_dst_lookup_flow(dev_net(ovpn->dev), NULL, &fl,
> + NULL);
> + if (IS_ERR(entry)) {
> + net_dbg_ratelimited("%s: no route to host %pI6c\n", __func__,
> + &dst);
> + /* if we end up here this packet is probably going to be
> + * thrown away later
> + */
> + return dst;
> + }
> +
> + rt = container_of(entry, struct rt6_info, dst);

dst_rt6_info(entry)

> +
> + if (!(rt->rt6i_flags & RTF_GATEWAY))
> + goto out;
> +
> + dst = rt->rt6i_gateway;
> +out:
> + dst_release((struct dst_entry *)rt);
> +#endif
> + return dst;
> +}
> +
> struct ovpn_peer *ovpn_peer_get_by_src(struct ovpn_struct *ovpn,
> struct sk_buff *skb)
> {
> struct ovpn_peer *tmp, *peer = NULL;
> + struct hlist_head *head;
> + sa_family_t sa_fam;
> + struct in6_addr addr6;
> + __be32 addr4;
> + u32 index;
>
> /* in P2P mode, no matter the destination, packets are always sent to
> * the single peer listening on the other side
> @@ -357,6 +612,28 @@ struct ovpn_peer *ovpn_peer_get_by_src(struct ovpn_struct *ovpn,
> if (likely(tmp && ovpn_peer_hold(tmp)))
> peer = tmp;
> rcu_read_unlock();
> + return peer;
> + }
> +
> + sa_fam = skb_protocol_to_family(skb);
> +
> + switch (sa_fam) {

nit:
switch (skb_protocol_to_family(skb))
seems a bit more readable to me (also in ovpn_peer_get_by_dst) - and
saves you from reverse xmas tree complaints (sa_fam should have been
after addr6)

> + case AF_INET:
> + addr4 = ovpn_nexthop_from_rt4(ovpn, ip_hdr(skb)->saddr);
> + index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr4,
> + sizeof(addr4));
> + head = &ovpn->peers.by_vpn_addr[index];
> +
> + peer = ovpn_peer_get_by_vpn_addr4(head, &addr4);
> + break;
> + case AF_INET6:
> + addr6 = ovpn_nexthop_from_rt6(ovpn, ipv6_hdr(skb)->saddr);
> + index = ovpn_peer_index(ovpn->peers.by_vpn_addr, &addr6,
> + sizeof(addr6));
> + head = &ovpn->peers.by_vpn_addr[index];
> +
> + peer = ovpn_peer_get_by_vpn_addr6(head, &addr6);
> + break;
> }
>
> return peer;
> --
> 2.43.2
>
>

--
Sabrina