daxctl_dev_get_memory allocation issue

[michalbiesek]

General Information

I try to to retrieve information about device-dax which are configured in "system-ram" mode.
For this scenario I use daxctl_dev_get_memory function.
See Implementation details below.

Implementation details

#include <daxctl/libdaxctl.h>
#include <stdlib.h>
#include <stdio.h>

int main()
{
    struct daxctl_ctx *ctx;
    struct daxctl_region *region;
    struct daxctl_dev *dev;


    int rc = daxctl_new(&ctx);
    if (rc < 0)
        return -1;

    daxctl_region_foreach(ctx, region) {
        daxctl_dev_foreach(region, dev) {
            struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
            if (mem) {
              int node = daxctl_dev_get_target_node(dev);
              printf("Node %d", node); 
            }
            free(mem);    ////potential issue
        }
    }

    daxctl_unref(ctx);
    return 0;
}

I saw that there is memory allocation in:

ndctl/daxctl/lib/libdaxctl.c

Line 445 in cec5975

mem = calloc(1, sizeof(*mem));

Currently I see potential issue that I don't know if could call free on struct daxctl_memory * or not, since there are 2 way to get struct daxctl_memory:

ndctl/daxctl/lib/libdaxctl.c

Lines 1064 to 1067 in cec5975

	if (dev->mem)
	return dev->mem;
	else
	return daxctl_dev_alloc_mem(dev);

Additional Information

I think that this operation in case of memory allocation could result with memory leak in e.g.:

ndctl/util/json.c

Line 272 in cec5975

struct daxctl_memory *mem = daxctl_dev_get_memory(dev);

in case of daxctl device is configured as system-mode-ram

[djbw]

The above code sequence needs a daxctl_dev_foreach_safe() definition which does not exist yet, but you can see an example of a _foreach_safe() implementation here:

ndctl/ndctl/libndctl.h

Lines 444 to 453 in cec5975

	#define ndctl_namespace_foreach(region, ndns) \
	for (ndns = ndctl_namespace_get_first(region); \
	ndns != NULL; \
	ndns = ndctl_namespace_get_next(ndns))
	#define ndctl_namespace_foreach_safe(region, ndns, _ndns) \
	for (ndns = ndctl_namespace_get_first(region), \
	_ndns = ndns ? ndctl_namespace_get_next(ndns) : NULL; \
	ndns != NULL; \
	ndns = _ndns, \
	_ndns = _ndns ? ndctl_namespace_get_next(_ndns) : NULL)

[stellarhopper]

You should not be freeing the 'mem' object at all. Freeing is handled at teardown, in free_dev() via free_mem().

You can call 'daxctl_dev_get_memory()' as many times as you want, and if a valid memory object has been allocated, you will always get the same one.

[djbw]

Oh whoops, I read too quickly. I second what Vishal said, but I'll still send some _safe helpers to finish off the api.

[michalbiesek]

@djbw @stellarhopper Thanks for quick response.

You should not be freeing the 'mem' object at all. Freeing is handled at teardown, in free_dev() via free_mem().
You can call 'daxctl_dev_get_memory()' as many times as you want, and if a valid memory object has been allocated, you will always get the same one.

IMHO this will not be handled correctly in following situation:

Configuration

OS: Ubuntu 18.04.3 LTS
Kernel: 5.3.1
ndctl: version 66

ndctl list

[
{
"dev":"namespace0.0",
"mode":"devdax",
"map":"mem",
"size":4292870144,
"uuid":"668d3db9-9fc2-422a-a395-91a219795d3c",
"chardev":"dax0.0",
"align":2097152
}
]

daxctl list

[
{
"chardev":"dax0.0",
"size":4292870144,
"target_node":2,
"mode":"system-ram"
}
]

Long story short I got one ndctl namespace in devdax mode - with one dax device configured in mode: system-ram.

Example

My example is a simple program with intention to print dax devices in system-ram mode.
I remove the free call which @stellarhopper suggest.

#include <daxctl/libdaxctl.h>
#include <stdlib.h>
#include <stdio.h>

