* Re: [Outreachy kernel] [PATCH] staging:rtl8192u: Check memory allocation
2017-02-28 20:02 [PATCH] staging:rtl8192u: Check memory allocation Georgiana Rodica Chelu
@ 2017-02-28 20:25 ` Julia Lawall
2017-03-01 21:32 ` Georgiana Chelu
1 sibling, 0 replies; 3+ messages in thread
From: Julia Lawall @ 2017-02-28 20:25 UTC (permalink / raw)
To: Georgiana Rodica Chelu; +Cc: outreachy-kernel, gregkh
On Tue, 28 Feb 2017, Georgiana Rodica Chelu wrote:
> Check if the allocation is not successful and
> return the error code -ENOMEM.
>
> Signed-off-by: Georgiana Rodica Chelu <georgiana.chelu93@gmail.com>
> ---
> drivers/staging/rtl8192u/r8192U_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c b/drivers/staging/rtl8192u/r8192U_core.c
> index b631990..04025b6 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1689,9 +1689,13 @@ static short rtl8192_usb_initendpoints(struct net_device *dev)
> #ifndef JACKSON_NEW_RX
> for (i = 0; i < (MAX_RX_URB + 1); i++) {
> priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->rx_urb[i])
> + return -ENOMEM;
>
> priv->rx_urb[i]->transfer_buffer =
> kmalloc(RX_URB_SIZE, GFP_KERNEL);
> + if (!priv->rx_urb[i]->transfer_buffer)
> + return -ENOMEM;
At this point, the usb_alloc_urb has succeeded but the kmalloc has failed.
The context won't know which of these failed, or whether the failure came
from other places in the function, so one can assume that it is not going
to try to continue. Thus, this code needs to clean up after itself.
An example of such cleanup code is near the end of the function with the
code:
priv->pp_rxskb = kcalloc(MAX_RX_URB, sizeof(struct sk_buff *),
GFP_KERNEL);
if (!priv->pp_rxskb) {
kfree(priv->rx_urb);
...
In your case there are several issues to consider:
* how to free the result of the kmalloc before the ifdef
* how to free the result of usb_alloc_urb (you can find examples in other
kernel files)
* how to free the things allocated on previous iterations of the for loop
* what to do about the various dangling pointers in the priv structure
Note that the current error handling code shown abover is not complete,
because it doesn't take into account the things defined in the for loop.
A common pattern is to not put the error handling code directly under the
error detecting if, but rather to have a goto into a sequence of labels at
the end of the function that free all of the things allocated up to a give
point. The typical strategy is to free things in the opposite of the
order in which they are allocated. You can find many examples of thisin
the kernel.
Be careful, because a lot of changes are required.
julia
>
> priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
> }
> --
> 2.7.4
>
> --
> 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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170228200257.GA14816%40fireworks.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] staging:rtl8192u: Check memory allocation
2017-02-28 20:02 [PATCH] staging:rtl8192u: Check memory allocation Georgiana Rodica Chelu
2017-02-28 20:25 ` [Outreachy kernel] " Julia Lawall
@ 2017-03-01 21:32 ` Georgiana Chelu
1 sibling, 0 replies; 3+ messages in thread
From: Georgiana Chelu @ 2017-03-01 21:32 UTC (permalink / raw)
To: outreachy-kernel; +Cc: gregkh
[-- Attachment #1.1: Type: text/plain, Size: 1429 bytes --]
Hi Julia,
Thank you for your time spent to explain me what is the better approach. I
tried to
take into account your advice and to make use of the existing code. Please,
let me
know if am I on the right path.
On Tuesday, 28 February 2017 22:02:59 UTC+2, Georgiana Chelu wrote:
>
> Check if the allocation is not successful and
> return the error code -ENOMEM.
>
> Signed-off-by: Georgiana Rodica Chelu <georgiana.chelu93@gmail.com>
> ---
> drivers/staging/rtl8192u/r8192U_core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c
> b/drivers/staging/rtl8192u/r8192U_core.c
> index b631990..04025b6 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -1689,9 +1689,13 @@ static short rtl8192_usb_initendpoints(struct
> net_device *dev)
> #ifndef JACKSON_NEW_RX
> for (i = 0; i < (MAX_RX_URB + 1); i++) {
> priv->rx_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
> + if (!priv->rx_urb[i])
> + return -ENOMEM;
>
> priv->rx_urb[i]->transfer_buffer =
> kmalloc(RX_URB_SIZE, GFP_KERNEL);
> + if (!priv->rx_urb[i]->transfer_buffer)
> + return -ENOMEM;
>
> priv->rx_urb[i]->transfer_buffer_length = RX_URB_SIZE;
> }
> --
> 2.7.4
>
>
[-- Attachment #1.2: Type: text/html, Size: 2174 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread