On Fri, 2018-11-23 at 16:40 +0100, Håkon Bugge wrote:
> @@ -421,7 +421,7 @@ static void acm_mark_addr_invalid(struct acmc_ep *ep,
> {
> int i;
>
> - for (i = 0; i < MAX_EP_ADDR; i++) {
> + for (i = 0; i < ep->nmbr_ep_addrs; i++) {
> if (!acm_addr_cmp(&ep->addr_info[i].addr, data->info.addr, data->type)) {
> ep->addr_info[i].addr.type = ACM_ADDRESS_INVALID;
> ep->port->prov->remove_address(ep->addr_info[i].prov_addr_context);
> @@ -437,7 +437,7 @@ acm_addr_lookup(const struct acm_endpoint *endpoint, uint8_t *addr, uint8_t addr
> int i;
>
> ep = container_of(endpoint, struct acmc_ep, endpoint);
> - for (i = 0; i < MAX_EP_ADDR; i++)
> + for (i = 0; i < ep->nmbr_ep_addrs; i++)
> if (!acm_addr_cmp(&ep->addr_info[i].addr, addr, addr_type))
> return &ep->addr_info[i].addr;
>
> @@ -838,7 +838,7 @@ acm_get_port_ep_address(struct acmc_port *port, struct acm_ep_addr_data *data)
> if ((data->type == ACM_EP_INFO_PATH) &&
> (!data->info.path.pkey ||
> (be16toh(data->info.path.pkey) == ep->endpoint.pkey))) {
> - for (i = 0; i < MAX_EP_ADDR; i++) {
> + for (i = 0; i < ep->nmbr_ep_addrs; i++) {
> if (ep->addr_info[i].addr.type)
> return &ep->addr_info[i];
> }
All of these loops over nmbr_ep_addrs worry me. What happens if we are
doing one of these loops while someone else is simultaneously removing
addresses? I suggesting running cmtime with some large number of
connections while simultaneously running a loop on another terminal that
adds addresses up to 250 addresses and then removes addresses all the
way back down to 1.
>
> static int acm_nl_is_valid_resolve_request(struct acm_nl_msg *acmnlmsg)
> @@ -2018,12 +2073,21 @@ acm_ep_insert_addr(struct acmc_ep *ep, const char *name, uint8_t *addr,
> memcpy(tmp, addr, acm_addr_len(addr_type));
>
> if (!acm_addr_lookup(&ep->endpoint, addr, addr_type)) {
> - for (i = 0; (i < MAX_EP_ADDR) &&
> + for (i = 0; (i < ep->nmbr_ep_addrs) &&
> (ep->addr_info[i].addr.type != ACM_ADDRESS_INVALID); i++)
> ;
> - if (i == MAX_EP_ADDR) {
> - ret = ENOMEM;
> - goto out;
> + if (i == ep->nmbr_ep_addrs) {
> + ++ep->nmbr_ep_addrs;
Don't increment this prior to the realloc succeeding. You just created
a race condition that can cause somewhere else in the code to access off
the end of the array if they read this incremented counter while you
still have the old addr_info pointer. Always increase the array first,
increment the array size counter second. Do the opposite when shrinking
the array.
> + ep->addr_info = realloc(ep->addr_info, ep->nmbr_ep_addrs * sizeof(*ep->addr_info));
> + if (!ep->addr_info) {
> + ret = ENOMEM;
> + --ep->nmbr_ep_addrs;
> + goto out;
You just leaked the old addr_info on a failed realloc because you saved
into your original pointer, thereby overwriting it, and realloc does not
free the old pointer on failure.
You have this same mistaken construct in patch 2/4.
In general, this patch, as it stands, looks very, very fragile to me.
The old method used a static array of a fixed size that never changed
address, so all sorts of simultaneous accesses were safe, but in the new
way of doing things that's no longer true. Our addr_info pointer can
change mid-way through one of those for loops. And our max address
count can also change mid-way through one of those for loops. We have
lots of those loops, so I think you are going to need some sort of
locking around them. Either a rwlock_t or something else. I suspect
the test I listed above will crash this new feature rather reliably.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
