staging: gasket Create a memory allocation path

PATCH Superseded Ioannis Valasakis 3 participants 8 messages Oct 31, 2018 Staging

lore

Share & tools

lore

Ioannis Valasakis

author

2018-10-31 16:48 UTC #0 lore raw

PATCH v2 1/1 staging: gasket Create a memory allocation path

Fix multiple error allocation path by using re-direction.

Signed-off-by Ioannis Valasakis <[email protected]>

---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre

drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------

 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/staging/gasket/gasket_interrupt.c +8 -6

index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {

@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }

Signature · 1 line

2.19.1

Sasha Levin

2018-10-31 16:54 UTC #1 lore raw

Quoted 45 lines · Ioannis Valasakis

Fix multiple error allocation path by using re-direction.

Signed-off-by Ioannis Valasakis <[email protected]>

---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre

drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------

1 file changed, 8 insertions(+), 6 deletions(-)

drivers/staging/gasket/gasket_interrupt.c +3 -4

index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {

@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
-err_alloc:

drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name, · line 407

Quoted 10 lines

drivers/staging/gasket/gasket_interrupt.c +5 -2

-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }

With this code, what will happen if, for example, the last allocation (interrupt_data->interrupt_counts) fails?

--
Thanks,
Sasha

Ioannis Valasakis

author

2018-10-31 20:51 UTC #2 lore raw

Quoted 45 lines · Sasha Levin

Quoted 44 lines · Ioannis Valasakis

Fix multiple error allocation path by using re-direction.

Signed-off-by Ioannis Valasakis <[email protected]>

---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre

drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------

1 file changed, 8 insertions(+), 6 deletions(-)

drivers/staging/gasket/gasket_interrupt.c +3 -3

index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {

@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;

Quoted 14 lines

drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name, · line 407
Quoted 11 lines
drivers/staging/gasket/gasket_interrupt.c +5 -3
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
With this code, what will happen if, for example, the last allocation (interrupt_data->interrupt_counts) fails?

It took my some time but I think I saw it; the mistake is that if interrupt_data->interrupt_counts fails it should 'goto err_free_msix'.

Same if interrupt_data->msix_entries fails it should 'goto err_free_data'.

Is that right?

(I got very ambitious with the labels and I mixed it up ;) Any way to make the goto labels make sense?)

Quoted 4 lines

--
Thanks,
Sasha

Best,
Ioannis

Julia Lawall

2018-10-31 20:57 UTC #3 lore raw

Quoted 64 lines · Ioannis Valasakis

Quoted 45 lines · Sasha Levin
Quoted 44 lines · Ioannis Valasakis
Fix multiple error allocation path by using re-direction.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre
drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +3 -3
index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
Quoted 14 lines
drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name, · line 407
Quoted 11 lines
drivers/staging/gasket/gasket_interrupt.c +5 -3
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
With this code, what will happen if, for example, the last allocation (interrupt_data->interrupt_counts) fails?
It took my some time but I think I saw it; the mistake is that if interrupt_data->interrupt_counts fails it should 'goto err_free_msix'.

Quoted 7 lines

Same if interrupt_data->msix_entries fails it should 'goto err_free_data'.
Is that right?
(I got very ambitious with the labels and I mixed it up ;) Any way to make the goto labels make sense?)

I don't think that the choice of labels is a problem. Basically, you should normally release things in the opposite order of the one in which they were allocated. At the beginning of your function, you can just do a return, because nothing has been allocated yet. Then when you have successfully allocated something, if you fail afterwards you should go to the very end of the function, because you have only one thing to free. When you have successfully allocated something else, if you fail afterwards, you should jump not quite to the end, so that you free both things that have been allocated, etc.

julia

Ioannis Valasakis

author

2018-10-31 21:00 UTC #4 lore raw

Quoted 76 lines · Julia Lawall

Quoted 64 lines · Ioannis Valasakis
Quoted 45 lines · Sasha Levin
Quoted 44 lines · Ioannis Valasakis
Fix multiple error allocation path by using re-direction.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre
drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +3 -3
index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
Quoted 14 lines
drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name, · line 407
Quoted 11 lines
drivers/staging/gasket/gasket_interrupt.c +5 -3
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
With this code, what will happen if, for example, the last allocation (interrupt_data->interrupt_counts) fails?
It took my some time but I think I saw it; the mistake is that if interrupt_data->interrupt_counts fails it should 'goto err_free_msix'.
Quoted 7 lines
Same if interrupt_data->msix_entries fails it should 'goto err_free_data'.
Is that right?
(I got very ambitious with the labels and I mixed it up ;) Any way to make the goto labels make sense?)

Quoted 9 lines

I don't think that the choice of labels is a problem. Basically, you should normally release things in the opposite order of the one in which they were allocated. At the beginning of your function, you can just do a return, because nothing has been allocated yet. Then when you have successfully allocated something, if you fail afterwards you should go to the very end of the function, because you have only one thing to free. When you have successfully allocated something else, if you fail afterwards, you should jump not quite to the end, so that you free both things that have been allocated, etc.

