On Fri, Jul 14, 2023 at 2:44 PM Dumitru Ceara <[email protected]> wrote:
> On 7/14/23 14:39, Dumitru Ceara wrote:
> > It's specified in RFC 8415. This also avoids having to free/realloc the
> > pfd->uuid.data memory. That part was not correct anyway and was flagged
> > by ASAN as a memleak:
> >
> > Direct leak of 42 byte(s) in 3 object(s) allocated from:
> > #0 0x55e5b6354c9e in malloc
> (/workspace/ovn-tmp/controller/ovn-controller+0x2edc9e) (BuildId:
> f963f8c756bd5a2207a9b3c922d4362e46bb3162)
> > #1 0x55e5b671878d in xmalloc__
> /workspace/ovn-tmp/ovs/lib/util.c:140:15
> > #2 0x55e5b671878d in xmalloc
> /workspace/ovn-tmp/ovs/lib/util.c:175:12
> > #3 0x55e5b642cebc in pinctrl_parse_dhcpv6_reply
> /workspace/ovn-tmp/controller/pinctrl.c:997:20
> > #4 0x55e5b642cebc in pinctrl_handle_dhcp6_server
> /workspace/ovn-tmp/controller/pinctrl.c:1040:9
> > #5 0x55e5b642cebc in process_packet_in
> /workspace/ovn-tmp/controller/pinctrl.c:3210:9
> > #6 0x55e5b642cebc in pinctrl_recv
> /workspace/ovn-tmp/controller/pinctrl.c:3290:9
> > #7 0x55e5b642cebc in pinctrl_handler
> /workspace/ovn-tmp/controller/pinctrl.c:3385:17
> > #8 0x55e5b66ef664 in ovsthread_wrapper
> /workspace/ovn-tmp/ovs/lib/ovs-thread.c:423:12
> > #9 0x7faa30194b42 (/lib/x86_64-linux-gnu/libc.so.6+0x94b42)
> (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
> >
> > Fixes: faa44a0c60a3 ("controller: IPv6 Prefix-Delegation: introduce
> RENEW/REBIND msg support")
>
> I forgot to add Ales as co-author, I apologize.
>
> Co-authored-by: Ales Musil <[email protected]>
> Signed-off-by: Ales Musil <[email protected]>
>
> > Signed-off-by: Dumitru Ceara <[email protected]>
> > ---
> > controller/pinctrl.c | 33 ++++++++++++++++-----------------
> > 1 file changed, 16 insertions(+), 17 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 6027ba0afb..bed90fe0b7 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -674,6 +674,14 @@ enum {
> > PREFIX_REBIND,
> > };
> >
> > +/* According to RFC 8415, section 11:
> > + * A DUID consists of a 2-octet type code represented in network byte
> > + * order, followed by a variable number of octets that make up the
> > + * actual identifier. The length of the DUID (not including the type
> > + * code) is at least 1 octet and at most 128 octets.
> > +*/
> > +#define DHCPV6_MAX_DUID_LEN 130
> > +
> > struct ipv6_prefixd_state {
> > long long int next_announce;
> > long long int last_complete;
> > @@ -683,7 +691,7 @@ struct ipv6_prefixd_state {
> > struct eth_addr sa;
> > /* server_id_info */
> > struct {
> > - uint8_t *data;
> > + uint8_t data[DHCPV6_MAX_DUID_LEN];
> > uint8_t len;
> > } uuid;
> > struct in6_addr ipv6_addr;
> > @@ -899,7 +907,7 @@ pinctrl_prefixd_state_handler(const struct flow
> *ip_flow,
> > struct eth_addr sa, struct in6_addr
> server_addr,
> > char prefix_len, unsigned t1, unsigned t2,
> > unsigned plife_time, unsigned vlife_time,
> > - uint8_t *uuid, uint8_t uuid_len)
> > + const uint8_t *uuid, uint8_t uuid_len)
> > {
> > struct ipv6_prefixd_state *pfd;
> >
> > @@ -908,7 +916,7 @@ pinctrl_prefixd_state_handler(const struct flow
> *ip_flow,
> > pfd->state = PREFIX_PENDING;
> > pfd->server_addr = server_addr;
> > pfd->sa = sa;
> > - pfd->uuid.data = uuid;
> > + memcpy(pfd->uuid.data, uuid, uuid_len);
> > pfd->uuid.len = uuid_len;
> > pfd->plife_time = plife_time * 1000;
> > pfd->vlife_time = vlife_time * 1000;
> > @@ -933,8 +941,9 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in,
> > unsigned char *in_dhcpv6_data = (unsigned char *)(udp_in + 1);
> > size_t dlen = MIN(ntohs(udp_in->udp_len),
> dp_packet_l4_size(pkt_in));
> > unsigned t1 = 0, t2 = 0, vlife_time = 0, plife_time = 0;
> > - uint8_t *end = (uint8_t *)udp_in + dlen, *uuid = NULL;
> > + uint8_t *end = (uint8_t *) udp_in + dlen;
> > uint8_t prefix_len = 0, uuid_len = 0;
> > + uint8_t uuid[DHCPV6_MAX_DUID_LEN];
> > struct in6_addr ipv6 = in6addr_any;
> > bool status = false;
> > unsigned aid = 0;
> > @@ -993,8 +1002,7 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet *pkt_in,
> > break;
> > }
> > case DHCPV6_OPT_SERVER_ID_CODE:
> > - uuid_len = ntohs(in_opt->len);
> > - uuid = xmalloc(uuid_len);
> > + uuid_len = MIN(ntohs(in_opt->len), DHCPV6_MAX_DUID_LEN);
> > memcpy(uuid, in_opt + 1, uuid_len);
> > break;
> > default:
> > @@ -1014,8 +1022,6 @@ pinctrl_parse_dhcpv6_reply(struct dp_packet
> *pkt_in,
> > pinctrl_prefixd_state_handler(ip_flow, ipv6, aid, eth->eth_src,
> > ip6_src, prefix_len, t1, t2,
> > plife_time, vlife_time, uuid,
> uuid_len);
> > - } else if (uuid) {
> > - free(uuid);
> > }
> > }
> >
> > @@ -1212,10 +1218,7 @@ static bool ipv6_prefixd_should_inject(void)
> > if (pfd->state == PREFIX_RENEW &&
> > cur_time > pfd->last_complete + pfd->t2) {
> > pfd->state = PREFIX_REBIND;
> > - if (pfd->uuid.len) {
> > - free(pfd->uuid.data);
> > - pfd->uuid.len = 0;
> > - }
> > + pfd->uuid.len = 0;
> > return true;
> > }
> > if (pfd->state == PREFIX_REBIND &&
> > @@ -1409,12 +1412,8 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > SHASH_FOR_EACH_SAFE (iter, &ipv6_prefixd) {
> > struct ipv6_prefixd_state *pfd = iter->data;
> > if (pfd->last_used + IPV6_PREFIXD_STALE_TIMEOUT < time_msec()) {
> > - if (pfd->uuid.len) {
> > - free(pfd->uuid.data);
> > - pfd->uuid.len = 0;
> > - }
> > - free(pfd);
> > shash_delete(&ipv6_prefixd, iter);
> > + free(pfd);
> > }
> > }
> >
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <[email protected]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
