Re: [PATCH] maple_tree: do not hash pointers on dump in debug mode

From: Liam R. Howlett
Date: Mon Oct 14 2024 - 21:34:17 EST

Next message: Masami Hiramatsu (Google): "[PATCH v16 17/18] Documentation: probes: Update fprobe on function-graph tracer"
Previous message: Masami Hiramatsu (Google): "[PATCH v16 16/18] selftests/ftrace: Add a test case for repeating register/unregister fprobe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [241007 07:53]:
> Many maple tree values output when an mt_validate() or equivalent hits an
> issue utilise tagged pointers, most notably parent nodes. Also some
> pivots/slots contain meaningful values, output as pointers, such as the
> index of the last entry with data for example.
>
> All pointer values such as this are destroyed by kernel pointer hashing
> rendering the debug output obtained from CONFIG_DEBUG_VM_MAPLE_TREE
> considerably less usable.
>
> Update this code to output the raw pointers using %px rather than %p when
> CONFIG_DEBUG_VM_MAPLE_TREE is defined. This is justified, as the use of
> this configuration flag indicates that this is a test environment.
>
> Userland does not understand %px, so use %p there.
>
> In an abundance of caution, if CONFIG_DEBUG_VM_MAPLE_TREE is not set, also
> use %p to avoid exposing raw kernel pointers except when we are positive a
> testing mode is enabled.
>
> This was inspired by the investigation performed in recent debugging
> efforts around a maple tree regression [0] where kernel pointer tagging had
> to be disabled in order to obtain truly meaningful and useful data.
>
> [0]:https://lore.kernel.org/all/20241001023402.3374-1-spasswolf@xxxxxx/
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>

Okay, I used to use the boot option but sure, it's only in debug code.

Reviewed-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>