Indeed that is what I meant. Badly phrased but I released things the wrong way around. I send a v3 of the patch (although as I just noticed your latest comment, if that makes sense I am going to rebase against greg's staging and resend it).

Thank you

Ioannis

Quoted 2 lines

julia

Julia Lawall

2018-10-31 21:04 UTC #5 lore raw

Quoted 86 lines · Ioannis Valasakis

Quoted 76 lines · Julia Lawall
Quoted 64 lines · Ioannis Valasakis
Quoted 45 lines · Sasha Levin
Quoted 44 lines · Ioannis Valasakis
Fix multiple error allocation path by using re-direction.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre
drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +3 -3
index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
Quoted 14 lines
drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name, · line 407
Quoted 11 lines
drivers/staging/gasket/gasket_interrupt.c +5 -3
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
With this code, what will happen if, for example, the last allocation (interrupt_data->interrupt_counts) fails?
It took my some time but I think I saw it; the mistake is that if interrupt_data->interrupt_counts fails it should 'goto err_free_msix'.
Quoted 7 lines
Same if interrupt_data->msix_entries fails it should 'goto err_free_data'.
Is that right?
(I got very ambitious with the labels and I mixed it up ;) Any way to make the goto labels make sense?)
Quoted 9 lines
I don't think that the choice of labels is a problem. Basically, you should normally release things in the opposite order of the one in which they were allocated. At the beginning of your function, you can just do a return, because nothing has been allocated yet. Then when you have successfully allocated something, if you fail afterwards you should go to the very end of the function, because you have only one thing to free. When you have successfully allocated something else, if you fail afterwards, you should jump not quite to the end, so that you free both things that have been allocated, etc.

Quoted 4 lines

Indeed that is what I meant. Badly phrased but I released things the wrong way around. I send a v3 of the patch (although as I just noticed your latest comment, if that makes sense I am going to rebase against greg's staging and resend it).

Please send the version against Greg's tree. It would be easiest to see if the patch is correct if one can see the actual starting point.

julia

Julia Lawall

2018-10-31 20:40 UTC #6 lore raw

In reply to Ioannis Valasakis

Quoted 1 line · Ioannis Valasakis

Fix multiple error allocation path by using re-direction.

Beside the semantic problem that has already been pointed out, I find the word "re-direction" a bit strange, or a bit vague. What is redirected? Could it be something like "Collect the common error handling code at the end of the function"?

I also have the impression that this patch is on top of your previous incorrect patch, instead of the original code. You should always work against the code in Greg's tree.

julia

Quoted 66 lines

Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre
drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +8 -7
index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
 
--
2.19.1
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181031164753.GA14140%40kvasir.local.
For more options, visit https://groups.google.com/d/optout.

Ioannis Valasakis

author

2018-10-31 20:58 UTC #7 lore raw

Quoted 6 lines · Julia Lawall

Quoted 1 line · Ioannis Valasakis
Fix multiple error allocation path by using re-direction.

Quoted 8 lines

Beside the semantic problem that has already been pointed out, I find the word "re-direction" a bit strange, or a bit vague. What is redirected? Could it be something like "Collect the common error handling code at the end of the function"?
I also have the impression that this patch is on top of your previous incorrect patch, instead of the original code. You should always work against the code in Greg's tree.

Thanks Julia. I am going to rebase on greg's staging and resend the patch (ah fourth one!). Sorry that I am bothering all of you but in every step I am learning something new. Thanks for being patient and helpful!

Quoted 69 lines

julia
Quoted 66 lines
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v2:
- Specific tags for memory allocation
- Free the memory in the right way as for kfre
drivers/staging/gasket/gasket_interrupt.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +8 -7
index cdfb72af1c52..23232ea8f4d9 100644
@@ -348,21 +348,21 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 					       sizeof(struct msix_entry),
 					       GFP_KERNEL);
 	if (!interrupt_data->msix_entries) {
-		goto err_alloc;
+		goto err_free_msix;
 	}
 
 	interrupt_data->eventfd_ctxs = kcalloc(num_interrupts,
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		goto err_alloc;
+		goto err_free_eventfd;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		goto err_alloc;
+		goto err_free_data;
 	}
 
 	switch (interrupt_data->type) {
@@ -397,10 +397,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev, const char *name,
 
 	return 0;
 
-err_alloc:
-	kfree(interrupt_data);
-	kfree(interrupt_data->msix_entries);
+err_free_msix:
 	kfree(interrupt_data->eventfd_ctxs);
+err_free_eventfd:
+	kfree(interrupt_data->msix_entries);
+err_free_data:
+	kfree(interrupt_data);
 	return -ENOMEM;
 }
 
--
2.19.1
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20181031164753.GA14140%40kvasir.local.
For more options, visit https://groups.google.com/d/optout.