[net-next] qed: Set error code for allocation failures
diff mbox series

Message ID 20171027064020.aq52ue4kfp5htrb5@mwanda
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series
  • [net-next] qed: Set error code for allocation failures
Related show

Commit Message

Dan Carpenter Oct. 27, 2017, 6:40 a.m. UTC
There are several places where we accidentally return success when
kcalloc() fails.

Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and processing mpa fpdus")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Yunsheng Lin Oct. 27, 2017, 9:32 a.m. UTC | #1
Hi, Dan

On 2017/10/27 14:40, Dan Carpenter wrote:
> There are several places where we accidentally return success when
> kcalloc() fails.
> 
> Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and processing mpa fpdus")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index 409041eab189..6366f2ef82b7 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>  	struct qed_ll2_cbs cbs;
>  	u32 mpa_buff_size;
>  	u16 n_ooo_bufs;
> -	int rc = 0;
> +	int rc;
>  	int i;
>  
>  	iwarp_info = &p_hwfn->p_rdma_info->iwarp;
> @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>  	if (rc)
>  		goto err;
>  
> +	rc = -ENOMEM;
>  	iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>  					    sizeof(*iwarp_info->partial_fpdus),
>  					    GFP_KERNEL);

Does the memory allocated here need to be freed when error happens below?


> @@ -2724,7 +2725,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>  	for (i = 0; i < data.input.rx_num_desc; i++)
>  		list_add_tail(&iwarp_info->mpa_bufs[i].list_entry,
>  			      &iwarp_info->mpa_buf_list);
> -	return rc;
> +	return 0;
>  err:
>  	qed_iwarp_ll2_stop(p_hwfn, p_ptt);
>  
> 
> .
>
Dan Carpenter Oct. 27, 2017, 11:52 a.m. UTC | #2
On Fri, Oct 27, 2017 at 05:32:42PM +0800, Yunsheng Lin wrote:
> Hi, Dan
> 
> On 2017/10/27 14:40, Dan Carpenter wrote:
> > There are several places where we accidentally return success when
> > kcalloc() fails.
> > 
> > Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and processing mpa fpdus")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> > index 409041eab189..6366f2ef82b7 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> > @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> >  	struct qed_ll2_cbs cbs;
> >  	u32 mpa_buff_size;
> >  	u16 n_ooo_bufs;
> > -	int rc = 0;
> > +	int rc;
> >  	int i;
> >  
> >  	iwarp_info = &p_hwfn->p_rdma_info->iwarp;
> > @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
> >  	if (rc)
> >  		goto err;
> >  
> > +	rc = -ENOMEM;
> >  	iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
> >  					    sizeof(*iwarp_info->partial_fpdus),
> >  					    GFP_KERNEL);
> 
> Does the memory allocated here need to be freed when error happens below?
> 

Hm...  I think you're right that it leaks.  Also I'm confused by the
qed_iwarp_ll2_alloc_buffers() allocation.  The comment in there says
that /* buffers will be deallocated by qed_ll2 */ but qed_ll2 is not
a function name or something which is useful to grep.

regards,
dan carpenter
Yunsheng Lin Oct. 28, 2017, 5:50 a.m. UTC | #3
Hi, Dan

On 2017/10/27 19:52, Dan Carpenter wrote:
> On Fri, Oct 27, 2017 at 05:32:42PM +0800, Yunsheng Lin wrote:
>> Hi, Dan
>>
>> On 2017/10/27 14:40, Dan Carpenter wrote:
>>> There are several places where we accidentally return success when
>>> kcalloc() fails.
>>>
>>> Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and processing mpa fpdus")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>>> index 409041eab189..6366f2ef82b7 100644
>>> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>>> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
>>> @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>>>  	struct qed_ll2_cbs cbs;
>>>  	u32 mpa_buff_size;
>>>  	u16 n_ooo_bufs;
>>> -	int rc = 0;
>>> +	int rc;
>>>  	int i;
>>>  
>>>  	iwarp_info = &p_hwfn->p_rdma_info->iwarp;
>>> @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>>>  	if (rc)
>>>  		goto err;
>>>  
>>> +	rc = -ENOMEM;
>>>  	iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>>>  					    sizeof(*iwarp_info->partial_fpdus),
>>>  					    GFP_KERNEL);
>>
>> Does the memory allocated here need to be freed when error happens below?
>>
> 
> Hm...  I think you're right that it leaks.  Also I'm confused by the
> qed_iwarp_ll2_alloc_buffers() allocation.  The comment in there says
> that /* buffers will be deallocated by qed_ll2 */ but qed_ll2 is not
> a function name or something which is useful to grep.

