On 4/2/25 6:46 AM, Dumitru Ceara wrote:
> If a router port has more than 16 networks configured we should set the
> flags.network_id to 0 for network whose index is greater than 16.
>
> This issue has been reported by coverity. Trimmed report:
>
> *** CID 459645: Code maintainability issues (UNUSED_VALUE)
> /northd/northd.c: 15062 in build_lrouter_network_id_flows()
> 15056 "flags.network_id = 0.", op->json_key,
> 15057 OVN_MAX_NETWORK_ID + 1,
> 15058 op->lrp_networks.ipv6_addrs[i].addr_s,
> 15059 op->lrp_networks.ipv6_addrs[i].plen);
> 15060 network_id = 0;
> 15061 } else {
> >>> CID 459645: Code maintainability issues (UNUSED_VALUE)
> >>> Assigning value from "i" to "network_id" here, but that stored
> >> value is overwritten before it can be used.
> 15062 network_id = i;
> 15063 }
> 15064
> 15065 ds_clear(match);
> 15066 ds_clear(actions);
> 15067
>
> Fixes: af6e83707568 ("northd: Use next-hop network for SNAT when
> lb_force_snat_ip=router_ip.")
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> northd/northd.c | 3 ++-
> tests/ovn-northd.at | 35 ++++++++++++++++++++++++++++++++---
> 2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 471d9969bf..00bb738b3c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -15070,7 +15070,8 @@ build_lrouter_network_id_flows(
> op->lrp_networks.ipv6_addrs[i].addr_s,
> op->lrp_networks.ipv6_addrs[i].plen);
>
> - ds_put_format(actions, "flags.network_id = %"PRIuSIZE"; next;",
> i);
> + ds_put_format(actions, "flags.network_id = %"PRIuSIZE"; next;",
> + network_id);
>
> ovn_lflow_add(lflows, op->od, S_ROUTER_IN_NETWORK_ID, 110,
> ds_cstr(match), ds_cstr(actions), lflow_ref);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 95993e454c..8652e5574b 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -16781,7 +16781,9 @@ check ovn-nbctl lrp-add lr lrp-server
> 02:00:00:00:00:03 1.1.2.1/24 7.7.7.1/24 \
> 8.8.8.1/24 1.2.1.1/24 1.2.2.1/24 3.3.3.1/24 4.4.4.1/24
> 5.5.5.1/24 \
> 6.6.6.1/24 6.2.1.1/24 6.3.1.1/24 6.4.1.1/24 6.5.1.1/24
> 6.6.1.1/24 \
> 6.7.1.1/24 6.8.1.1/24 6.9.1.1/24
> 7.2.1.1/24 \
> - ff01::02 ff01::03 ff01::06
> + ff01::02 ff01::03 ff01::04 ff01::05 ff01::06
> ff01::07 \
> + ff01::08 ff01::09 ff01::0a ff01::0b ff01::0c
> ff01::0d \
> + ff01::0e ff01::0f ff01::a1 ff01::a2 ff01::a3
> check ovn-nbctl ls-add ls-client
> check ovn-nbctl ls-add ls-server
> check ovn-nbctl lsp-add ls-client lsp-client-router
> @@ -16827,7 +16829,21 @@ AT_CHECK([grep -E flags.network_id lrflows |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip4 && reg0 == 8.8.8.1/24), action=(flags.network_id = 0;
> next;)
> table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::2/128), action=(flags.network_id = 0;
> next;)
> table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::3/128), action=(flags.network_id = 1;
> next;)
> - table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::6/128), action=(flags.network_id = 2;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::4/128), action=(flags.network_id = 2;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::5/128), action=(flags.network_id = 3;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::6/128), action=(flags.network_id = 4;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::7/128), action=(flags.network_id = 5;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::8/128), action=(flags.network_id = 6;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::9/128), action=(flags.network_id = 7;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::a/128), action=(flags.network_id = 8;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::a1/128), action=(flags.network_id =
> 14; next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::a2/128), action=(flags.network_id =
> 15; next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::a3/128), action=(flags.network_id = 0;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::b/128), action=(flags.network_id = 9;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::c/128), action=(flags.network_id = 10;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::d/128), action=(flags.network_id = 11;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::e/128), action=(flags.network_id = 12;
> next;)
> + table=??(lr_in_network_id ), priority=110 , match=(outport ==
> "lrp-server" && ip6 && xxreg0 == ff01::f/128), action=(flags.network_id = 13;
> next;)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 &&
> outport == "lrp-client"), action=(ct_snat(1.1.1.1);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(1.1.2.1);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 0 && ip6 &&
> outport == "lrp-client"), action=(ct_snat(ff01::1);)
> @@ -16835,20 +16851,33 @@ AT_CHECK([grep -E flags.network_id lrflows |
> ovn_strip_lflows], [0], [dnl
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 1 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(1.2.1.1);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 1 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::3);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 10 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.6.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 10 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::c);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 11 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.6.6.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 11 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::d);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 12 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.7.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 12 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::e);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 13 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.8.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 13 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::f);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 14 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.9.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 14 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::a1);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 15 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(7.2.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 15 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::a2);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 2 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(1.2.2.1);)
> - table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 2 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::6);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 2 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::4);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 3 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(3.3.3.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 3 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::5);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 4 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(4.4.4.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 4 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::6);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 5 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(5.5.5.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 5 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::7);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 6 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.2.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 6 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::8);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 7 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.3.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 7 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::9);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 8 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.4.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 8 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::a);)
> table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 9 && ip4 &&
> outport == "lrp-server"), action=(ct_snat(6.5.1.1);)
> + table=??(lr_out_snat ), priority=110 ,
> match=(flags.force_snat_for_lb == 1 && flags.network_id == 9 && ip6 &&
> outport == "lrp-server"), action=(ct_snat(ff01::b);)
> ])
> AT_CLEANUP
> ])
This corrects the IPv6 behavior and makes it consistent with that of
IPv4. Looks good to me. Thanks!
Acked-by: Rosemarie O'Riorden <rosema...@redhat.com>
--
Rosemarie O'Riorden
rosema...@redhat.com
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