#define daxctl_dev_foreach_safe(region, dev, _dev) \
    for (dev = daxctl_dev_get_first(region), \
         _dev = dev ? daxctl_dev_get_next(dev) : NULL; \
         dev != NULL; \
         dev = _dev, \
        _dev = _dev ? daxctl_dev_get_next(_dev) : NULL)
int main()
{
    struct daxctl_ctx *ctx;
    struct daxctl_region *region;
    struct daxctl_dev *dev, *_dev;


    int rc = daxctl_new(&ctx);
    if (rc < 0)
        return -1;

    daxctl_region_foreach(ctx, region) {
        daxctl_dev_foreach_safe(region, dev, _dev) {
            struct daxctl_memory *mem = daxctl_dev_get_memory(dev);
            if (mem) {
              int node = daxctl_dev_get_target_node(dev);
              printf("Node %d", node);
            }
        }
    }

    daxctl_unref(ctx);
    return 0;
}

Details

I use valgrind to confirm my concern, please see below fragment of valgrind log, which described memory leak ( full log valgrind-out.txt

==22056== HEAP SUMMARY:
==22056==     in use at exit: 356 bytes in 3 blocks
==22056==   total heap usage: 1,261 allocs, 1,258 frees, 425,667 bytes allocated
==22056== 
==22056== Searching for pointers to 3 not-freed blocks
==22056== Checked 79,512 bytes
==22056== 
==22056== 31 bytes in 1 blocks are indirectly lost in loss record 1 of 3
==22056==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22056==    by 0x517848F: __vasprintf_chk (vasprintf_chk.c:80)
==22056==    by 0x517832E: __asprintf_chk (asprintf_chk.c:32)
==22056==    by 0x4E40B4D: asprintf (stdio2.h:178)
==22056==    by 0x4E40B4D: daxctl_dev_alloc_mem (libdaxctl.c:462)
==22056==    by 0x108A4B: main (daxctl_example.c:25)
==22056== 
==22056== 285 bytes in 1 blocks are indirectly lost in loss record 2 of 3
==22056==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22056==    by 0x4E40B60: daxctl_dev_alloc_mem (libdaxctl.c:469)
==22056==    by 0x108A4B: main (daxctl_example.c:25)
==22056== 
==22056== 356 (40 direct, 316 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==22056==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22056==    by 0x4E40AFB: daxctl_dev_alloc_mem (libdaxctl.c:445)
==22056==    by 0x108A4B: main (daxctl_example.c:25)
==22056== 
==22056== LEAK SUMMARY:
==22056==    definitely lost: 40 bytes in 1 blocks
==22056==    indirectly lost: 316 bytes in 2 blocks
==22056==      possibly lost: 0 bytes in 0 blocks
==22056==    still reachable: 0 bytes in 0 blocks
==22056==         suppressed: 0 bytes in 0 blocks

So I think memory allocated in daxctl_dev_alloc_mem function should be free.

If my example program miss something or you need more information please let me know.

[stellarhopper]

@michalbiesek Thanks for those details. Your example code above is correct, but there were two separate problems with the memory object handling in libdaxctl. We were neglecting to free mem_buf in free_mem(), and we were also neglecting to attach newly allocated mem objects to their parent dev objects. This resulted in each invocation of daxctl_dev_get_memory() creating a new mem object, and free_mem() never getting called, thus leaking all the mem objects.

In hindsight, the defensive programming in free_mem() (checking for NULL, and setting dev->mem = NULL) is completely bogus, and should be nuked. It masked the actual problem where dev->mem was never set, and replaced it by a memory leak. We should've let it segfault and this would never have escaped attention :)

I'll add a Reported-by tag for you if you can send me your Full Name <email>, and I'll CC you on the patch.

[stellarhopper]

In hindsight, the defensive programming in free_mem() (checking for NULL, and setting dev->mem = NULL) is completely bogus, and should be nuked.

I spoke too soon, we do need the NULL check, because not every execution path ends up allocating a mem object, and dev->mem also needs to be set to NULL to prevent reusing a freed object.

[michalbiesek]

I'll add a Reported-by tag for you if you can send me your Full Name <email>, and I'll CC you on the patch.

Michal Biesek michal.biesek@intel.com

[stellarhopper]

Fixed in v67.