> ---
> lib/maple_tree.c | 100 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 59 insertions(+), 41 deletions(-)
>
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index 37abf0fe380b..0bcaa1804b79 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -64,6 +64,21 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/maple_tree.h>
>
> +/*
> + * Kernel pointer hashing renders much of the maple tree dump useless as tagged
> + * pointers get hashed to arbitrary values.
> + *
> + * If CONFIG_DEBUG_VM_MAPLE_TREE is set we are in a debug mode where it is
> + * permissible to bypass this. Otherwise remain cautious and retain the hashing.
> + *
> + * Userland doesn't know about %px so also use %p there.
> + */
> +#if defined(__KERNEL__) && defined(CONFIG_DEBUG_VM_MAPLE_TREE)
> +#define PTR_FMT "%px"
> +#else
> +#define PTR_FMT "%p"
> +#endif
> +
> #define MA_ROOT_PARENT 1
>
> /*
> @@ -5414,7 +5429,8 @@ void *mas_store(struct ma_state *mas, void *entry)
> trace_ma_write(__func__, mas, 0, entry);
> #ifdef CONFIG_DEBUG_MAPLE_TREE
> if (MAS_WARN_ON(mas, mas->index > mas->last))
> - pr_err("Error %lX > %lX %p\n", mas->index, mas->last, entry);
> + pr_err("Error %lX > %lX " PTR_FMT "\n", mas->index, mas->last,
> + entry);
>
> if (mas->index > mas->last) {
> mas_set_err(mas, -EINVAL);
> @@ -7119,14 +7135,14 @@ static void mt_dump_entry(void *entry, unsigned long min, unsigned long max,
> mt_dump_range(min, max, depth, format);
>
> if (xa_is_value(entry))
> - pr_cont("value %ld (0x%lx) [%p]\n", xa_to_value(entry),
> - xa_to_value(entry), entry);
> + pr_cont("value %ld (0x%lx) [" PTR_FMT "]\n", xa_to_value(entry),
> + xa_to_value(entry), entry);
> else if (xa_is_zero(entry))
> pr_cont("zero (%ld)\n", xa_to_internal(entry));
> else if (mt_is_reserved(entry))
> - pr_cont("UNKNOWN ENTRY (%p)\n", entry);
> + pr_cont("UNKNOWN ENTRY (" PTR_FMT ")\n", entry);
> else
> - pr_cont("%p\n", entry);
> + pr_cont(PTR_FMT "\n", entry);
> }
>
> static void mt_dump_range64(const struct maple_tree *mt, void *entry,
> @@ -7142,13 +7158,13 @@ static void mt_dump_range64(const struct maple_tree *mt, void *entry,
> for (i = 0; i < MAPLE_RANGE64_SLOTS - 1; i++) {
> switch(format) {
> case mt_dump_hex:
> - pr_cont("%p %lX ", node->slot[i], node->pivot[i]);
> + pr_cont(PTR_FMT " %lX ", node->slot[i], node->pivot[i]);
> break;
> case mt_dump_dec:
> - pr_cont("%p %lu ", node->slot[i], node->pivot[i]);
> + pr_cont(PTR_FMT " %lu ", node->slot[i], node->pivot[i]);
> }
> }
> - pr_cont("%p\n", node->slot[i]);
> + pr_cont(PTR_FMT "\n", node->slot[i]);
> for (i = 0; i < MAPLE_RANGE64_SLOTS; i++) {
> unsigned long last = max;
>
> @@ -7170,11 +7186,11 @@ static void mt_dump_range64(const struct maple_tree *mt, void *entry,
> if (last > max) {
> switch(format) {
> case mt_dump_hex:
> - pr_err("node %p last (%lx) > max (%lx) at pivot %d!\n",
> + pr_err("node " PTR_FMT " last (%lx) > max (%lx) at pivot %d!\n",
> node, last, max, i);
> break;
> case mt_dump_dec:
> - pr_err("node %p last (%lu) > max (%lu) at pivot %d!\n",
> + pr_err("node " PTR_FMT " last (%lu) > max (%lu) at pivot %d!\n",
> node, last, max, i);
> }
> }
> @@ -7204,13 +7220,13 @@ static void mt_dump_arange64(const struct maple_tree *mt, void *entry,
> for (i = 0; i < MAPLE_ARANGE64_SLOTS - 1; i++) {
> switch (format) {
> case mt_dump_hex:
> - pr_cont("%p %lX ", node->slot[i], node->pivot[i]);
> + pr_cont(PTR_FMT " %lX ", node->slot[i], node->pivot[i]);
> break;
> case mt_dump_dec:
> - pr_cont("%p %lu ", node->slot[i], node->pivot[i]);
> + pr_cont(PTR_FMT " %lu ", node->slot[i], node->pivot[i]);
> }
> }
> - pr_cont("%p\n", node->slot[i]);
> + pr_cont(PTR_FMT "\n", node->slot[i]);
> for (i = 0; i < MAPLE_ARANGE64_SLOTS; i++) {
> unsigned long last = max;
>
> @@ -7229,11 +7245,11 @@ static void mt_dump_arange64(const struct maple_tree *mt, void *entry,
> if (last > max) {
> switch(format) {
> case mt_dump_hex:
> - pr_err("node %p last (%lx) > max (%lx) at pivot %d!\n",
> + pr_err("node " PTR_FMT " last (%lx) > max (%lx) at pivot %d!\n",
> node, last, max, i);
> break;
> case mt_dump_dec:
> - pr_err("node %p last (%lu) > max (%lu) at pivot %d!\n",
> + pr_err("node " PTR_FMT " last (%lu) > max (%lu) at pivot %d!\n",
> node, last, max, i);
> }
> }
> @@ -7251,8 +7267,8 @@ static void mt_dump_node(const struct maple_tree *mt, void *entry,
>
> mt_dump_range(min, max, depth, format);
>
> - pr_cont("node %p depth %d type %d parent %p", node, depth, type,
> - node ? node->parent : NULL);
> + pr_cont("node " PTR_FMT " depth %d type %d parent " PTR_FMT, node,
> + depth, type, node ? node->parent : NULL);
> switch (type) {
> case maple_dense:
> pr_cont("\n");
> @@ -7280,7 +7296,7 @@ void mt_dump(const struct maple_tree *mt, enum mt_dump_format format)
> {
> void *entry = rcu_dereference_check(mt->ma_root, mt_locked(mt));
>
> - pr_info("maple_tree(%p) flags %X, height %u root %p\n",
> + pr_info("maple_tree(" PTR_FMT ") flags %X, height %u root " PTR_FMT "\n",
> mt, mt->ma_flags, mt_height(mt), entry);
> if (!xa_is_node(entry))
> mt_dump_entry(entry, 0, 0, 0, format);
> @@ -7332,7 +7348,7 @@ static void mas_validate_gaps(struct ma_state *mas)
> MT_BUG_ON(mas->tree, !entry);
>
> if (gap > p_end - p_start + 1) {
> - pr_err("%p[%u] %lu >= %lu - %lu + 1 (%lu)\n",
> + pr_err(PTR_FMT "[%u] %lu >= %lu - %lu + 1 (%lu)\n",
> mas_mn(mas), i, gap, p_end, p_start,
> p_end - p_start + 1);
> MT_BUG_ON(mas->tree, gap > p_end - p_start + 1);
> @@ -7352,19 +7368,19 @@ static void mas_validate_gaps(struct ma_state *mas)
> MT_BUG_ON(mas->tree, !gaps);
> offset = ma_meta_gap(node);
> if (offset > i) {
> - pr_err("gap offset %p[%u] is invalid\n", node, offset);
> + pr_err("gap offset " PTR_FMT "[%u] is invalid\n", node, offset);
> MT_BUG_ON(mas->tree, 1);
> }
>
> if (gaps[offset] != max_gap) {
> - pr_err("gap %p[%u] is not the largest gap %lu\n",
> + pr_err("gap " PTR_FMT "[%u] is not the largest gap %lu\n",
> node, offset, max_gap);
> MT_BUG_ON(mas->tree, 1);
> }
>
> for (i++ ; i < mt_slot_count(mte); i++) {
> if (gaps[i] != 0) {
> - pr_err("gap %p[%u] beyond node limit != 0\n",
> + pr_err("gap " PTR_FMT "[%u] beyond node limit != 0\n",
> node, i);
> MT_BUG_ON(mas->tree, 1);
> }
> @@ -7378,7 +7394,7 @@ static void mas_validate_gaps(struct ma_state *mas)
> p_mn = mte_parent(mte);
> MT_BUG_ON(mas->tree, max_gap > mas->max);
> if (ma_gaps(p_mn, mas_parent_type(mas, mte))[p_slot] != max_gap) {
> - pr_err("gap %p[%u] != %lu\n", p_mn, p_slot, max_gap);
> + pr_err("gap " PTR_FMT "[%u] != %lu\n", p_mn, p_slot, max_gap);
> mt_dump(mas->tree, mt_dump_hex);
> MT_BUG_ON(mas->tree, 1);
> }
> @@ -7408,11 +7424,11 @@ static void mas_validate_parent_slot(struct ma_state *mas)
> node = mas_slot(mas, slots, i);
> if (i == p_slot) {
> if (node != mas->node)
> - pr_err("parent %p[%u] does not have %p\n",
> + pr_err("parent " PTR_FMT "[%u] does not have " PTR_FMT "\n",
> parent, i, mas_mn(mas));
> MT_BUG_ON(mas->tree, node != mas->node);
> } else if (node == mas->node) {
> - pr_err("Invalid child %p at parent %p[%u] p_slot %u\n",
> + pr_err("Invalid child " PTR_FMT " at parent " PTR_FMT "[%u] p_slot %u\n",
> mas_mn(mas), parent, i, p_slot);
> MT_BUG_ON(mas->tree, node == mas->node);
> }
> @@ -7434,20 +7450,20 @@ static void mas_validate_child_slot(struct ma_state *mas)
> child = mas_slot(mas, slots, i);
>
> if (!child) {
> - pr_err("Non-leaf node lacks child at %p[%u]\n",
> + pr_err("Non-leaf node lacks child at " PTR_FMT "[%u]\n",
> mas_mn(mas), i);
> MT_BUG_ON(mas->tree, 1);
> }
>
> if (mte_parent_slot(child) != i) {
> - pr_err("Slot error at %p[%u]: child %p has pslot %u\n",
> + pr_err("Slot error at " PTR_FMT "[%u]: child " PTR_FMT " has pslot %u\n",
> mas_mn(mas), i, mte_to_node(child),
> mte_parent_slot(child));
> MT_BUG_ON(mas->tree, 1);
> }
>
> if (mte_parent(child) != mte_to_node(mas->node)) {
> - pr_err("child %p has parent %p not %p\n",
> + pr_err("child " PTR_FMT " has parent " PTR_FMT " not " PTR_FMT "\n",
> mte_to_node(child), mte_parent(child),
> mte_to_node(mas->node));
> MT_BUG_ON(mas->tree, 1);
> @@ -7477,24 +7493,24 @@ static void mas_validate_limits(struct ma_state *mas)
> piv = mas_safe_pivot(mas, pivots, i, type);
>
> if (!piv && (i != 0)) {
> - pr_err("Missing node limit pivot at %p[%u]",
> + pr_err("Missing node limit pivot at " PTR_FMT "[%u]",
> mas_mn(mas), i);
> MAS_WARN_ON(mas, 1);
> }
>
> if (prev_piv > piv) {
> - pr_err("%p[%u] piv %lu < prev_piv %lu\n",
> + pr_err(PTR_FMT "[%u] piv %lu < prev_piv %lu\n",
> mas_mn(mas), i, piv, prev_piv);
> MAS_WARN_ON(mas, piv < prev_piv);
> }
>
> if (piv < mas->min) {
> - pr_err("%p[%u] %lu < %lu\n", mas_mn(mas), i,
> + pr_err(PTR_FMT "[%u] %lu < %lu\n", mas_mn(mas), i,
> piv, mas->min);
> MAS_WARN_ON(mas, piv < mas->min);
> }
> if (piv > mas->max) {
> - pr_err("%p[%u] %lu > %lu\n", mas_mn(mas), i,
> + pr_err(PTR_FMT "[%u] %lu > %lu\n", mas_mn(mas), i,
> piv, mas->max);
> MAS_WARN_ON(mas, piv > mas->max);
> }
> @@ -7504,7 +7520,7 @@ static void mas_validate_limits(struct ma_state *mas)
> }
>
> if (mas_data_end(mas) != i) {
> - pr_err("node%p: data_end %u != the last slot offset %u\n",
> + pr_err("node" PTR_FMT ": data_end %u != the last slot offset %u\n",
> mas_mn(mas), mas_data_end(mas), i);
> MT_BUG_ON(mas->tree, 1);
> }
> @@ -7513,8 +7529,8 @@ static void mas_validate_limits(struct ma_state *mas)
> void *entry = mas_slot(mas, slots, i);
>
> if (entry && (i != mt_slots[type] - 1)) {
> - pr_err("%p[%u] should not have entry %p\n", mas_mn(mas),
> - i, entry);
> + pr_err(PTR_FMT "[%u] should not have entry " PTR_FMT "\n",
> + mas_mn(mas), i, entry);
> MT_BUG_ON(mas->tree, entry != NULL);
> }
>
> @@ -7524,7 +7540,7 @@ static void mas_validate_limits(struct ma_state *mas)
> if (!piv)
> continue;
>
> - pr_err("%p[%u] should not have piv %lu\n",
> + pr_err(PTR_FMT "[%u] should not have piv %lu\n",
> mas_mn(mas), i, piv);
> MAS_WARN_ON(mas, i < mt_pivots[type] - 1);
> }
> @@ -7549,7 +7565,7 @@ static void mt_validate_nulls(struct maple_tree *mt)
> do {
> entry = mas_slot(&mas, slots, offset);
> if (!last && !entry) {
> - pr_err("Sequential nulls end at %p[%u]\n",
> + pr_err("Sequential nulls end at " PTR_FMT "[%u]\n",
> mas_mn(&mas), offset);
> }
> MT_BUG_ON(mt, !last && !entry);
> @@ -7591,7 +7607,8 @@ void mt_validate(struct maple_tree *mt)
> end = mas_data_end(&mas);
> if (MAS_WARN_ON(&mas, (end < mt_min_slot_count(mas.node)) &&
> (mas.max != ULONG_MAX))) {
> - pr_err("Invalid size %u of %p\n", end, mas_mn(&mas));
> + pr_err("Invalid size %u of " PTR_FMT "\n",
> + end, mas_mn(&mas));
> }
>
> mas_validate_parent_slot(&mas);
> @@ -7607,7 +7624,8 @@ EXPORT_SYMBOL_GPL(mt_validate);
>
> void mas_dump(const struct ma_state *mas)
> {
> - pr_err("MAS: tree=%p enode=%p ", mas->tree, mas->node);
> + pr_err("MAS: tree=" PTR_FMT " enode=" PTR_FMT " ",
> + mas->tree, mas->node);
> switch (mas->status) {
> case ma_active:
> pr_err("(ma_active)");
> @@ -7671,7 +7689,7 @@ void mas_dump(const struct ma_state *mas)
>
> pr_err("[%u/%u] index=%lx last=%lx\n", mas->offset, mas->end,
> mas->index, mas->last);
> - pr_err(" min=%lx max=%lx alloc=%p, depth=%u, flags=%x\n",
> + pr_err(" min=%lx max=%lx alloc=" PTR_FMT ", depth=%u, flags=%x\n",
> mas->min, mas->max, mas->alloc, mas->depth, mas->mas_flags);
> if (mas->index > mas->last)
> pr_err("Check index & last\n");
> @@ -7680,7 +7698,7 @@ EXPORT_SYMBOL_GPL(mas_dump);
>
> void mas_wr_dump(const struct ma_wr_state *wr_mas)
> {
> - pr_err("WR_MAS: node=%p r_min=%lx r_max=%lx\n",
> + pr_err("WR_MAS: node=" PTR_FMT " r_min=%lx r_max=%lx\n",
> wr_mas->node, wr_mas->r_min, wr_mas->r_max);
> pr_err(" type=%u off_end=%u, node_end=%u, end_piv=%lx\n",
> wr_mas->type, wr_mas->offset_end, wr_mas->mas->end,
> --
> 2.46.2
>

Next message: Masami Hiramatsu (Google): "[PATCH v16 17/18] Documentation: probes: Update fprobe on function-graph tracer"
Previous message: Masami Hiramatsu (Google): "[PATCH v16 16/18] selftests/ftrace: Add a test case for repeating register/unregister fprobe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]