d/numam-spdk
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

nvmf: don't keep a global discovery log page.

Browse Source 
Keeping a global discovery log page was meant to be a time saving
mechanism, but in the current implementation, it doesn't work properly,
and can cause undesirable behavior and potential crashes. There are two
main problems with keeping a global log page.

1. Admin qpairs can be assigned to any SPDK thread. This means that when
multiple initiators connect to the host and request the discovery log,
they can both be running through the spdk_nvmf_ctrlr_get_log_page
function at the same time. In the event that the discovery generation
counter is incremented while these accesses are occurring, it can cause
one or both of the threads to update the log at the same time. This
results in both logs trying to free the old log page (double free) and
set their log as the new one (possible memory leak).

2. The second problem is that each host is supposed to get a unique
discovery log based on the subsystems to which they have access.
Currently the code relies on whether the discovery log page offset in
the request is equal to 0 to determine if it should load a new discovery
log page or use the cached one. This is inherently faulty because it
relies on initiator provided value to determine what information to
provide from the log page. An initiator could easily send a discovery
request with an offset greater than 0 on purpose to procure most of a
log page provided to another host.

Overall, I think it's safest to not cache the log page at all anymore
and rely on a thread local fresh log page each time.

Reported-by: Curt Bruns <curt.e.bruns@intel.com>

Signed-off-by: Seth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466839 (master)

(cherry picked from commit 20b35d769d)
Change-Id: Ib048e26f139927d888fed7019e0deec346359582
Signed-off-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/467594
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
This commit is contained in:
Seth Howell 2019-08-29 14:54:46 -07:00 committed by Jim Harris
parent f666018041
commit 12f85fa320
4 changed files with 14 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
lib/nvmf/ctrlr_discovery.c
View File

@ -48,8 +48,8 @@
#include "spdk/bdev_module.h"
#include "spdk_internal/log.h"
static void
nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn)
static struct spdk_nvmf_discovery_log_page *
nvmf_generate_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn, size_t *log_page_size)
{
uint64_t numrec = 0;
struct spdk_nvmf_subsystem *subsystem;
@ -66,7 +66,7 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn)
disc_log = calloc(1, cur_size);
if (disc_log == NULL) {
SPDK_ERRLOG("Discovery log page memory allocation error\n");
return;
return NULL;
}
for (sid = 0; sid < tgt->max_subsystems; sid++) {
@ -114,11 +114,9 @@ nvmf_update_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn)
disc_log->numrec = numrec;
disc_log->genctr = tgt->discovery_genctr;
*log_page_size = cur_size;
free(tgt->discovery_log_page);
tgt->discovery_log_page = disc_log;
tgt->discovery_log_page_size = cur_size;
return disc_log;
}
void
@ -128,24 +126,23 @@ spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn,
size_t copy_len = 0;
size_t zero_len = 0;
struct iovec *tmp;
size_t log_page_size = 0;
struct spdk_nvmf_discovery_log_page *discovery_log_page;
if (offset == 0 || tgt->discovery_log_page == NULL ||
tgt->discovery_log_page->genctr != tgt->discovery_genctr) {
nvmf_update_discovery_log(tgt, hostnqn);
}
discovery_log_page = nvmf_generate_discovery_log(tgt, hostnqn, &log_page_size);
/* Copy the valid part of the discovery log page, if any */
if (tgt->discovery_log_page) {
if (discovery_log_page) {
for (tmp = iov; tmp < iov + iovcnt; tmp++) {
copy_len = spdk_min(tmp->iov_len, length);
copy_len = spdk_min(tgt->discovery_log_page_size - offset, copy_len);
copy_len = spdk_min(log_page_size - offset, copy_len);
memcpy(tmp->iov_base, (char *)tgt->discovery_log_page + offset, copy_len);
memcpy(tmp->iov_base, (char *)discovery_log_page + offset, copy_len);
offset += copy_len;
length -= copy_len;
zero_len = tmp->iov_len - copy_len;
if (tgt->discovery_log_page_size <= offset || length == 0) {
if (log_page_size <= offset || length == 0) {
break;
}
}
@ -157,5 +154,7 @@ spdk_nvmf_get_discovery_log_page(struct spdk_nvmf_tgt *tgt, const char *hostnqn,
for (++tmp; tmp < iov + iovcnt; tmp++) {
memset((char *)tmp->iov_base, 0, tmp->iov_len);
}
free(discovery_log_page);
}
}

6
lib/nvmf/nvmf.c
View File

@ -233,8 +233,6 @@ spdk_nvmf_tgt_create(uint32_t max_subsystems)
}
tgt->discovery_genctr = 0;
tgt->discovery_log_page = NULL;
tgt->discovery_log_page_size = 0;
TAILQ_INIT(&tgt->transports);
tgt->subsystems = calloc(tgt->max_subsystems, sizeof(struct spdk_nvmf_subsystem *));
@ -261,10 +259,6 @@ spdk_nvmf_tgt_destroy_cb(void *io_device)
void *destroy_cb_arg;
uint32_t i;
if (tgt->discovery_log_page) {
free(tgt->discovery_log_page);
}
if (tgt->subsystems) {
for (i = 0; i < tgt->max_subsystems; i++) {
if (tgt->subsystems[i]) {

2
lib/nvmf/nvmf_internal.h
View File

@ -81,8 +81,6 @@ struct spdk_nvmf_tgt {
/* Array of subsystem pointers of size max_subsystems indexed by sid */
struct spdk_nvmf_subsystem **subsystems;
struct spdk_nvmf_discovery_log_page *discovery_log_page;
size_t discovery_log_page_size;
TAILQ_HEAD(, spdk_nvmf_transport) transports;
spdk_nvmf_tgt_destroy_done_fn *destroy_cb_fn;

1
test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c
View File

@ -274,7 +274,6 @@ test_discovery_log(void)
subsystem->state = SPDK_NVMF_SUBSYSTEM_INACTIVE;
spdk_nvmf_subsystem_destroy(subsystem);
free(tgt.subsystems);
free(tgt.discovery_log_page);
}
int main(int argc, char **argv)