[PATCH] radix-tree: must check __radix_tree_preload() return value

From: Eric Dumazet <edumazet@google.com>

__radix_tree_preload() only disables preemption if no error is returned.

So we really need to make sure callers always check the return value.

idr_preload() contract is to always disable preemption, so we need
to add a missing preempt_disable() if an error happened.

Similarly, ida_pre_get() only needs to call preempt_enable() in the
case no error happened.

Fixes: 0a835c4f090a ("Reimplement IDR and IDA using the radix tree")
Fixes: 7ad3d4d85c7a ("ida: Move ida_bitmap to a percpu variable")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: <redacted>    [4.11+]
---
 lib/radix-tree.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 3527eb364964..fac702039304 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c

@@ -463,7 +463,7 @@ radix_tree_node_free(struct radix_tree_node *node)
  * To make use of this facility, the radix tree must be initialised without
  * __GFP_DIRECT_RECLAIM being passed to INIT_RADIX_TREE().
  */
-static int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
+static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
 {
 	struct radix_tree_preload *rtp;
 	struct radix_tree_node *node;

@@ -2104,7 +2104,10 @@ EXPORT_SYMBOL(radix_tree_tagged);
  */
 void idr_preload(gfp_t gfp_mask)
 {
-	__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+	int ret = __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+
+	if (ret)
+		preempt_disable();
 }
 EXPORT_SYMBOL(idr_preload);
 

@@ -2118,13 +2121,14 @@ EXPORT_SYMBOL(idr_preload);
  */
 int ida_pre_get(struct ida *ida, gfp_t gfp)
 {
-	__radix_tree_preload(gfp, IDA_PRELOAD_SIZE);
+	int ret = __radix_tree_preload(gfp, IDA_PRELOAD_SIZE);
 	/*
 	 * The IDA API has no preload_end() equivalent.  Instead,
 	 * ida_get_new() can return -EAGAIN, prompting the caller
 	 * to return to the ida_pre_get() step.
 	 */
-	preempt_enable();
+	if (!ret)
+		preempt_enable();
 
 	if (!this_cpu_read(ida_bitmap)) {
 		struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);

On Tue, Sep 5, 2017 at 10:59 AM, Eric Dumazet [off-list ref] wrote:

Is there a reason for the "ret" variable that is entirely mis-named,
since it's never actually used as a return value?

(Sure. it's the return value of a function, but that is entirely
useless and pointless information, and adds no value. We should name
variables by the data they contain or how they are used, not by "it
was the return value of a function").

In other words, why isn't this just

        if (__radix_tree_preload(..))
                preempt_disable();

which is shorter and clearer and not confusing?

Same issue, but this time strengthened by an additional "why doesn't
this just use that idr_preload function then?" question..

               Linus

On Tue, 2017-09-05 at 11:07 -0700, Linus Torvalds wrote:

On Tue, Sep 5, 2017 at 10:59 AM, Eric Dumazet [off-list ref] wrote:

quoted

@@ -2104,7 +2104,10 @@ EXPORT_SYMBOL(radix_tree_tagged);
  */
 void idr_preload(gfp_t gfp_mask)
 {
-       __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+       int ret = __radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+
+       if (ret)
+               preempt_disable();
 }
 EXPORT_SYMBOL(idr_preload);

Is there a reason for the "ret" variable that is entirely mis-named,
since it's never actually used as a return value?

(Sure. it's the return value of a function, but that is entirely
useless and pointless information, and adds no value. We should name
variables by the data they contain or how they are used, not by "it
was the return value of a function").

In other words, why isn't this just

        if (__radix_tree_preload(..))
                preempt_disable();

which is shorter and clearer and not confusing?

...

Same issue, but this time strengthened by an additional "why doesn't
this just use that idr_preload function then?" question..

Yep, I will send a v2, thanks.

From: Eric Dumazet <edumazet@google.com>

__radix_tree_preload() only disables preemption if no error is returned.

So we really need to make sure callers always check the return value.

idr_preload() contract is to always disable preemption, so we need
to add a missing preempt_disable() if an error happened.

Similarly, ida_pre_get() only needs to call preempt_enable() in the
case no error happened.

Fixes: 0a835c4f090a ("Reimplement IDR and IDA using the radix tree")
Fixes: 7ad3d4d85c7a ("ida: Move ida_bitmap to a percpu variable")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: <redacted>    [4.11+]
---
v2: addressed Linus feedback, not adding useless/confusing 'ret' variables.

 lib/radix-tree.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 3527eb364964..afb3cb4d44b6 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c

@@ -463,7 +463,7 @@ radix_tree_node_free(struct radix_tree_node *node)
  * To make use of this facility, the radix tree must be initialised without
  * __GFP_DIRECT_RECLAIM being passed to INIT_RADIX_TREE().
  */
-static int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
+static __must_check int __radix_tree_preload(gfp_t gfp_mask, unsigned nr)
 {
 	struct radix_tree_preload *rtp;
 	struct radix_tree_node *node;

@@ -2104,7 +2104,8 @@ EXPORT_SYMBOL(radix_tree_tagged);
  */
 void idr_preload(gfp_t gfp_mask)
 {
-	__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE);
+	if (__radix_tree_preload(gfp_mask, IDR_PRELOAD_SIZE))
+		preempt_disable();
 }
 EXPORT_SYMBOL(idr_preload);
 

@@ -2118,13 +2119,13 @@ EXPORT_SYMBOL(idr_preload);
  */
 int ida_pre_get(struct ida *ida, gfp_t gfp)
 {
-	__radix_tree_preload(gfp, IDA_PRELOAD_SIZE);
 	/*
 	 * The IDA API has no preload_end() equivalent.  Instead,
 	 * ida_get_new() can return -EAGAIN, prompting the caller
 	 * to return to the ida_pre_get() step.
 	 */
-	preempt_enable();
+	if (!__radix_tree_preload(gfp, IDA_PRELOAD_SIZE))
+		preempt_enable();
 
 	if (!this_cpu_read(ida_bitmap)) {
 		struct ida_bitmap *bitmap = kmalloc(sizeof(*bitmap), gfp);