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

Antonio Quartulli
2024-05-28T20:08:11Z

On 28/05/2024 18:42, Sabrina Dubroca wrote:
> 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?

Yes! I have been looking for this guy all over the place in
sk_buff.h....it was just in another header :) thanks!

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

ACK

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

Thanks

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

yup, good idea

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

ACK.
I presume it's a leftover from the past, otherwise it wouldn't make much
sense.

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

Well, not at the same time, but theoretically we could re-use the ID of
a peer that is being released (i.e. still in the list but refcnt at 0)
because it won't be returned by this lookup.

This said, I truly believe it's impossible for a peer to have refcnt 0
and still being in the list:
Either
* delete on the peer was not yet called, thus peer is in the list and
the last reference wasn't yet dropped
* delete on the peer was called, thus peer cannot be in the list anymore
and refcnt may or may not be 0...

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

hm yeah, you're right. I'll do it!

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

well, in my brain a next hop can exists only when I want to reach a
certain destination. Therefore, at a low level, the terms nextop and
destination always need to go hand in hand.

This said, when implementing RPF (Reverse Path Filtering) I need to
imagine that I want to route to the source IP of the incoming packet. If
the nexthop I looked up matches the peer the packet came from, then
everything is fine.

makes sense?

[FTR I have already renamed/changed get_by_src into check_by_src,
because I don't need to truly extract a peer and get a reference, but I
only need to perform the aforementioned comparison.]

>
>> + * @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"?

typ0: "the IP"

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

it refers to @dst (the function argument). That's basically the case
where the destination is "onlink" and thus it is the nexthop (basically
the destination is the connected peer).

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

Oh, I see this just came to life in 6.10-rc1. Thanks!

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

ACK, thanks!

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

--
Antonio Quartulli
OpenVPN Inc.