Yes, I am confused by it too.

Even in qed_iwarp_ll2_alloc_buffers, if kzcalloc failed, it do not clean
up the memory allocated by pre kzcalloc.

> 
> regards,
> dan carpenter
> 
> 
> .
>
Michal Kalderon Oct. 29, 2017, 7:45 a.m. UTC | #4
From: Dan Carpenter <dan.carpenter@oracle.com>
Sent: Friday, October 27, 2017 2:52 PM

>On Fri, Oct 27, 2017 at 05:32:42PM +0800, Yunsheng Lin wrote:
>> >     iwarp_info = &p_hwfn->p_rdma_info->iwarp;
>> > @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>> >     if (rc)
>> >             goto err;
>> >
>> > +   rc = -ENOMEM;
>> >     iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>> >                                         sizeof(*iwarp_info->partial_fpdus),
>> >                                         GFP_KERNEL);
>>
>> Does the memory allocated here need to be freed when error happens below?
>>
>
>Hm...  I think you're right that it leaks.  Also I'm confused by the
>qed_iwarp_ll2_alloc_buffers() allocation.  The comment in there says
>that /* buffers will be deallocated by qed_ll2 */ but qed_ll2 is not
>a function name or something which is useful to grep.

Thanks Dan, partial_fpdus is released during qed_iwarp_resc_free, called from qed_rdma_resc_free
called on qed_rdma_stop and on failure during qed_rdma_start.
Regarding ll2 buffers. For each successfully allocated buffer we call
qed_iwarp_ll2_post_rx->qed_ll2_post_rx_buffer.
These buffers will get released and freed when we call qed_ll2_terminate_connection
called from qed_iwarp_ll2_stop ( which is called during qed_iwarp_ll2_start on error ).
thanks,
Michal
Michal Kalderon Oct. 29, 2017, 7:50 a.m. UTC | #5
From: Dan Carpenter <dan.carpenter@oracle.com>
Sent: Friday, October 27, 2017 9:40 AM

> There are several places where we accidentally return success when
> kcalloc() fails.
> 
> Fixes: fcb39f6c10b2 ("qed: Add mpa buffer descriptors for storing and processing mpa fpdus")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> index 409041eab189..6366f2ef82b7 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
> @@ -2585,7 +2585,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>         struct qed_ll2_cbs cbs;
>         u32 mpa_buff_size;
>         u16 n_ooo_bufs;
> -       int rc = 0;
> +       int rc;
>         int i;
> 
>         iwarp_info = &p_hwfn->p_rdma_info->iwarp;
> @@ -2696,6 +2696,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>         if (rc)
>                goto err;
> 
> +       rc = -ENOMEM;
>         iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
>                                             sizeof(*iwarp_info->partial_fpdus),
>                                             GFP_KERNEL);
> @@ -2724,7 +2725,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
>         for (i = 0; i < data.input.rx_num_desc; i++)
>                 list_add_tail(&iwarp_info->mpa_bufs[i].list_entry,
>                               &iwarp_info->mpa_buf_list);
> -       return rc;
> +       return 0;
>  err:
>         qed_iwarp_ll2_stop(p_hwfn, p_ptt);

Thanks Dan,
Acked-by: Michal Kalderon <michal.kalderon@cavium.com>

Patch
diff mbox series

diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 409041eab189..6366f2ef82b7 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -2585,7 +2585,7 @@  qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 	struct qed_ll2_cbs cbs;
 	u32 mpa_buff_size;
 	u16 n_ooo_bufs;
-	int rc = 0;
+	int rc;
 	int i;
 
 	iwarp_info = &p_hwfn->p_rdma_info->iwarp;
@@ -2696,6 +2696,7 @@  qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 	if (rc)
 		goto err;
 
+	rc = -ENOMEM;
 	iwarp_info->partial_fpdus = kcalloc((u16)p_hwfn->p_rdma_info->num_qps,
 					    sizeof(*iwarp_info->partial_fpdus),
 					    GFP_KERNEL);
@@ -2724,7 +2725,7 @@  qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 	for (i = 0; i < data.input.rx_num_desc; i++)
 		list_add_tail(&iwarp_info->mpa_bufs[i].list_entry,
 			      &iwarp_info->mpa_buf_list);
-	return rc;
+	return 0;
 err:
 	qed_iwarp_ll2_stop(p_hwfn, p_ptt);