On 2/6/26 6:05 PM, Mike Pattrick via dev wrote:
> The source address parameter in ovs_router_lookup is both an input and
> an output. However, the interface was complex. The caller could
> inadvertently set a search over v4 or v6 rules based on if the source
> address was initialized to in6addr_any or in6addr_v4mapped_any. The
> lookup function even used these two values interchangeably.
>
> This patch uses dst address to determine if the lookup is v4 or v6, and
> considers both v6_any and v4mapped_any to be the null value equally. Now
> if the caller just wants src as output, they can initialize it to v6_any
> and lookup will still work correctly.
>
> Fixes: dc14e92bcc25 ("route-table: Introduce multi-table route lookup.")
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
Hi, Mike. Thanks for the fix! See some comments inline.
> lib/ovs-router.c | 47 ++++++++++++++++++++++++---------------------
> lib/packets.h | 5 +++++
> tests/ovs-router.at | 26 +++++++++++++++++++++++++
> 3 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index c11a52764..3ac421915 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -175,28 +175,31 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> struct in6_addr *src, struct in6_addr *gw)
I think, most of the issues in this code stem from the 'src' being both
the input and the output argument. Though it is never actually both.
So, I'd suggest we split it into a const input argument and the new
output argument, e.g. 'src_out' or 'route_src'. This will allow us to
get rid of all the things like 'from_src' and 'is_any'. And we'll not
need to compare the value with all the is_set() functions and just check
for NULL instead, in which case we'd also not need to change the nesting
of the if statements in this patch for the source lookup.
> {
> struct flow flow = {.ipv6_dst = *ip6_dst, .pkt_mark = mark};
> + bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(ip6_dst);
It's probably worth adding an assertion that src and dst have the
same family.
> const struct in6_addr *from_src = src;
> const struct cls_rule *cr = NULL;
> struct router_rule *rule;
> + bool is_any = true;
>
> - if (src && ipv6_addr_is_set(src)) {
> - struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
> - struct classifier *cls_local = cls_find(CLS_LOCAL);
> - const struct cls_rule *cr_src;
> + if (src) {
> + if (ipv4_addr_is_set(src) || ipv6_addr_is_set(src)) {
> + struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
> + struct classifier *cls_local = cls_find(CLS_LOCAL);
> + const struct cls_rule *cr_src;
>
> - if (!cls_local) {
> - return false;
> - }
> + if (!cls_local) {
> + return false;
> + }
>
> - cr_src = classifier_lookup(cls_local, OVS_VERSION_MAX, &flow_src,
> - NULL, NULL);
> - if (!cr_src) {
> - return false;
> + cr_src = classifier_lookup(cls_local, OVS_VERSION_MAX, &flow_src,
> + NULL, NULL);
> + if (!cr_src) {
> + return false;
> + }
> + is_any = false;
> }
> - }
> -
> - if (!from_src) {
> - if (IN6_IS_ADDR_V4MAPPED(ip6_dst)) {
> + } else {
This feels a little off that we're checking 'src', but setting 'from_src'
in this branch. But that wouldn't be a problem if we didn't have 'from_src'.
> + if (is_ipv4) {
> from_src = &in6addr_v4mapped_any;
> } else {
> from_src = &in6addr_any;
> @@ -207,8 +210,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> uint8_t plen = rule->ipv4 ? rule->src_prefix + 96 : rule->src_prefix;
> bool matched;
>
> - if ((IN6_IS_ADDR_V4MAPPED(from_src) && !rule->ipv4) ||
> - (!IN6_IS_ADDR_V4MAPPED(from_src) && rule->ipv4)) {
note: I missed the fact that IN6_IS_ADDR_V4MAPPED() actually returns a proper
boolean when I suggested this bit of code, so we could just directly compare
the IN6_IS_ADDR_V4MAPPED(from_src) and the rule->ipv4 here without calling it
twice.
> + if (is_ipv4 != rule->ipv4) {
> continue;
> }
>
> @@ -231,12 +233,13 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> if (cr) {
> struct ovs_router_entry *p = ovs_router_entry_cast(cr);
> /* Avoid matching mapped IPv4 of a packet against default
> IPv6
> - * route entry. Either packet dst is IPv6 or both packet and
> - * route entry dst are mapped IPv4.
> + * route entry. IPv6 status of the packet dst and route dst
> + * must be the same.
nit: Feels like the second sentence is redundant here.
> */
> - if (!IN6_IS_ADDR_V4MAPPED(ip6_dst) ||
> - IN6_IS_ADDR_V4MAPPED(&p->nw_addr)) {
> + if (is_ipv4 == IN6_IS_ADDR_V4MAPPED(&p->nw_addr)) {
> break;
> + } else {
> + cr = NULL;
> }
The 'else' branch is not needed here. But we may want to add a comment
on why we're setting cr to NULL.
Alternatively, we may remove most of the nesting from this loop, so we
don't need to clear the value. If we change all the conditions inside
to if (!something) { continue; }, it may be much easier to follow the
logic. e.g.:
PVECTOR_FOR_EACH (rule, &rules) {
struct ovs_router_entry *p;
bool matched;
if (is_ipv4 != rule->ipv4) {
continue;
}
matched = ...
if (!matched) {
continue;
}
cls = cls_find();
if (!cls) {
continue;
}
And so on.
Then we can also move the body of the final 'if (cr)' into the loop and
make cls a function-scoped variable and share it between the source lookup
and the rule lookup loop.
WDYT?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
