lkml.org 
[lkml]   [2022]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] assoc_array: Fix BUG_ON during garbage collect
Date
Stephen Brennan <stephen.s.brennan@oracle.com> writes:
> A rare BUG_ON triggered in assoc_array_gc:
>
> [3430308.818153] kernel BUG at lib/assoc_array.c:1609!
>
> Which corresponded to the statement currently at line 1593 upstream:
>
> BUG_ON(assoc_array_ptr_is_meta(p));
>
> Using the data from the core dump, I was able to generate a userspace
> reproducer[1] and determine the cause of the bug.
>
> [1]: https://github.com/brenns10/kernel_stuff/tree/master/assoc_array_gc
>
> After running the iterator on the entire branch, an internal tree node
> looked like the following:
>
> NODE (nr_leaves_on_branch: 3)
> SLOT [0] NODE (2 leaves)
> SLOT [1] NODE (1 leaf)
> SLOT [2..f] NODE (empty)
>
> In the userspace reproducer, the pr_devel output when compressing this
> node was:
>
> -- compress node 0x5607cc089380 --
> free=0, leaves=0
> [0] retain node 2/1 [nx 0]
> [1] fold node 1/1 [nx 0]
> [2] fold node 0/1 [nx 2]
> [3] fold node 0/2 [nx 2]
> [4] fold node 0/3 [nx 2]
> [5] fold node 0/4 [nx 2]
> [6] fold node 0/5 [nx 2]
> [7] fold node 0/6 [nx 2]
> [8] fold node 0/7 [nx 2]
> [9] fold node 0/8 [nx 2]
> [10] fold node 0/9 [nx 2]
> [11] fold node 0/10 [nx 2]
> [12] fold node 0/11 [nx 2]
> [13] fold node 0/12 [nx 2]
> [14] fold node 0/13 [nx 2]
> [15] fold node 0/14 [nx 2]
> after: 3
>
> At slot 0, an internal node with 2 leaves could not be folded into the
> node, because there was only one available slot (slot 0). Thus, the
> internal node was retained. At slot 1, the node had one leaf, and was
> able to be folded in successfully. The remaining nodes had no leaves,
> and so were removed. By the end of the compression stage, there were 14
> free slots, and only 3 leaf nodes. The tree was ascended and then its
> parent node was compressed. When this node was seen, it could not be
> folded, due to the internal node it contained.
>
> The invariant for compression in this function is: whenever
> nr_leaves_on_branch < ASSOC_ARRAY_FAN_OUT, the node should contain all
> leaf nodes. The compression step currently cannot guarantee this, given
> the corner case shown above.
>
> To fix this issue, retry compression whenever we have retained a node,
> and yet nr_leaves_on_branch < ASSOC_ARRAY_FAN_OUT. This second
> compression will then allow the node in slot 1 to be folded in,
> satisfying the invariant. Below is the output of the reproducer once the
> fix is applied:
>
> -- compress node 0x560e9c562380 --
> free=0, leaves=0
> [0] retain node 2/1 [nx 0]
> [1] fold node 1/1 [nx 0]
> [2] fold node 0/1 [nx 2]
> [3] fold node 0/2 [nx 2]
> [4] fold node 0/3 [nx 2]
> [5] fold node 0/4 [nx 2]
> [6] fold node 0/5 [nx 2]
> [7] fold node 0/6 [nx 2]
> [8] fold node 0/7 [nx 2]
> [9] fold node 0/8 [nx 2]
> [10] fold node 0/9 [nx 2]
> [11] fold node 0/10 [nx 2]
> [12] fold node 0/11 [nx 2]
> [13] fold node 0/12 [nx 2]
> [14] fold node 0/13 [nx 2]
> [15] fold node 0/14 [nx 2]
> internal nodes remain despite enough space, retrying
> -- compress node 0x560e9c562380 --
> free=14, leaves=1
> [0] fold node 2/15 [nx 0]
> after: 3
>
> Fixes: 3cb989501c26 ("Add a generic associative array implementation.")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
>
> Hi Andrew,
>
> As far as I can tell, lib/assoc_array.c has no maintainer, so I'm
> sending this bugfix to you. Hopefully David can take a look at this and
> verify sure it's all sane. I tested it on my userspace reproducer, and
> also by booting it and exercising the keyring_gc functions a bit.
>
> Thanks,
> Stephen
>
> lib/assoc_array.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/lib/assoc_array.c b/lib/assoc_array.c
> index 079c72e26493..7ed20233a770 100644
> --- a/lib/assoc_array.c
> +++ b/lib/assoc_array.c
> @@ -1462,6 +1462,7 @@ int assoc_array_gc(struct assoc_array *array,
> struct assoc_array_ptr *new_root, *new_parent, **new_ptr_pp;
> unsigned long nr_leaves_on_tree;
> int keylen, slot, nr_free, next_slot, i;
> + bool retained;
>
> pr_devel("-->%s()\n", __func__);
>
> @@ -1536,6 +1537,7 @@ int assoc_array_gc(struct assoc_array *array,
> goto descend;
> }
>
> +retry_compress:
> pr_devel("-- compress node %p --\n", new_n);
>
> /* Count up the number of empty slots in this node and work out the
> @@ -1554,6 +1556,7 @@ int assoc_array_gc(struct assoc_array *array,
>
> /* See what we can fold in */
> next_slot = 0;
> + retained = 0;
> for (slot = 0; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
> struct assoc_array_shortcut *s;
> struct assoc_array_node *child;
> @@ -1602,9 +1605,14 @@ int assoc_array_gc(struct assoc_array *array,
> pr_devel("[%d] retain node %lu/%d [nx %d]\n",
> slot, child->nr_leaves_on_branch, nr_free + 1,
> next_slot);
> + retained = true;
> }
> }
>
> + if (retained && new_n->nr_leaves_on_branch < ASSOC_ARRAY_FAN_OUT) {

Hi Andrew,

I'm sorry to ask this of you - but would you be willing to yank this
patch out and replace it with the v2 I'll send in reply to this? The
above test should be a "<=" condition, not a "<". And of course you
caught my spelling mistake on the line below. Both will be corrected in
the v2.

thanks,
Stephen

> + pr_devel("internal nodes remain despite neough space, retrying\n");
> + goto retry_compress;
> + }
> pr_devel("after: %lu\n", new_n->nr_leaves_on_branch);
>
> nr_leaves_on_tree = new_n->nr_leaves_on_branch;
> --
> 2.30.2

\
 
 \ /
  Last update: 2022-05-12 23:52    [W:0.056 / U:0.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site