| Message ID | baf92a6f690b140097a65b7b199000b5cbebaa26.1542987254.git.Haakon.Bugge@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Leon Romanovsky |
| Headers | show |
| Series | Allocate EP addresses dynamically | expand |
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.
Thanks for the review, Doug! > On 3 Dec 2018, at 16:26, Doug Ledford <dledford@redhat.com> wrote: > > 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? The commit doesn't shrink the table, but your point is perfectly valid anyway. The addr_info pointer may change at any time. A read/write locking scheme is needed. > 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. That is a good test. Will add this test. >> 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. I agree with your point. Holding the rw_lock may alleviate the issue here, but doing as you suggest doesn't hurt. Not sure there is a use-case for shrinking it. Either few addresses are used, the old maximum at 4 worked, or we have another use-case where many addresses are used, but not typically deleted. >> + 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. I sure did. Will fix. > You have this same mistaken construct in patch 2/4. Noted. > 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. Safe in the sense it doesn't crash, but not safe in the sense that it could use incorrect data. Look at this hunk in acm_ep_insert_addr(): ep->addr_info[i].addr.type = addr_type; <== Now the entry is valid, but strncpy(ep->addr_info[i].string_buf, name, ACM_MAX_ADDRESS); memcpy(ep->addr_info[i].addr.info.addr, tmp, ACM_MAX_ADDRESS); <=== address info copied here... Well, the above is no excuse for me not doing the right thing ;-) > 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. I do not like this series either. Let me suggest; 1) I clean the commits up and 2), I go back to re-evaluate the use of Port GUID instead of Node GUID. The reason for 2) is that I do not think the ibacm address file will change for most cases, which I presume is a single HCA with one of two ports, and only one address assigned per port. After all, the address file doesn't contain any GUIDs. If the address file doesn't change, my take is to abandon this series and use Port GUIDs instead. The fix, in case of incompatibility, would simply be to delete the address file before starting ibacm. And me confused, since ibacm creates the address file if it is not there, why isn't created _every_ time ibacm is started? And if so, why is it needed at all? Thxs, Håkon
On Mon, 2018-12-03 at 17:14 +0100, Håkon Bugge wrote: > Thanks for the review, Doug! You're welcome. > I do not like this series either. Let me suggest; 1) I clean the > commits up and 2), I go back to re-evaluate the use of Port GUID > instead of Node GUID. > > The reason for 2) is that I do not think the ibacm address file will change for most cases, which I presume is a single HCA with one of two ports, and only one address assigned per port. I wouldn't assume one address per port. Every P_Key on a port creates additional addresses to be stored. > After all, the address file doesn't contain any GUIDs. If the address file doesn't change, my take is to abandon this series and use Port GUIDs instead. The fix, in case of incompatibility, would simply be to delete the address file before starting ibacm. > > And me confused, since ibacm creates the address file if it is not > there, why isn't created _every_ time ibacm is started? And if so, why > is it needed at all? For performance reasons I think. Doesn't it also store remote addresses that it has learned over time? In a 10,000 node cluster, being able to prime your machine name -> address mapping table instead of having to relearn it on reboot is a big time saver. But I could be off, it's been a while since I looked into ibacm.
> On 3 Dec 2018, at 19:10, Doug Ledford <dledford@redhat.com> wrote: > > On Mon, 2018-12-03 at 17:14 +0100, Håkon Bugge wrote: >> Thanks for the review, Doug! > > You're welcome. > >> I do not like this series either. Let me suggest; 1) I clean the >> commits up and 2), I go back to re-evaluate the use of Port GUID >> instead of Node GUID. >> >> The reason for 2) is that I do not think the ibacm address file will change for most cases, which I presume is a single HCA with one of two ports, and only one address assigned per port. > > I wouldn't assume one address per port. Every P_Key on a port creates > additional addresses to be stored. Yes, but in another endpoint, as endpoints are designated by Node GUID, Port, and P_Key. Having looked into proper read/write locking, it will be hairy, because ibacm returns pointers to the (now) dynamic addr_info array. For example, in acm_addr_lookup(), one could add locking such as: ep = container_of(endpoint, struct acmc_ep, endpoint); pthread_rwlock_rdlock(&ep->addr_info_lock); for (i = 0; i < ep->nmbr_ep_addrs; i++) if (!acm_addr_cmp(&ep->addr_info[i].addr, addr, addr_type)) { addr = &ep->addr_info[i].addr; break; } pthread_rwlock_unlock(&ep->addr_info_lock); return addr; But this function returns a pointer to what the rw lock protected, so after the unlock, the address area pointed to by the return value may have been freed and resurrected. So, the lock must be held until the returned pointer "has been fully consumed". I will look into 2) above instead. >> After all, the address file doesn't contain any GUIDs. If the address file doesn't change, my take is to abandon this series and use Port GUIDs instead. The fix, in case of incompatibility, would simply be to delete the address file before starting ibacm. >> >> And me confused, since ibacm creates the address file if it is not >> there, why isn't created _every_ time ibacm is started? And if so, why >> is it needed at all? > > For performance reasons I think. Doesn't it also store remote addresses > that it has learned over time? In a 10,000 node cluster, being able to > prime your machine name -> address mapping table instead of having to > relearn it on reboot is a big time saver. But I could be off, it's been > a while since I looked into ibacm. By default, there is no storing of addresses. It was in legacy versions, but now we have the following option in the ibacm_opts.cfg file: # support_ips_in_addr_cfg: # If 1 continue to read IP addresses from ibacm_addr.cfg # Default is 0 "no" # support_ips_in_addr_cfg 0 I will see if I can have an option to select Node vs. Port GUID. This way, the backwards compatibility issues will be solved. Thxs, Håkon
On Fri, Dec 07, 2018 at 02:51:45PM +0100, Håkon Bugge wrote: > > > > On 3 Dec 2018, at 19:10, Doug Ledford <dledford@redhat.com> wrote: > > > > On Mon, 2018-12-03 at 17:14 +0100, Håkon Bugge wrote: > >> Thanks for the review, Doug! > > > > You're welcome. > > > >> I do not like this series either. Let me suggest; 1) I clean the > >> commits up and 2), I go back to re-evaluate the use of Port GUID > >> instead of Node GUID. > >> > >> The reason for 2) is that I do not think the ibacm address file will change for most cases, which I presume is a single HCA with one of two ports, and only one address assigned per port. > > > > I wouldn't assume one address per port. Every P_Key on a port creates > > additional addresses to be stored. > > Yes, but in another endpoint, as endpoints are designated by Node GUID, Port, and P_Key. > > Having looked into proper read/write locking, it will be hairy, because ibacm returns pointers to the (now) dynamic addr_info array. For example, in acm_addr_lookup(), one could add locking such as: > > ep = container_of(endpoint, struct acmc_ep, endpoint); > pthread_rwlock_rdlock(&ep->addr_info_lock); > for (i = 0; i < ep->nmbr_ep_addrs; i++) > if (!acm_addr_cmp(&ep->addr_info[i].addr, addr, addr_type)) { > addr = &ep->addr_info[i].addr; > break; > } > > pthread_rwlock_unlock(&ep->addr_info_lock); > return addr; > > But this function returns a pointer to what the rw lock protected, > so after the unlock, the address area pointed to by the return value > may have been freed and resurrected. So, the lock must be held until > the returned pointer "has been fully consumed". Just copy it? Jason
diff --git a/ibacm/prov/acmp/src/acmp.c b/ibacm/prov/acmp/src/acmp.c index c10e62c0..f63f1dbf 100644 --- a/ibacm/prov/acmp/src/acmp.c +++ b/ibacm/prov/acmp/src/acmp.c @@ -166,7 +166,7 @@ struct acmp_send_queue { struct acmp_addr { uint16_t type; union acm_ep_info info; - struct acm_address *addr; + struct acm_address addr; struct acmp_ep *ep; }; @@ -2379,7 +2379,7 @@ static int acmp_add_addr(const struct acm_address *addr, void *ep_context, } ep->addr_info[i].type = addr->type; memcpy(&ep->addr_info[i].info, &addr->info, sizeof(addr->info)); - ep->addr_info[i].addr = (struct acm_address *) addr; + memcpy(&ep->addr_info[i].addr, addr, sizeof(*addr)); ep->addr_info[i].ep = ep; if (loopback_prot != ACMP_LOOPBACK_PROT_LOCAL) { @@ -2439,7 +2439,7 @@ static void acmp_remove_addr(void *addr_context) pthread_mutex_lock(&port->lock); list_for_each(&port->ep_list, ep, entry) { pthread_mutex_unlock(&port->lock); - dest = acmp_get_dest(ep, address->type, address->addr->info.addr); + dest = acmp_get_dest(ep, address->type, address->addr.info.addr); if (dest) { acm_log(2, "Found a dest addr, deleting it\n"); pthread_mutex_lock(&ep->lock); diff --git a/ibacm/src/acm.c b/ibacm/src/acm.c index 182e89d9..6a02f33d 100644 --- a/ibacm/src/acm.c +++ b/ibacm/src/acm.c @@ -72,7 +72,6 @@ #define src_index data[1] #define dst_index data[2] -#define MAX_EP_ADDR 4 #define NL_MSG_BUF_SIZE 4096 #define ACM_PROV_NAME_SIZE 64 #define NL_CLIENT_INDEX 0 @@ -139,7 +138,8 @@ struct acmc_ep { struct acmc_port *port; struct acm_endpoint endpoint; void *prov_ep_context; - struct acmc_addr addr_info[MAX_EP_ADDR]; + int nmbr_ep_addrs; + struct acmc_addr *addr_info; struct list_node entry; }; @@ -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]; } @@ -1118,17 +1118,22 @@ acm_svr_resolve_path(struct acmc_client *client, struct acm_msg *msg) static int acm_svr_resolve(struct acmc_client *client, struct acm_msg *msg) { + int ret = 0; + (void) atomic_inc(&client->refcnt); if (msg->resolve_data[0].type == ACM_EP_INFO_PATH) { if (msg->resolve_data[0].flags & ACM_FLAGS_QUERY_SA) { - return acm_svr_query_path(client, msg); + ret = acm_svr_query_path(client, msg); } else { - return acm_svr_resolve_path(client, msg); + ret = acm_svr_resolve_path(client, msg); } } else { - return acm_svr_resolve_dest(client, msg); + ret = acm_svr_resolve_dest(client, msg); } + + free(msg); + return ret; } static int acm_svr_perf_query(struct acmc_client *client, struct acm_msg *msg) @@ -1181,15 +1186,48 @@ static int acm_svr_perf_query(struct acmc_client *client, struct acm_msg *msg) else ret = 0; + free(msg); return ret; } +static int may_be_realloc(struct acm_msg **msg_ptr, + int len, + int cnt, + int *cur_msg_siz_ptr, + int max_msg_siz) +{ + + /* Check if a new address exceeds the protocol constrained max size */ + if (len + (cnt + 1) * ACM_MAX_ADDRESS > max_msg_siz) { + acm_log(0, "ERROR - unable to amend more addresses to acm_msg due to protocol constraints\n"); + return ENOMEM; + } + + /* Check if a new address exeeds current size of msg */ + if (len + (cnt + 1) * ACM_MAX_ADDRESS > *cur_msg_siz_ptr) { + const size_t chunk_size = 16 * ACM_MAX_ADDRESS; + struct acm_msg *new_msg = realloc(*msg_ptr, *cur_msg_siz_ptr + chunk_size); + + if (!new_msg) { + acm_log(0, "ERROR - failed to allocate longer acm_msg\n"); + return ENOMEM; + } + + *msg_ptr = new_msg; + *cur_msg_siz_ptr += chunk_size; + } + + return 0; +} + static int acm_svr_ep_query(struct acmc_client *client, struct acm_msg *msg) { int ret, i; uint16_t len; struct acmc_ep *ep; int index, cnt = 0; + int cur_msg_siz = sizeof(*msg); + int max_msg_siz = USHRT_MAX; acm_log(2, "client %d\n", client->index); index = msg->hdr.data[0]; @@ -1203,8 +1241,10 @@ static int acm_svr_ep_query(struct acmc_client *client, struct acm_msg *msg) ACM_MAX_PROV_NAME - 1); msg->ep_data[0].prov_name[ACM_MAX_PROV_NAME - 1] = '\0'; len = ACM_MSG_HDR_LENGTH + sizeof(struct acm_ep_config_data); - for (i = 0; i < MAX_EP_ADDR; i++) { + for (i = 0; i < ep->nmbr_ep_addrs; i++) { if (ep->addr_info[i].addr.type != ACM_ADDRESS_INVALID) { + if (may_be_realloc(&msg, len, cnt, &cur_msg_siz, max_msg_siz)) + break; memcpy(msg->ep_data[0].addrs[cnt++].name, ep->addr_info[i].string_buf, ACM_MAX_ADDRESS); @@ -1227,6 +1267,7 @@ static int acm_svr_ep_query(struct acmc_client *client, struct acm_msg *msg) else ret = 0; + free(msg); return ret; } @@ -1238,38 +1279,47 @@ static int acm_msg_length(struct acm_msg *msg) static void acm_svr_receive(struct acmc_client *client) { - struct acm_msg msg; + struct acm_msg *msg = malloc(sizeof(*msg)); int ret; + if (!msg) { + acm_log(0, "ERROR - Unable to alloc acm_msg\n"); + ret = ENOMEM; + goto out; + } + acm_log(2, "client %d\n", client->index); - ret = recv(client->sock, (char *) &msg, sizeof msg, 0); - if (ret <= 0 || ret != acm_msg_length(&msg)) { + ret = recv(client->sock, (char *) msg, sizeof(*msg), 0); + if (ret <= 0 || ret != acm_msg_length(msg)) { acm_log(2, "client disconnected\n"); ret = ACM_STATUS_ENOTCONN; - goto out; + goto out_free; } - if (msg.hdr.version != ACM_VERSION) { - acm_log(0, "ERROR - unsupported version %d\n", msg.hdr.version); - goto out; + if (msg->hdr.version != ACM_VERSION) { + acm_log(0, "ERROR - unsupported version %d\n", msg->hdr.version); + goto out_free; } - switch (msg.hdr.opcode & ACM_OP_MASK) { + switch (msg->hdr.opcode & ACM_OP_MASK) { case ACM_OP_RESOLVE: atomic_inc(&counter[ACM_CNTR_RESOLVE]); - ret = acm_svr_resolve(client, &msg); - break; + ret = acm_svr_resolve(client, msg); + goto out; case ACM_OP_PERF_QUERY: - ret = acm_svr_perf_query(client, &msg); - break; + ret = acm_svr_perf_query(client, msg); + goto out; case ACM_OP_EP_QUERY: - ret = acm_svr_ep_query(client, &msg); - break; + ret = acm_svr_ep_query(client, msg); + goto out; default: - acm_log(0, "ERROR - unknown opcode 0x%x\n", msg.hdr.opcode); + acm_log(0, "ERROR - unknown opcode 0x%x\n", msg->hdr.opcode); break; } +out_free: + free(msg); + out: if (ret) acm_disconnect_client(client); @@ -1412,7 +1462,7 @@ static int resync_system_ips(void) port = &dev->port[cnt]; list_for_each(&port->ep_list, ep, entry) { - for (i = 0; i < MAX_EP_ADDR; i++) { + for (i = 0; i < ep->nmbr_ep_addrs; i++) { if (ep->addr_info[i].addr.type == ACM_ADDRESS_IP || ep->addr_info[i].addr.type == ACM_ADDRESS_IP6) ep->addr_info[i].addr.type = ACM_ADDRESS_INVALID; @@ -1672,7 +1722,7 @@ static void acm_nl_process_invalid_request(struct acmc_client *client, static void acm_nl_process_resolve(struct acmc_client *client, struct acm_nl_msg *acmnlmsg) { - struct acm_msg msg; + struct acm_msg *msg = malloc(sizeof(*msg)); struct nlattr *attr; int payload_len; int resolve_hdr_len; @@ -1681,21 +1731,26 @@ static void acm_nl_process_resolve(struct acmc_client *client, int status; unsigned char *data; - memset(&msg, 0, sizeof(msg)); - msg.hdr.opcode = ACM_OP_RESOLVE; - msg.hdr.version = ACM_VERSION; - msg.hdr.length = ACM_MSG_HDR_LENGTH + ACM_MSG_EP_LENGTH; - msg.hdr.status = ACM_STATUS_SUCCESS; - msg.hdr.tid = (uintptr_t) acmnlmsg; - msg.resolve_data[0].type = ACM_EP_INFO_PATH; + if (!msg) { + acm_log(0, "ERROR - Unable to allocate msg\n"); + return; + } + + memset(msg, 0, sizeof(*msg)); + msg->hdr.opcode = ACM_OP_RESOLVE; + msg->hdr.version = ACM_VERSION; + msg->hdr.length = ACM_MSG_HDR_LENGTH + ACM_MSG_EP_LENGTH; + msg->hdr.status = ACM_STATUS_SUCCESS; + msg->hdr.tid = (uintptr_t) acmnlmsg; + msg->resolve_data[0].type = ACM_EP_INFO_PATH; /* We support only one pathrecord */ acm_log(2, "path use 0x%x\n", acmnlmsg->resolve_header.path_use); if (acmnlmsg->resolve_header.path_use == LS_RESOLVE_PATH_USE_UNIDIRECTIONAL) - msg.resolve_data[0].info.path.reversible_numpath = 1; + msg->resolve_data[0].info.path.reversible_numpath = 1; else - msg.resolve_data[0].info.path.reversible_numpath = + msg->resolve_data[0].info.path.reversible_numpath = IBV_PATH_RECORD_REVERSIBLE | 1; data = (unsigned char *) &acmnlmsg->nlmsg_header + NLMSG_HDRLEN; @@ -1710,7 +1765,7 @@ static void acm_nl_process_resolve(struct acmc_client *client, attr->nla_len > rem) break; - status = acm_nl_parse_path_attr(attr, &msg.resolve_data[0]); + status = acm_nl_parse_path_attr(attr, &msg->resolve_data[0]); if (status) { acm_nl_process_invalid_request(client, acmnlmsg); return; @@ -1723,7 +1778,7 @@ static void acm_nl_process_resolve(struct acmc_client *client, } atomic_inc(&counter[ACM_CNTR_RESOLVE]); - acm_svr_resolve(client, &msg); + acm_svr_resolve(client, msg); } 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; + 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; + } + /* Added memory is not initialized */ + memset(ep->addr_info + i, 0, sizeof(*ep->addr_info)); + ep->addr_info[i].addr.endpoint = &ep->endpoint; + ep->addr_info[i].addr.id_string = ep->addr_info[i].string_buf; } /* Open the provider endpoint only if at least a name or @@ -2194,7 +2258,7 @@ static void acm_ep_down(struct acmc_ep *ep) acm_log(1, "%s %d pkey 0x%04x\n", ep->port->dev->device.verbs->device->name, ep->port->port.port_num, 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 && ep->addr_info[i].prov_addr_context) ep->port->prov->remove_address(ep->addr_info[i]. @@ -2211,7 +2275,6 @@ static struct acmc_ep * acm_alloc_ep(struct acmc_port *port, uint16_t pkey) { struct acmc_ep *ep; - int i; acm_log(1, "\n"); ep = calloc(1, sizeof *ep); @@ -2221,11 +2284,7 @@ acm_alloc_ep(struct acmc_port *port, uint16_t pkey) ep->port = port; ep->endpoint.port = &port->port; ep->endpoint.pkey = pkey; - - for (i = 0; i < MAX_EP_ADDR; i++) { - ep->addr_info[i].addr.endpoint = &ep->endpoint; - ep->addr_info[i].addr.id_string = ep->addr_info[i].string_buf; - } + ep->nmbr_ep_addrs = 0; return ep; } diff --git a/ibacm/src/acm_util.c b/ibacm/src/acm_util.c index f4654f12..0cc23304 100644 --- a/ibacm/src/acm_util.c +++ b/ibacm/src/acm_util.c @@ -143,7 +143,7 @@ int acm_if_iter_sys(acm_if_iter_cb cb, void *ctx) return -1; } - len = sizeof(*ifc) + sizeof(*ifr) * 64; + len = sizeof(*ifc) + sizeof(*ifr) * 256; ifc = malloc(len); if (!ifc) { ret = -1; diff --git a/ibacm/src/libacm.c b/ibacm/src/libacm.c index 1d9d7145..8f0bfc2a 100644 --- a/ibacm/src/libacm.c +++ b/ibacm/src/libacm.c @@ -384,11 +384,13 @@ out: int ib_acm_enum_ep(int index, struct acm_ep_config_data **data) { + struct acm_ep_config_data *netw_edata = NULL; + struct acm_ep_config_data *host_edata = NULL; struct acm_msg msg; + struct acm_hdr hdr; int ret; int len; - int cnt; - struct acm_ep_config_data *edata; + int i; pthread_mutex_lock(&acm_lock); memset(&msg, 0, sizeof msg); @@ -401,33 +403,49 @@ int ib_acm_enum_ep(int index, struct acm_ep_config_data **data) if (ret != ACM_MSG_HDR_LENGTH) goto out; - ret = recv(sock, (char *) &msg, sizeof msg, 0); - if (ret < ACM_MSG_HDR_LENGTH || ret != be16toh(msg.hdr.length)) { + ret = recv(sock, (char *) &hdr, sizeof(hdr), 0); + if (ret != sizeof(hdr)) { ret = ACM_STATUS_EINVAL; goto out; } - if (msg.hdr.status) { - ret = acm_error(msg.hdr.status); + if (hdr.status) { + ret = acm_error(hdr.status); goto out; } - cnt = be16toh(msg.ep_data[0].addr_cnt); - len = sizeof(struct acm_ep_config_data) + - ACM_MAX_ADDRESS * cnt; - edata = malloc(len); - if (!edata) { + len = be16toh(hdr.length) - sizeof(hdr); + netw_edata = (struct acm_ep_config_data *)malloc(len); + host_edata = (struct acm_ep_config_data *)malloc(len); + if (!netw_edata || !host_edata) { ret = ACM_STATUS_ENOMEM; goto out; } - memcpy(edata, &msg.ep_data[0], len); - edata->dev_guid = be64toh(msg.ep_data[0].dev_guid); - edata->pkey = be16toh(msg.ep_data[0].pkey); - edata->addr_cnt = cnt; - *data = edata; + ret = recv(sock, (char *)netw_edata, len, 0); + if (ret != len) { + ret = ACM_STATUS_EINVAL; + goto out; + } + + host_edata->dev_guid = be64toh(netw_edata->dev_guid); + host_edata->port_num = netw_edata->port_num; + host_edata->pkey = be16toh(netw_edata->pkey); + host_edata->addr_cnt = be16toh(netw_edata->addr_cnt); + + memcpy(host_edata->prov_name, netw_edata->prov_name, + sizeof(host_edata->prov_name)); + + for (i = 0; i < host_edata->addr_cnt; ++i) + host_edata->addrs[i] = netw_edata->addrs[i]; + + *data = host_edata; ret = 0; out: + if (ret) + free(host_edata); + free(netw_edata); + pthread_mutex_unlock(&acm_lock); return ret; }
ibacm has a fixed number (4) of end-point addresses. An endpoint is associated with the node GUID. Hence, if a system is brought up with VFs enabled, ibacm will not be able to handle the end-point addresses. When adding an address, the default provider, acmp, saves a pointer into ibacm's acmc_ep addr_info attribute. But this may now be re-allocated. Its generally a bad idea to have pointers in the provider(s) pointing into acm's data structures. This is fixed in acmp by changing struct acmp_addr to contain struct acm_address instead of a pointer to it, and copying the address, similar to how the info struct is handled. In acm_svr_ep_query(), dynamic allocation and conditional reallocating is implemented, in the case the number of end-point addresses exceeds the fixed size of struct acm_msg. A re-factoring in acm_nl_process_resolve() and in the acm_svr_foo_bar() functions was necessary to accommodate the dynamic allocation of struct acm_msg. In acm_if_iter_sys(), the number of struct ifreq's was increased to 256, in order to be able to test with up-to 256 addresses when ibacm starts. Finally, in ib_acm_enum_ep(), the read() was split in two, first read the hdr, then determine the size of the rest of the data, allocate it, and read them. To avoid endian conversion to/from the same structure, two structures were allocated for simplicity. Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> --- ibacm/prov/acmp/src/acmp.c | 6 +- ibacm/src/acm.c | 155 +++++++++++++++++++++++++++++++-------------- ibacm/src/acm_util.c | 2 +- ibacm/src/libacm.c | 50 ++++++++++----- 4 files changed, 145 insertions(+), 68 deletions(-)