diff for duplicates of <20160715190040.GA7195@linux.intel.com> diff --git a/a/1.txt b/N1/1.txt index d4a5f04..66f42dc 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -85,3 +85,77 @@ I've run this patch in my test setup, and it fixes the reproducer provided by Dmitry. I've also run xfstests against it with out any failures. --- 8< --- +>From 533beefac12f61f467aeb72e2d2c46685247b9bc Mon Sep 17 00:00:00 2001 +From: Ross Zwisler <ross.zwisler@linux.intel.com> +Date: Fri, 15 Jul 2016 12:46:38 -0600 +Subject: [PATCH] radix-tree: 'slot' can be NULL in radix_tree_next_slot() + +There are four cases I can see where we could end up with a NULL 'slot' in +radix_tree_next_slot() (there might be more): + +1) radix_tree_iter_retry() via a non-tagged iteration like +radix_tree_for_each_slot(). In this case we currently aren't seeing a bug +because radix_tree_iter_retry() sets + + iter->next_index = iter->index; + +which means that in in the else case in radix_tree_next_slot(), 'count' is +zero, so we skip over the while() loop and effectively just return NULL +without ever dereferencing 'slot'. + +2) radix_tree_iter_retry() via tagged iteration like +radix_tree_for_each_tagged(). With the current code this case is +unhandled and we have seen it result in a kernel crash when we dereference +'slot'. + +3) radix_tree_iter_next() via via a non-tagged iteration like +radix_tree_for_each_slot(). This currently happens in shmem_tag_pins() +and shmem_partial_swap_usage(). + +I think that this case is currently unhandled. Unlike with +radix_tree_iter_retry() case (#1 above) we can't rely on 'count' in the else +case of radix_tree_next_slot() to be zero, so I think it's possible we'll end +up executing code in the while() loop in radix_tree_next_slot() that assumes +'slot' is valid. + +I haven't actually seen this crash on a test setup, but I don't think the +current code is safe. + +4) radix_tree_iter_next() via tagged iteration like +radix_tree_for_each_tagged(). This happens in shmem_wait_for_pins(). + +radix_tree_iter_next() zeros out iter->tags, so we end up exiting +radix_tree_next_slot() here: + + if (flags & RADIX_TREE_ITER_TAGGED) { + void *canon = slot; + + iter->tags >>= 1; + if (unlikely(!iter->tags)) + return NULL; + +Really we want to guarantee that we just bail out of +radix_tree_next_slot() if we have a NULL 'slot'. This is a more explicit +way of handling all the 4 above cases. + +Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> +--- + include/linux/radix-tree.h | 3 +++ + 1 file changed, 3 insertions(+) + +diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h +index cb4b7e8..840308d 100644 +--- a/include/linux/radix-tree.h ++++ b/include/linux/radix-tree.h +@@ -463,6 +463,9 @@ static inline struct radix_tree_node *entry_to_node(void *ptr) + static __always_inline void ** + radix_tree_next_slot(void **slot, struct radix_tree_iter *iter, unsigned flags) + { ++ if (unlikely(!slot)) ++ return NULL; ++ + if (flags & RADIX_TREE_ITER_TAGGED) { + void *canon = slot; + +-- +2.9.0 diff --git a/a/content_digest b/N1/content_digest index 10452a0..aad3b2f 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -110,6 +110,80 @@ "I've run this patch in my test setup, and it fixes the reproducer provided by\n" "Dmitry. I've also run xfstests against it with out any failures.\n" "\n" - --- 8< --- + "--- 8< ---\n" + ">From 533beefac12f61f467aeb72e2d2c46685247b9bc Mon Sep 17 00:00:00 2001\n" + "From: Ross Zwisler <ross.zwisler@linux.intel.com>\n" + "Date: Fri, 15 Jul 2016 12:46:38 -0600\n" + "Subject: [PATCH] radix-tree: 'slot' can be NULL in radix_tree_next_slot()\n" + "\n" + "There are four cases I can see where we could end up with a NULL 'slot' in\n" + "radix_tree_next_slot() (there might be more):\n" + "\n" + "1) radix_tree_iter_retry() via a non-tagged iteration like\n" + "radix_tree_for_each_slot(). In this case we currently aren't seeing a bug\n" + "because radix_tree_iter_retry() sets\n" + "\n" + "\titer->next_index = iter->index;\n" + "\n" + "which means that in in the else case in radix_tree_next_slot(), 'count' is\n" + "zero, so we skip over the while() loop and effectively just return NULL\n" + "without ever dereferencing 'slot'.\n" + "\n" + "2) radix_tree_iter_retry() via tagged iteration like\n" + "radix_tree_for_each_tagged(). With the current code this case is\n" + "unhandled and we have seen it result in a kernel crash when we dereference\n" + "'slot'.\n" + "\n" + "3) radix_tree_iter_next() via via a non-tagged iteration like\n" + "radix_tree_for_each_slot(). This currently happens in shmem_tag_pins()\n" + "and shmem_partial_swap_usage().\n" + "\n" + "I think that this case is currently unhandled. Unlike with\n" + "radix_tree_iter_retry() case (#1 above) we can't rely on 'count' in the else\n" + "case of radix_tree_next_slot() to be zero, so I think it's possible we'll end\n" + "up executing code in the while() loop in radix_tree_next_slot() that assumes\n" + "'slot' is valid.\n" + "\n" + "I haven't actually seen this crash on a test setup, but I don't think the\n" + "current code is safe.\n" + "\n" + "4) radix_tree_iter_next() via tagged iteration like\n" + "radix_tree_for_each_tagged(). This happens in shmem_wait_for_pins().\n" + "\n" + "radix_tree_iter_next() zeros out iter->tags, so we end up exiting\n" + "radix_tree_next_slot() here:\n" + "\n" + "\tif (flags & RADIX_TREE_ITER_TAGGED) {\n" + "\t\tvoid *canon = slot;\n" + "\n" + "\t\titer->tags >>= 1;\n" + "\t\tif (unlikely(!iter->tags))\n" + "\t\t\treturn NULL;\n" + "\n" + "Really we want to guarantee that we just bail out of\n" + "radix_tree_next_slot() if we have a NULL 'slot'. This is a more explicit\n" + "way of handling all the 4 above cases.\n" + "\n" + "Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>\n" + "---\n" + " include/linux/radix-tree.h | 3 +++\n" + " 1 file changed, 3 insertions(+)\n" + "\n" + "diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h\n" + "index cb4b7e8..840308d 100644\n" + "--- a/include/linux/radix-tree.h\n" + "+++ b/include/linux/radix-tree.h\n" + "@@ -463,6 +463,9 @@ static inline struct radix_tree_node *entry_to_node(void *ptr)\n" + " static __always_inline void **\n" + " radix_tree_next_slot(void **slot, struct radix_tree_iter *iter, unsigned flags)\n" + " {\n" + "+\tif (unlikely(!slot))\n" + "+\t\treturn NULL;\n" + "+\n" + " \tif (flags & RADIX_TREE_ITER_TAGGED) {\n" + " \t\tvoid *canon = slot;\n" + " \n" + "-- \n" + 2.9.0 -f8bf3843c8532a57187e503115a4114f0f7645a290ce8b501cb57a872dfa998e +0c717be02dd53eebd04fdc1e3c0099e0e55e339b40a562f5848b4cd346ac0dc3
diff --git a/a/1.txt b/N2/1.txt index d4a5f04..d8b3758 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -85,3 +85,83 @@ I've run this patch in my test setup, and it fixes the reproducer provided by Dmitry. I've also run xfstests against it with out any failures. --- 8< --- +>From 533beefac12f61f467aeb72e2d2c46685247b9bc Mon Sep 17 00:00:00 2001 +From: Ross Zwisler <ross.zwisler@linux.intel.com> +Date: Fri, 15 Jul 2016 12:46:38 -0600 +Subject: [PATCH] radix-tree: 'slot' can be NULL in radix_tree_next_slot() + +There are four cases I can see where we could end up with a NULL 'slot' in +radix_tree_next_slot() (there might be more): + +1) radix_tree_iter_retry() via a non-tagged iteration like +radix_tree_for_each_slot(). In this case we currently aren't seeing a bug +because radix_tree_iter_retry() sets + + iter->next_index = iter->index; + +which means that in in the else case in radix_tree_next_slot(), 'count' is +zero, so we skip over the while() loop and effectively just return NULL +without ever dereferencing 'slot'. + +2) radix_tree_iter_retry() via tagged iteration like +radix_tree_for_each_tagged(). With the current code this case is +unhandled and we have seen it result in a kernel crash when we dereference +'slot'. + +3) radix_tree_iter_next() via via a non-tagged iteration like +radix_tree_for_each_slot(). This currently happens in shmem_tag_pins() +and shmem_partial_swap_usage(). + +I think that this case is currently unhandled. Unlike with +radix_tree_iter_retry() case (#1 above) we can't rely on 'count' in the else +case of radix_tree_next_slot() to be zero, so I think it's possible we'll end +up executing code in the while() loop in radix_tree_next_slot() that assumes +'slot' is valid. + +I haven't actually seen this crash on a test setup, but I don't think the +current code is safe. + +4) radix_tree_iter_next() via tagged iteration like +radix_tree_for_each_tagged(). This happens in shmem_wait_for_pins(). + +radix_tree_iter_next() zeros out iter->tags, so we end up exiting +radix_tree_next_slot() here: + + if (flags & RADIX_TREE_ITER_TAGGED) { + void *canon = slot; + + iter->tags >>= 1; + if (unlikely(!iter->tags)) + return NULL; + +Really we want to guarantee that we just bail out of +radix_tree_next_slot() if we have a NULL 'slot'. This is a more explicit +way of handling all the 4 above cases. + +Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> +--- + include/linux/radix-tree.h | 3 +++ + 1 file changed, 3 insertions(+) + +diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h +index cb4b7e8..840308d 100644 +--- a/include/linux/radix-tree.h ++++ b/include/linux/radix-tree.h +@@ -463,6 +463,9 @@ static inline struct radix_tree_node *entry_to_node(void *ptr) + static __always_inline void ** + radix_tree_next_slot(void **slot, struct radix_tree_iter *iter, unsigned flags) + { ++ if (unlikely(!slot)) ++ return NULL; ++ + if (flags & RADIX_TREE_ITER_TAGGED) { + void *canon = slot; + +-- +2.9.0 + +-- +To unsubscribe, send a message with 'unsubscribe linux-mm' in +the body to majordomo@kvack.org. For more info on Linux MM, +see: http://www.linux-mm.org/ . +Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> diff --git a/a/content_digest b/N2/content_digest index 10452a0..4c426d3 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -110,6 +110,86 @@ "I've run this patch in my test setup, and it fixes the reproducer provided by\n" "Dmitry. I've also run xfstests against it with out any failures.\n" "\n" - --- 8< --- + "--- 8< ---\n" + ">From 533beefac12f61f467aeb72e2d2c46685247b9bc Mon Sep 17 00:00:00 2001\n" + "From: Ross Zwisler <ross.zwisler@linux.intel.com>\n" + "Date: Fri, 15 Jul 2016 12:46:38 -0600\n" + "Subject: [PATCH] radix-tree: 'slot' can be NULL in radix_tree_next_slot()\n" + "\n" + "There are four cases I can see where we could end up with a NULL 'slot' in\n" + "radix_tree_next_slot() (there might be more):\n" + "\n" + "1) radix_tree_iter_retry() via a non-tagged iteration like\n" + "radix_tree_for_each_slot(). In this case we currently aren't seeing a bug\n" + "because radix_tree_iter_retry() sets\n" + "\n" + "\titer->next_index = iter->index;\n" + "\n" + "which means that in in the else case in radix_tree_next_slot(), 'count' is\n" + "zero, so we skip over the while() loop and effectively just return NULL\n" + "without ever dereferencing 'slot'.\n" + "\n" + "2) radix_tree_iter_retry() via tagged iteration like\n" + "radix_tree_for_each_tagged(). With the current code this case is\n" + "unhandled and we have seen it result in a kernel crash when we dereference\n" + "'slot'.\n" + "\n" + "3) radix_tree_iter_next() via via a non-tagged iteration like\n" + "radix_tree_for_each_slot(). This currently happens in shmem_tag_pins()\n" + "and shmem_partial_swap_usage().\n" + "\n" + "I think that this case is currently unhandled. Unlike with\n" + "radix_tree_iter_retry() case (#1 above) we can't rely on 'count' in the else\n" + "case of radix_tree_next_slot() to be zero, so I think it's possible we'll end\n" + "up executing code in the while() loop in radix_tree_next_slot() that assumes\n" + "'slot' is valid.\n" + "\n" + "I haven't actually seen this crash on a test setup, but I don't think the\n" + "current code is safe.\n" + "\n" + "4) radix_tree_iter_next() via tagged iteration like\n" + "radix_tree_for_each_tagged(). This happens in shmem_wait_for_pins().\n" + "\n" + "radix_tree_iter_next() zeros out iter->tags, so we end up exiting\n" + "radix_tree_next_slot() here:\n" + "\n" + "\tif (flags & RADIX_TREE_ITER_TAGGED) {\n" + "\t\tvoid *canon = slot;\n" + "\n" + "\t\titer->tags >>= 1;\n" + "\t\tif (unlikely(!iter->tags))\n" + "\t\t\treturn NULL;\n" + "\n" + "Really we want to guarantee that we just bail out of\n" + "radix_tree_next_slot() if we have a NULL 'slot'. This is a more explicit\n" + "way of handling all the 4 above cases.\n" + "\n" + "Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>\n" + "---\n" + " include/linux/radix-tree.h | 3 +++\n" + " 1 file changed, 3 insertions(+)\n" + "\n" + "diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h\n" + "index cb4b7e8..840308d 100644\n" + "--- a/include/linux/radix-tree.h\n" + "+++ b/include/linux/radix-tree.h\n" + "@@ -463,6 +463,9 @@ static inline struct radix_tree_node *entry_to_node(void *ptr)\n" + " static __always_inline void **\n" + " radix_tree_next_slot(void **slot, struct radix_tree_iter *iter, unsigned flags)\n" + " {\n" + "+\tif (unlikely(!slot))\n" + "+\t\treturn NULL;\n" + "+\n" + " \tif (flags & RADIX_TREE_ITER_TAGGED) {\n" + " \t\tvoid *canon = slot;\n" + " \n" + "-- \n" + "2.9.0\n" + "\n" + "--\n" + "To unsubscribe, send a message with 'unsubscribe linux-mm' in\n" + "the body to majordomo@kvack.org. For more info on Linux MM,\n" + "see: http://www.linux-mm.org/ .\n" + "Don't email: <a href=mailto:\"dont@kvack.org\"> email@kvack.org </a>" -f8bf3843c8532a57187e503115a4114f0f7645a290ce8b501cb57a872dfa998e +a751e71ccab3afaea4038927e7d141c9b2bc7732881b247bab495ae204d4108c
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.