libdaxctl: Fix multi-range memory block handling

A failed sysfs read can silently truncate a device's mapping list, leaving callers with incomplete range information and potentially incorrect memory block accounting. An allocation failure also causes an infinite loop, hanging the caller.

Distinguish the end of the mapping list from read failures, reject incomplete lists, and discard partial results on error. The mappings describe the device's constituent ranges, so validate that their combined size matches the device size. Return -ENOMEM on allocation failure.

Fixes 5a31bf710399 ("libdaxctl: add mapping iterator APIs")

Signed-off-by Alison Schofield <[email protected]>

A failed sysfs read can silently truncate a device's mapping list, leaving callers with incomplete range information and potentially incorrect memory block accounting. An allocation failure also causes an infinite loop, hanging the caller.

Distinguish the end of the mapping list from read failures, reject incomplete lists, and discard partial results on error. The mappings describe the device's constituent ranges, so validate that their combined size matches the device size. Return -ENOMEM on allocation failure.

Fixes 5a31bf710399 ("libdaxctl: add mapping iterator APIs")

Signed-off-by Alison Schofield <[email protected]>

---
 daxctl/lib/libdaxctl.c | 70 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 12 deletions(-)

daxctl/lib/libdaxctl.c +58 -12

index 5b47c770bd15..f630f4791f59 100644
@@ -222,6 +222,17 @@ static void free_mem(struct daxctl_dev *dev)
 	}
 }
 
