| Message ID | 20240109183234.3926405-1-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] pinctrl: Directly retrieve desired port_binding MAC. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Jan 9, 2024 at 10:32 AM Mark Michelson <mmichels@redhat.com> wrote: > > A static analyzer determined that if pb->n_mac was 0, then the c_addrs > lport_addresses struct would never be initialized. We would then use > and attempt to free uninitialized memory. > > In reality, pb->n_mac will always be 1. This is because the port binding is a > representation of a northbound logical router port. Logical router ports do > not contain an array of MACs like the southbound port bindings do. Instead, > they have a single MAC string that is always guaranteed to be non-NULL. This > string is copied into the port binding's MAC array. Therefore, a southbound > port binding that comes from a logical router port will always have n_mac set > to 1. > > How do we know this is a logical router port? The ports iterated over in this > function must have IPv6 prefix delegation configured on them. Only northbound > logical router ports have this option available. > > To silence the static analyzer, this change directly retrieves pb->mac[0] > instead of iterating over the pb->mac array. It also adds an assertion that > pb->n_mac == 1. This arguably makes the code's intent more clear as > well. > > Reported-at: https://issues.redhat.com/browse/FDP-224 > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > controller/pinctrl.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 12055a675..d5957ad69 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1286,11 +1286,15 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn *ovnsb_idl_txn, > continue; > } > > + /* To reach this point, the port binding must be a logical router > + * port. LRPs are configured with a single MAC that is always non-NULL. > + * Therefore, we can always safely extract pb->mac[0] since it will be > + * non-NULL > + */ > + ovs_assert(pb->n_mac == 1); Thanks Mark for the fix. I think we shouldn't use assert here, since the DB status is not controlled by ovn-controller and it is not good to crash the ovn-controller when the mac column is cleared by an external tool for whatever reason. Assertion should be used when we are 100% sure about the assumption from developer's point of view except when there is a bug in the current component. In this case I think it is better to print an error and skip, if not doing more complex error handling. Regards, Han > struct lport_addresses c_addrs; > - for (size_t j = 0; j < pb->n_mac; j++) { > - if (extract_lsp_addresses(pb->mac[j], &c_addrs)) { > - break; > - } > + if (!extract_lsp_addresses(pb->mac[0], &c_addrs)) { > + continue; > } > > pfd = shash_find_data(&ipv6_prefixd, pb->logical_port); > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1/9/24 19:16, Han Zhou wrote: > > > On Tue, Jan 9, 2024 at 10:32 AM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > > > A static analyzer determined that if pb->n_mac was 0, then the c_addrs > > lport_addresses struct would never be initialized. We would then use > > and attempt to free uninitialized memory. > > > > In reality, pb->n_mac will always be 1. This is because the port > binding is a > > representation of a northbound logical router port. Logical router > ports do > > not contain an array of MACs like the southbound port bindings do. > Instead, > > they have a single MAC string that is always guaranteed to be > non-NULL. This > > string is copied into the port binding's MAC array. Therefore, a > southbound > > port binding that comes from a logical router port will always have > n_mac set > > to 1. > > > > How do we know this is a logical router port? The ports iterated over > in this > > function must have IPv6 prefix delegation configured on them. Only > northbound > > logical router ports have this option available. > > > > To silence the static analyzer, this change directly retrieves pb->mac[0] > > instead of iterating over the pb->mac array. It also adds an > assertion that > > pb->n_mac == 1. This arguably makes the code's intent more clear as > > well. > > > > Reported-at: https://issues.redhat.com/browse/FDP-224 > <https://issues.redhat.com/browse/FDP-224> > > > > Signed-off-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > --- > > controller/pinctrl.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 12055a675..d5957ad69 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -1286,11 +1286,15 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > continue; > > } > > > > + /* To reach this point, the port binding must be a logical > router > > + * port. LRPs are configured with a single MAC that is > always non-NULL. > > + * Therefore, we can always safely extract pb->mac[0] since > it will be > > + * non-NULL > > + */ > > + ovs_assert(pb->n_mac == 1); > > Thanks Mark for the fix. I think we shouldn't use assert here, since the > DB status is not controlled by ovn-controller and it is not good to > crash the ovn-controller when the mac column is cleared by an external > tool for whatever reason. Assertion should be used when we are 100% sure > about the assumption from developer's point of view except when there is > a bug in the current component. In this case I think it is better to > print an error and skip, if not doing more complex error handling. > > Regards, > Han Thanks Han, that makes total sense. I will make this change in v2. > > > struct lport_addresses c_addrs; > > - for (size_t j = 0; j < pb->n_mac; j++) { > > - if (extract_lsp_addresses(pb->mac[j], &c_addrs)) { > > - break; > > - } > > + if (!extract_lsp_addresses(pb->mac[0], &c_addrs)) { > > + continue; > > } > > > > pfd = shash_find_data(&ipv6_prefixd, pb->logical_port); > > -- > > 2.40.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 12055a675..d5957ad69 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -1286,11 +1286,15 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn *ovnsb_idl_txn, continue; } + /* To reach this point, the port binding must be a logical router + * port. LRPs are configured with a single MAC that is always non-NULL. + * Therefore, we can always safely extract pb->mac[0] since it will be + * non-NULL + */ + ovs_assert(pb->n_mac == 1); struct lport_addresses c_addrs; - for (size_t j = 0; j < pb->n_mac; j++) { - if (extract_lsp_addresses(pb->mac[j], &c_addrs)) { - break; - } + if (!extract_lsp_addresses(pb->mac[0], &c_addrs)) { + continue; } pfd = shash_find_data(&ipv6_prefixd, pb->logical_port);
A static analyzer determined that if pb->n_mac was 0, then the c_addrs lport_addresses struct would never be initialized. We would then use and attempt to free uninitialized memory. In reality, pb->n_mac will always be 1. This is because the port binding is a representation of a northbound logical router port. Logical router ports do not contain an array of MACs like the southbound port bindings do. Instead, they have a single MAC string that is always guaranteed to be non-NULL. This string is copied into the port binding's MAC array. Therefore, a southbound port binding that comes from a logical router port will always have n_mac set to 1. How do we know this is a logical router port? The ports iterated over in this function must have IPv6 prefix delegation configured on them. Only northbound logical router ports have this option available. To silence the static analyzer, this change directly retrieves pb->mac[0] instead of iterating over the pb->mac array. It also adds an assertion that pb->n_mac == 1. This arguably makes the code's intent more clear as well. Reported-at: https://issues.redhat.com/browse/FDP-224 Signed-off-by: Mark Michelson <mmichels@redhat.com> --- controller/pinctrl.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)