Skip to contentSkip to conversation

libiscsi: fix iscsi pool error path

PATCH In review Chris Wright 3 participants 6 messages Mar 30, 2009 – Mar 31, 2009 1 message not in the archive SCSI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Chris Wright

author
#1 lore raw
Quoted 12 lines · * Jean Delvare ([email protected]) wrote:
drivers/scsi/libiscsi.c +1 -1
@@ -1944,7 +1944,7 @@ iscsi_pool_init(struct iscsi_pool *q, in
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		goto enomem;
+		return -ENOMEM;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

drivers/scsi/libiscsi.c void iscsi_pool_free(struct iscsi_pool * · line 1983

Quoted 7 lines
drivers/scsi/libiscsi.c +1 -2
@@ -1979,8 +1979,7 @@ void iscsi_pool_free(struct iscsi_pool *
 
 	for (i = 0; i < q->max; i++)
 		kfree(q->pool[i]);
-	if (q->pool)
-		kfree(q->pool);
+	kfree(q->pool);
 	kfree(q->queue);

AFAICT, This is still broken.

thanks,
-chris
--

Subject: [PATCH] libiscsi: fix error path on iscsi_pool_init

From: Chris Wright <[email protected]>

I'm not all that keen on kfifo_init returning ERR_PTR, but... q->queue could be ERR_PTR(-ENOMEM) which will break unwinding on error. Make iscsi_pool_free more defensive.

Signed-off-by Chris Wright <[email protected]>

---

drivers/scsi/libiscsi.c +8 -5
index dfaa8ad..2f4df53 100644
@@ -1995,7 +1995,7 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		return -ENOMEM;
+		goto enomem;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

@@ -2028,10 +2028,13 @@ void iscsi_pool_free(struct iscsi_pool *q)
 {
 	int i;
 
-	for (i = 0; i < q->max; i++)
-		kfree(q->pool[i]);
-	kfree(q->pool);
-	kfree(q->queue);
+	if (q->pool) {
+		for (i = 0; i < q->max; i++)
+			kfree(q->pool[i]);
+		kfree(q->pool);
+		if (!IS_ERR(q->queue))
+			kfree(q->queue);
+	}
 }
 EXPORT_SYMBOL_GPL(iscsi_pool_free);
author
#2 lore raw

PATCH 1/1 libsrp: free kfifo struct in srp_iu_pool_free

Appears to be a small leak here when freeing pool, the kfifo gets left behind. Did I miss a caller that frees kfifo?

Signed-off-by Chris Wright <[email protected]>

---

drivers/scsi/libsrp.c +1 -0
index 15e2d13..3a305f8 100644
@@ -81,6 +81,7 @@ static void srp_iu_pool_free(struct srp_queue *q)
 {
 	kfree(q->items);
 	kfree(q->pool);
+	kfree(q->queue);
 }
 
 static struct srp_buf **srp_ring_alloc(struct device *dev,
Reviewed-by on v1 1/1
#3 lore raw

Re: [PATCH] libsrp: free kfifo struct in srp_iu_pool_free

Hi Chris,

Le lundi 30 mars 2009, Chris Wright a écrit :

Quoted 11 lines

Appears to be a small leak here when freeing pool, the kfifo gets left behind. Did I miss a caller that frees kfifo?

Signed-off-by Chris Wright <[email protected]>

---

drivers/scsi/libsrp.c +0 -0
index 15e2d13..3a305f8 100644

drivers/scsi/libsrp.c static void srp_iu_pool_free(struct srp_queue *q) · line 87

Quoted 7 lines
drivers/scsi/libsrp.c +1 -0
@@ -81,6 +81,7 @@ static void srp_iu_pool_free(struct srp_queue *q)
 {
 	kfree(q->items);
 	kfree(q->pool);
+	kfree(q->queue);
 }
 
 static struct srp_buf **srp_ring_alloc(struct device *dev,

You appear to be correct, I can't see q->queue being freed anywhere either.

Reviewed-by Jean Delvare <[email protected]>
Signature · 6 lines
Jean Delvare
Suse L3
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
In reply to Chris Wright

Hi Chris,

Le lundi 30 mars 2009, Chris Wright a écrit :

Quoted 27 lines · * Jean Delvare ([email protected]) wrote:
Quoted 12 lines
drivers/scsi/libiscsi.c +1 -1
@@ -1944,7 +1944,7 @@ iscsi_pool_init(struct iscsi_pool *q, in
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		goto enomem;
+		return -ENOMEM;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

drivers/scsi/libiscsi.c void iscsi_pool_free(struct iscsi_pool * · line 1983

Quoted 7 lines
drivers/scsi/libiscsi.c +1 -2
@@ -1979,8 +1979,7 @@ void iscsi_pool_free(struct iscsi_pool *
 
 	for (i = 0; i < q->max; i++)
 		kfree(q->pool[i]);
-	if (q->pool)
-		kfree(q->pool);
+	kfree(q->pool);
 	kfree(q->queue);

AFAICT, This is still broken.

thanks,
-chris
--

Quoted 5 lines

Subject: [PATCH] libiscsi: fix error path on iscsi_pool_init

From: Chris Wright <[email protected]>

I'm not all that keen on kfifo_init returning ERR_PTR, but...

Ah, I had not noticed this. Good catch!

Quoted 25 lines

q->queue could be ERR_PTR(-ENOMEM) which will break unwinding on error. Make iscsi_pool_free more defensive.

Signed-off-by Chris Wright <[email protected]>

---

drivers/scsi/libiscsi.c +1 -4
index dfaa8ad..2f4df53 100644
@@ -1995,7 +1995,7 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		return -ENOMEM;
+		goto enomem;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

@@ -2028,10 +2028,13 @@ void iscsi_pool_free(struct iscsi_pool *q)
 {
 	int i;
 
-	for (i = 0; i < q->max; i++)
-		kfree(q->pool[i]);
-	kfree(q->pool);

drivers/scsi/libiscsi.c void iscsi_pool_free(struct iscsi_pool *q) · line 2039

Quoted 10 lines
drivers/scsi/libiscsi.c +7 -1
-	kfree(q->queue);
+	if (q->pool) {
+		for (i = 0; i < q->max; i++)
+			kfree(q->pool[i]);
+		kfree(q->pool);
+		if (!IS_ERR(q->queue))
+			kfree(q->queue);
+	}
 }
 EXPORT_SYMBOL_GPL(iscsi_pool_free);

Making the freeing of q->queue dependent on q->pool being set looks really weird (although it is correct at the moment. But this seems to be fixable in a much simpler way:

---
 drivers/scsi/libiscsi.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/libiscsi.c +3 -1
@@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, in
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);
-	if (q->queue == ERR_PTR(-ENOMEM))
+	if (q->queue == ERR_PTR(-ENOMEM)) {
+		q->queue = NULL;
 		goto enomem;
+	}
 
 	for (i = 0; i < max; i++) {
 		q->pool[i] = kzalloc(item_size, GFP_KERNEL);

With the benefit that only the error case is slowed down. In both cases we have a problem if q->queue contains an error value but it's not -ENOMEM. Apparently this can't happen today, but it doesn't feel right to assume this will always be true. Maybe it's the right time to fix this as well.

Signature · 6 lines
Jean Delvare
Suse L3
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author Acked-by
#5 lore raw
Quoted 3 lines · * Jean Delvare ([email protected]) wrote:

Making the freeing of q->queue dependent on q->pool being set looks really weird (although it is correct at the moment. But this seems to be fixable in a much simpler way:

Fine by me. I condsidered that and for some reason I thought it was odd to do it that way ;-) The pool is the ->buffer used in the fifo, but...

Acked-by Chris Wright <[email protected]>
#6 lore raw
In reply to Jean Delvare
Quoted 29 lines · Jean Delvare

Hi Chris,

Quoted 26 lines · Chris Wright
Quoted 12 lines · * Jean Delvare ([email protected]) wrote:
drivers/scsi/libiscsi.c +1 -1
@@ -1944,7 +1944,7 @@ iscsi_pool_init(struct iscsi_pool *q, in
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		goto enomem;
+		return -ENOMEM;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

drivers/scsi/libiscsi.c void iscsi_pool_free(struct iscsi_pool * · line 1983

Quoted 7 lines
drivers/scsi/libiscsi.c +1 -2
@@ -1979,8 +1979,7 @@ void iscsi_pool_free(struct iscsi_pool *
 
 	for (i = 0; i < q->max; i++)
 		kfree(q->pool[i]);
-	if (q->pool)
-		kfree(q->pool);
+	kfree(q->pool);
 	kfree(q->queue);

AFAICT, This is still broken.

thanks,
-chris
--

Quoted 8 lines
Quoted 5 lines

Subject: [PATCH] libiscsi: fix error path on iscsi_pool_init

From: Chris Wright <[email protected]>

I'm not all that keen on kfifo_init returning ERR_PTR, but...

Ah, I had not noticed this. Good catch!

Yeah, thanks Chris.

Quoted 59 lines
Quoted 25 lines

q->queue could be ERR_PTR(-ENOMEM) which will break unwinding on error. Make iscsi_pool_free more defensive.

Signed-off-by Chris Wright <[email protected]>

---

drivers/scsi/libiscsi.c +1 -4
index dfaa8ad..2f4df53 100644
@@ -1995,7 +1995,7 @@ iscsi_pool_init(struct iscsi_pool *q, int max, void ***items, int item_size)
 		num_arrays++;
 	q->pool = kzalloc(num_arrays * max * sizeof(void*), GFP_KERNEL);
 	if (q->pool == NULL)
-		return -ENOMEM;
+		goto enomem;
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);

@@ -2028,10 +2028,13 @@ void iscsi_pool_free(struct iscsi_pool *q)
 {
 	int i;
 
-	for (i = 0; i < q->max; i++)
-		kfree(q->pool[i]);
-	kfree(q->pool);

drivers/scsi/libiscsi.c void iscsi_pool_free(struct iscsi_pool *q) · line 2039

Quoted 10 lines
drivers/scsi/libiscsi.c +7 -1
-	kfree(q->queue);
+	if (q->pool) {
+		for (i = 0; i < q->max; i++)
+			kfree(q->pool[i]);
+		kfree(q->pool);
+		if (!IS_ERR(q->queue))
+			kfree(q->queue);
+	}
 }
 EXPORT_SYMBOL_GPL(iscsi_pool_free);

Making the freeing of q->queue dependent on q->pool being set looks really weird (although it is correct at the moment. But this seems to be fixable in a much simpler way:

---
 drivers/scsi/libiscsi.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/libiscsi.c +3 -1
@@ -1999,8 +1999,10 @@ iscsi_pool_init(struct iscsi_pool *q, in
 
 	q->queue = kfifo_init((void*)q->pool, max * sizeof(void*),
 			      GFP_KERNEL, NULL);
-	if (q->queue == ERR_PTR(-ENOMEM))
+	if (q->queue == ERR_PTR(-ENOMEM)) {
+		q->queue = NULL;
 		goto enomem;
+	}
 
 	for (i = 0; i < max; i++) {
 		q->pool[i] = kzalloc(item_size, GFP_KERNEL);
Quoted 6 lines

With the benefit that only the error case is slowed down. In both cases we have a problem if q->queue contains an error value but it's not -ENOMEM. Apparently this can't happen today, but it doesn't feel right to assume this will always be true. Maybe it's the right time to fix this as well.

Yeah I agree. I will just modify the patch to check for IS_ERR(q->queue) in there.

I will do a quick sanity test on it here then pass it on to James with some other stuff for 2.6.30.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html