+static void free_mappings(struct daxctl_dev *dev)
+{
+	struct daxctl_mapping *mapping, *_m;
+
+	list_for_each_safe(&dev->mappings, mapping, _m, list) {
+		list_del_from(&dev->mappings, &mapping->list);
+		free(mapping);
+	}
+	dev->num_mappings = -1;
+}
+
 static void free_dev(struct daxctl_dev *dev, struct list_head *head)
 {
 	if (head)

@@ -1269,15 +1280,17 @@ DAXCTL_EXPORT unsigned long daxctl_memory_get_block_size(struct daxctl_memory *m
 	return mem->block_size;
 }
 
-static void mappings_init(struct daxctl_dev *dev)
+static int mappings_init(struct daxctl_dev *dev)
 {
+	const char *devname = daxctl_dev_get_devname(dev);
 	struct daxctl_ctx *ctx = daxctl_dev_get_ctx(dev);
+	unsigned long long size = 0;
 	char buf[SYSFS_ATTR_SIZE];
 	char *path = dev->dev_buf;
-	int i;
+	int i, rc;
 
 	if (dev->num_mappings != -1)
-		return;
+		return 0;
 
 	dev->num_mappings = 0;
 	for (;;) {

@@ -1285,31 +1298,43 @@ static void mappings_init(struct daxctl_dev *dev)
 		unsigned long long pgoff, start, end;
 
 		i = dev->num_mappings;
+		sprintf(path, "%s/mapping%d", dev->dev_path, i);
+		if (access(path, F_OK) < 0) {
+			if (errno == ENOENT)
+				break;
+			rc = -errno;
+			err(ctx, "%s: Unable to access mapping%d: %s\n",
+				devname, i, strerror(-rc));
+			goto out;
+		}
+
 		mapping = calloc(1, sizeof(*mapping));
 		if (!mapping) {
-			err(ctx, "%s: mapping%u allocation failure\n",
-				daxctl_dev_get_devname(dev), i);
-			continue;
+			rc = -ENOMEM;
+			goto err;
 		}
 
 		sprintf(path, "%s/mapping%d/start", dev->dev_path, i);
-		if (sysfs_read_attr(ctx, path, buf) < 0) {
+		rc = sysfs_read_attr(ctx, path, buf);
+		if (rc < 0) {
 			free(mapping);
-			break;
+			goto err;
 		}
 		start = strtoull(buf, NULL, 0);
 
 		sprintf(path, "%s/mapping%d/end", dev->dev_path, i);
-		if (sysfs_read_attr(ctx, path, buf) < 0) {
+		rc = sysfs_read_attr(ctx, path, buf);
+		if (rc < 0) {
 			free(mapping);
-			break;
+			goto err;
 		}
 		end = strtoull(buf, NULL, 0);
 
 		sprintf(path, "%s/mapping%d/page_offset", dev->dev_path, i);
-		if (sysfs_read_attr(ctx, path, buf) < 0) {
+		rc = sysfs_read_attr(ctx, path, buf);
+		if (rc < 0) {
 			free(mapping);
-			break;
+			goto err;
 		}
 		pgoff = strtoull(buf, NULL, 0);
 

@@ -1320,9 +1345,30 @@ static void mappings_init(struct daxctl_dev *dev)
 
 		dev->num_mappings++;
 		list_add(&dev->mappings, &mapping->list);
+		size += end - start + 1;
 	}
+
+	/* each of the device's ranges has a mappingN */
+	if (dev->num_mappings && size != dev->size) {
+		err(ctx, "%s: mappings total %#llx, device size %#llx\n",
+			devname, size, dev->size);
+		rc = -ENXIO;
+		goto out;
+	}
+
+	return 0;
+
+err:
+	/* sysfs_read_attr() already logged these */
+	if (rc != -EACCES && rc != -EPERM)
+		err(ctx, "%s: Unable to read mapping%d: %s\n", devname, i,
+			strerror(-rc));
+out:
+	free_mappings(dev);
+	return rc;
 }
 
+/* NULL means either no mappings or a failure to read them */
 DAXCTL_EXPORT struct daxctl_mapping *daxctl_mapping_get_first(struct daxctl_dev *dev)
 {
 	mappings_init(dev);

Cached mappings are not freed when a device is destroyed or invalidated when its ranges change. This leaks memory and can leave callers using obsolete physical address ranges, potentially operating on memory blocks that no longer belong to the device.

Free mappings on device destruction and invalidate the cache when device ranges may have changed.

Fixes 5a31bf710399 ("libdaxctl: add mapping iterator APIs")

Fixes 52a2653098a5 ("libdaxctl: add daxctl_dev_set_mapping()")

Signed-off-by Alison Schofield <[email protected]>

Cached mappings are not freed when a device is destroyed or invalidated when its ranges change. This leaks memory and can leave callers using obsolete physical address ranges, potentially operating on memory blocks that no longer belong to the device.

Free mappings on device destruction and invalidate the cache when device ranges may have changed.

Fixes 5a31bf710399 ("libdaxctl: add mapping iterator APIs")

Fixes 52a2653098a5 ("libdaxctl: add daxctl_dev_set_mapping()")

Signed-off-by Alison Schofield <[email protected]>

---
 daxctl/lib/libdaxctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

daxctl/lib/libdaxctl.c +6 -2

index f630f4791f59..f7cbf3fa12e7 100644
@@ -241,6 +241,7 @@ static void free_dev(struct daxctl_dev *dev, struct list_head *head)
 	free(dev->dev_buf);
 	free(dev->dev_path);
 	free_mem(dev);
+	free_mappings(dev);
 	free(dev);
 }
 

@@ -593,14 +594,15 @@ static void *add_dax_dev(void *parent, int id, const char *daxdev_base)
 	else
 		dev->target_node = -1;
 
+	dev->num_mappings = -1;
+	list_head_init(&dev->mappings);
+
 	daxctl_dev_foreach(region, dev_dup)
 		if (dev_dup->id == dev->id) {
 			free_dev(dev, NULL);
 			free(path);
 			return dev_dup;
 		}
-	dev->num_mappings = -1;
-	list_head_init(&dev->mappings);
 	list_add(&region->devices, &dev->list);
 	free(path);
 	return dev;

@@ -1186,6 +1188,7 @@ DAXCTL_EXPORT int daxctl_dev_set_size(struct daxctl_dev *dev, unsigned long long
 	}
 
 	sprintf(buf, "%#llx\n", size);
+	free_mappings(dev);
 	if (sysfs_write_attr(ctx, path, buf) < 0) {
 		err(ctx, "%s: failed to set size\n",
 				daxctl_dev_get_devname(dev));

@@ -1242,6 +1245,7 @@ DAXCTL_EXPORT int daxctl_dev_set_mapping(struct daxctl_dev *dev,
 	}
 
 	sprintf(buf, "%#llx-%#llx\n", start, end);
+	free_mappings(dev);
 	if (sysfs_write_attr(ctx, path, buf) < 0) {
 		err(ctx, "%s: failed to set mapping\n",
 				daxctl_dev_get_devname(dev));

memblock_in_dev() treats a DAX device as one contiguous physical address range, even when the device has multiple ranges separated by gaps. This misses blocks in later ranges and may incorrectly include blocks belonging to another device.

Memory block counts and per-block online/offline operations use this lookup, so daxctl may operate on memory outside the requested device.

Match blocks against each individual mapping, retaining the single-range fallback for older kernels.

Fixes e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")

Signed-off-by Alison Schofield <[email protected]>

---
 daxctl/lib/libdaxctl.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Quoted 8 lines · Alison Schofield

memblock_in_dev() treats a DAX device as one contiguous physical address range, even when the device has multiple ranges separated by gaps. This misses blocks in later ranges and may incorrectly include blocks belonging to another device.

Memory block counts and per-block online/offline operations use this lookup, so daxctl may operate on memory outside the requested device.

Quoted 9 lines

Match blocks against each individual mapping, retaining the single-range fallback for older kernels.

Fixes e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")

Signed-off-by Alison Schofield <[email protected]>

---
 daxctl/lib/libdaxctl.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Ran through an analysis and testing round with my clanker, seems to want this fixup. Looked reasonable, just passing it along.

Will get proper tags for you this week.

-- >8 --

Counting an unplugged device's blocks can return -ENOENT when its node still has unrelated RAM. mappings_init() leaves ENOENT after probing past the final mapping, and unrelated blocks take continue before the scan resets errno. End-of-directory can preserve that stale error.

Clear errno immediately before each readdir() call so the final errno check describes that read alone. Errors from block operations still exit through out_dir with their saved return value.

This corrects "libdaxctl: match memory blocks against each dax device mapping". Returning real node-directory entries with memory entries last reproduces the error in the posted series and verifies the fix.

---
 daxctl/lib/libdaxctl.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

daxctl/lib/libdaxctl.c +2 -3

index e106fff..c154db9 100644
@@ -1716,8 +1716,8 @@ static int daxctl_memory_op(struct daxctl_memory *mem, enum memory_op op)
 		return rc;
 	}
 
-	errno = 0;
-	while ((de = readdir(node_dir)) != NULL) {
+	/* Helpers may leave errno set even when a block is skipped. */
+	for (errno = 0; (de = readdir(node_dir)) != NULL; errno = 0) {
 		if (strncmp(de->d_name, "memory", 6) == 0) {
 			if (strncmp(de->d_name, "memory_", 7) == 0)
 				continue;

@@ -1734,7 +1734,6 @@ static int daxctl_memory_op(struct daxctl_memory *mem, enum memory_op op)
 			if (rc == 0)
 				count++;
 		}
-		errno = 0;
 	}
 
 	if (status_flags & MEM_ST_ZONE_INCONSISTENT)

Signature · 2 lines

-- 
Gregory Price (Meta) <[email protected]>

Existing tests use single-range devices and cannot detect incorrect memory block accounting across discontiguous mappings. A two-range device exposed this bug by reporting only one of its two blocks.

Create a device with two separated mappings and verify its memory block count against the expected count from both mappings.

Signed-off-by Alison Schofield <[email protected]>

Existing tests use single-range devices and cannot detect incorrect memory block accounting across discontiguous mappings. A two-range device exposed this bug by reporting only one of its two blocks.

Create a device with two separated mappings and verify its memory block count against the expected count from both mappings.

Signed-off-by Alison Schofield <[email protected]>

---
 test/daxctl-create.sh | 58 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

test/daxctl-create.sh +58 -0

index 954a112d0f18..c4fe369eac3a 100755
@@ -7,6 +7,9 @@ rc=77
 
 trap 'cleanup $LINENO' ERR
 
+policy=/sys/devices/system/memory/auto_online_blocks
+saved_policy=
+
 modprobe -r cxl_test
 modprobe cxl_test
 $CXL list

@@ -14,6 +17,7 @@ $CXL list
 cleanup()
 {
 	printf "Error at line %d\n" "$1"
+	[[ $saved_policy ]] && echo "$saved_policy" > "$policy"
 	[[ $testdev ]] && reset_dax
 	exit $rc
 }

@@ -396,6 +400,59 @@ daxctl_test7()
 	test_pass
 }
 
+# Test 8: multi-range system-ram
+# Successfully counts every memory block of a device whose two ranges
+# have a gap between them. The memory is never onlined: cxl_test memory
+# is not backed by RAM.
+daxctl_test8()
+{
+	local daxdev_1 daxdev_2 daxdev path m start end n nmaps=0 total=0
+	local bs size
+
+	bs=$((0x$(cat /sys/devices/system/memory/block_size_bytes)))
+	size=$((available / 4))
+	if (( size < bs || size % bs )); then
+		echo "test8: skipped, region too small for $bs byte memory blocks"
+		return 0
+	fi
+
+	# a devdax device in the gap between the two ranges, it has no
+	# memory blocks to be miscounted
+	daxdev_1=$("$DAXCTL" create-device -r "$region_id" -s $size | jq -er '.[].chardev')
+	daxdev_2=$("$DAXCTL" create-device -r "$region_id" -s $size | jq -er '.[].chardev')
+	test -n "$daxdev_2"
+	"$DAXCTL" disable-device "$daxdev_1" && "$DAXCTL" destroy-device "$daxdev_1"
+
+	daxdev=$("$DAXCTL" create-device -r "$region_id" -s $((size * 2)) | jq -er '.[].chardev')
+	test "$(daxctl_get_nr_mappings "$daxdev")" -eq 2
+
+	# memory blocks wholly inside each mapping
+	path=$(readlink -f /sys/bus/dax/devices/"$daxdev")
+	for m in "$path"/mapping[0-9]*; do
+		start=$(cat "$m"/start)
+		end=$(cat "$m"/end)
+		n=$(((end + 1) / bs - (start + bs - 1) / bs))
+		(( n > 0 ))
+		total=$((total + n))
+		nmaps=$((nmaps + 1))
+	done
+	test "$nmaps" -eq 2
+
+	saved_policy=$(cat "$policy")
+	echo offline > "$policy"
+	"$DAXCTL" reconfigure-device -N -m system-ram "$daxdev"
+	test "$("$DAXCTL" list -d "$daxdev" | jq -er '.[].total_memblocks')" -eq "$total"
+	"$DAXCTL" reconfigure-device -m devdax "$daxdev"
+	echo "$saved_policy" > "$policy"
+	saved_policy=
+
+	"$DAXCTL" disable-device -r "$region_id" all
+	"$DAXCTL" destroy-device -r "$region_id" all
+
+	clear_dev
+	test_pass
+}
+
 find_testdev
 rc=1
 setup_dev

@@ -407,6 +464,7 @@ daxctl_test4
 daxctl_test5
 daxctl_test6
 daxctl_test7
+daxctl_test8
 reset_dev
 modprobe -r cxl_test
 exit 0

[ Gregory: Hoping you can help w reviews here ]