| Message ID | 20240506011637.27272-16-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
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 > >
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 >> >> >
2024-05-28, 22:09:37 +0200, Antonio Quartulli wrote: > On 28/05/2024 18:42, Sabrina Dubroca wrote: > > 2024-05-06, 03:16:28 +0200, Antonio Quartulli wrote: > > > @@ -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... Ok, thanks. Let's just keep this code. > > > +/** > > > + * 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? Yeah, that's fair. > > [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.] Ok. > > > + * @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). I understand that, it's just the wording "the dst" that I'm nitpicking. s/dst/addr/ would help easily-confused people like me (for both "the dst" and my confusion with source vs destination in caller/callee), but I can live with this.
On 29/05/2024 18:42, Sabrina Dubroca wrote: > 2024-05-28, 22:09:37 +0200, Antonio Quartulli wrote: >> On 28/05/2024 18:42, Sabrina Dubroca wrote: >>> 2024-05-06, 03:16:28 +0200, Antonio Quartulli wrote: >>>> @@ -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... > > Ok, thanks. Let's just keep this code. ok > > >>>> +/** >>>> + * 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? > > Yeah, that's fair. > >> >> [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.] > > Ok. > >>>> + * @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). > > I understand that, it's just the wording "the dst" that I'm > nitpicking. s/dst/addr/ would help easily-confused people like me (for > both "the dst" and my confusion with source vs destination in > caller/callee), but I can live with this. Oh ok, now I understand your concern. I will reword this part a bit and add a comment in the caller to clarify why we invoke nexthop_from_rt4/6 passing the source address as param. Cheers, >
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c index 38a89595dade..31d7fb718b6b 100644 --- a/drivers/net/ovpn/peer.c +++ b/drivers/net/ovpn/peer.c @@ -198,6 +198,98 @@ static bool ovpn_peer_skb_to_sockaddr(struct sk_buff *skb, return true; } +/** + * ovpn_nexthop_from_skb4 - retrieve IPv4 nexthop for outgoing skb + * @skb: the outgoing packet + * + * Return: the IPv4 of the nexthop + */ +static __be32 ovpn_nexthop_from_skb4(struct sk_buff *skb) +{ + struct rtable *rt = skb_rtable(skb); + + if (rt && rt->rt_uses_gateway) + return rt->rt_gw4; + + return ip_hdr(skb)->daddr; +} + +/** + * ovpn_nexthop_from_skb6 - retrieve IPv6 nexthop for outgoing skb + * @skb: the outgoing packet + * + * Return: the IPv6 of the nexthop + */ +static struct in6_addr ovpn_nexthop_from_skb6(struct sk_buff *skb) +{ + struct rt6_info *rt = (struct rt6_info *)skb_rtable(skb); + + 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 + */ +static struct ovpn_peer *ovpn_peer_get_by_vpn_addr4(struct hlist_head *head, + __be32 *addr) +{ + struct ovpn_peer *tmp, *peer = NULL; + + rcu_read_lock(); + hlist_for_each_entry_rcu(tmp, head, hash_entry_addr4) { + if (*addr != tmp->vpn_addrs.ipv4.s_addr) + continue; + + if (!ovpn_peer_hold(tmp)) + continue; + + peer = tmp; + break; + } + rcu_read_unlock(); + + return peer; +} + +/** + * ovpn_peer_get_by_vpn_addr6 - retrieve peer by its VPN IPv6 address + * @head: list head to search + * @addr: VPN IPv6 to use as search key + * + * Return: the peer if found or NULL otherwise + */ +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; + } + + if (!ovpn_peer_hold(tmp)) + continue; + + peer = tmp; + break; + } + rcu_read_unlock(); + + return peer; +} + /** * ovpn_peer_transp_match - check if sockaddr and peer binding match * @peer: the peer to get the binding from @@ -268,14 +360,46 @@ ovpn_peer_get_by_transp_addr_p2p(struct ovpn_struct *ovpn, struct ovpn_peer *ovpn_peer_get_by_transp_addr(struct ovpn_struct *ovpn, struct sk_buff *skb) { - struct ovpn_peer *peer = NULL; + struct ovpn_peer *tmp, *peer = NULL; struct sockaddr_storage ss = { 0 }; + struct hlist_head *head; + size_t sa_len; + bool found; + u32 index; if (unlikely(!ovpn_peer_skb_to_sockaddr(skb, &ss))) return NULL; if (ovpn->mode == OVPN_MODE_P2P) - peer = ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss); + return ovpn_peer_get_by_transp_addr_p2p(ovpn, &ss); + + switch (ss.ss_family) { + case AF_INET: + sa_len = sizeof(struct sockaddr_in); + break; + case AF_INET6: + sa_len = sizeof(struct sockaddr_in6); + break; + default: + return NULL; + } + + index = ovpn_peer_index(ovpn->peers.by_transp_addr, &ss, sa_len); + head = &ovpn->peers.by_transp_addr[index]; + + rcu_read_lock(); + hlist_for_each_entry_rcu(tmp, head, hash_entry_transp_addr) { + found = ovpn_peer_transp_match(tmp, &ss); + if (!found) + 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; + + 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); + break; } return peer; } +/** + * ovpn_nexthop_from_rt4 - look up the IPv4 nexthop for the given destination + * @ovpn: the private data representing the current VPN session + * @dst: the destination to be looked up + * + * Looks up in the IPv4 system routing table the IP of the nexthop to be used + * 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 + */ +static __be32 ovpn_nexthop_from_rt4(struct ovpn_struct *ovpn, __be32 dst) +{ + struct rtable *rt; + struct flowi4 fl = { + .daddr = dst + }; + + rt = ip_route_output_flow(dev_net(ovpn->dev), &fl, NULL); + if (IS_ERR(rt)) { + net_dbg_ratelimited("%s: no route to host %pI4\n", __func__, + &dst); + /* if we end up here this packet is probably going to be + * thrown away later + */ + return dst; + } + + if (!rt->rt_uses_gateway) + goto out; + + dst = rt->rt_gw4; +out: + ip_rt_put(rt); + return dst; +} + +/** + * ovpn_nexthop_from_rt6 - look up the IPv6 nexthop for the given destination + * @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 + * 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 + */ +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); + + 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) { + 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;
In a multi-peer scenario there are a number of situations when a specific peer needs to be looked up. We may want to lookup a peer by: 1. its ID 2. its VPN destination IP 3. its transport IP/port couple For each of the above, there is a specific routing table referencing all peers for fast look up. Case 2. is a bit special in the sense that an outgoing packet may not be sent to the peer VPN IP directly, but rather to a network behind it. For this reason we first perform a nexthop lookup in the system routing table and then we use the retrieved nexthop as peer search key. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/peer.c | 285 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 281 insertions(+), 4 deletions(-)