| 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 | expand |
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); > > > . >
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
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 > > > . >
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
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>
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);
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>