OK, thanks for the explanation, Mohammad. I added the following comment
to the test to explain what is being tested.
# The below commands change the LSP from "router" type in order to
ensure that
# no incorrect memory accesses occur. The test passes because there
is no crash
# in ovn-controller.
I added my Ack and pushed the change to branch-22.03, branch-22.06, and
main.
On 8/18/22 14:58, Mohammad Heib wrote:
Hi Mark,
Thank you for reviewing the patch.
actually, the test here is less interested in the type update but more
in the LSP deletion from theld->peer_ports list
i don't have a way to see if theld->n_peer_portsupdated properly from
the tests units when we change the port type, but i know that
if something wrong happens in the ovn-controller and ld->peer_ports
keeps pointing to the deleted LSP( which is what this patch
comes to fix ) the command*"check ovn-nbctl lrp-del lrp"* will cause
some invalid memory access and that will lead to a core dump in the
ovn-controller
and then the test case will fail with dump in the ovn-controller logs.
i know it is a bad way to test it like that but as i mentioned i don't
have a way to see the content of ld->peer_ports from the test :(.
see comments in the tests below.
On Thu, Aug 18, 2022 at 9:18 PM Mark Michelson <mmich...@redhat.com
<mailto:mmich...@redhat.com>> wrote:
Hi Mohammad. The code changes look good, but I'm a bit confused about
what the test case is intending to prove. It updates the type of the
LSP, but there is no check to ensure that the change was successful, as
far as I can tell.
On 8/16/22 06:56, Mohammad Heib wrote:
> The local_datapath->peer_ports list contains peers pointers
> to lsp<-->lrp ports that are supposed to be router end ports,
> those pointers are added and deleted to the
local_datapath->peer_ports
> when logical switch port of type router are added or deleted from
the database.
>
> The deletion and creation of those ports are handled very well
when the LSP type
> is a router, but if in any case, the user has changed the LSP
type from router
> port to any other LSP type the ld->peer_ports will keep pointing
to this port
> and if it was deleted it will keep pointing to invalid memory
regions and that
> could lead to invalid memory access in the ovn-controller.
>
> To solve the above issue this patch will update the
local_dataoath->peer_ports
> whenever a lport is updated.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078
<https://bugzilla.redhat.com/show_bug.cgi?id=2077078>
> Co-authored-by: Xavier Simonart <xsimo...@redhat.com
<mailto:xsimo...@redhat.com>>
> Signed-off-by: Mohammad Heib <mh...@redhat.com
<mailto:mh...@redhat.com>>
> Signed-off-by: Xavier Simonart <xsimo...@redhat.com
<mailto:xsimo...@redhat.com>>
> Signed-off-by: Mohammad Heib <mh...@redhat.com
<mailto:mh...@redhat.com>>
> ---
> controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++
> tests/ovn.at <http://ovn.at> | 37
+++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 9f5393a92..1221419a9 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -566,6 +566,39 @@ remove_related_lport(const struct
sbrec_port_binding *pb,
> }
> }
>
> +/*
> + * Update local_datapath peers when port type changed
> + * and remove irrelevant ports from this list.
> + */
> +static void
> +update_ld_peers(const struct sbrec_port_binding *pb,
> + struct hmap *local_datapaths)
> +{
> + struct local_datapath *ld =
> + get_local_datapath(local_datapaths,
pb->datapath->tunnel_key);
> +
> + if (!ld) {
> + return;
> + }
> +
> + /*
> + * This will handle cases where the pb type was explicitly
> + * changed from router type to any other port type and will
> + * remove it from the ld peers list.
> + */
> + enum en_lport_type type = get_lport_type(pb);
> + int num_peers = ld->n_peer_ports;
> + if (type != LP_PATCH) {
> + remove_local_datapath_peer_port(pb, ld, local_datapaths);
> + if (num_peers != ld->n_peer_ports) {
> + static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(1, 5);
> + VLOG_DBG_RL(&rl,
> + "removing lport %s from the ld peers list",
> + pb->logical_port);
> + }
> + }
> +}
> +
> static void
> delete_active_pb_ras_pd(const struct sbrec_port_binding *pb,
> struct shash *ras_pd_map)
> @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in
*b_ctx_in,
> return true;
> }
>
> + if (sbrec_port_binding_is_updated(pb,
SBREC_PORT_BINDING_COL_TYPE)) {
> + update_ld_peers(pb, b_ctx_out->local_datapaths);
> + }
> +
> update_active_pb_ras_pd(pb,
b_ctx_out->local_active_ports_ipv6_pd,
> "ipv6_prefix_delegation");
>
> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
<http://ovn.at>
> index c8cc8cde4..79eda21d3 100644
> --- a/tests/ovn.at <http://ovn.at>
> +++ b/tests/ovn.at <http://ovn.at>
> @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb |
grep -c "00:00:00:00:10:30") = 0])
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([router port type update and then remove])
> +ovn_start
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> + options:tx_pcap=hv1/vif1-tx.pcap \
> + options:rxq_pcap=hv1/vif1-rx.pcap \
> + ofport-request=1
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lr-add ro0
> +check ovn-nbctl lsp-add sw0 sw0-p1
> +check ovn-nbctl lsp-add sw0 lsp
> +check ovn-nbctl lsp-set-type lsp router
> +check ovn-nbctl lsp-set-options lsp router-port=lrp
> +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1
> +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1
aef0:0:0:0:0:0:0:1/64
> +check ovn-nbctl set Logical_Router_Port lrp
ipv6_ra_configs:send_periodic=true \
> + -- set Logical_Router_Port lrp
ipv6_ra_configs:address_mode=slaac \
> + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \
> + -- set Logical_Router_Port lrp
ipv6_ra_configs:max_interval=2 \
> + -- set Logical_Router_Port lrp
ipv6_ra_configs:min_interval=1
> +check ovn-nbctl lsp-set-type lsp localnet
<-------------- this will delete the lsp from the ld->peer_ports and
will delete its peer lrp from the same list
> +check ovn-nbctl --wait=hv sync
> +check ovn-nbctl lsp-del lsp
<--------- normall LSP deletion ( without this patch
theld->peer_ports will keeps pointing to deleted LSP invalid memory
area)
> +check ovn-nbctl lrp-del lrp
<--------- noremal LRP deletions
> +check ovn-nbctl --wait=hv sync
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
thanks
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
