Message ID | 20190923155300.20407-1-navid.emamdoost@gmail.com (mailing list archive) |
---|---|
State | Rejected |
Delegated to: | Jason Gunthorpe |
Headers | show |
Series | [v2] RDMA: release allocated skb | expand |
On Mon, Sep 23, 2019 at 10:52:59AM -0500, Navid Emamdoost wrote: > In create_cq, the allocated skb buffer needs to be released on error > path. > Moved the kfree_skb(skb) under err4 label. This didn't move anything > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > drivers/infiniband/hw/cxgb4/cq.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > index b1bb61c65f4f..1886c1af10bc 100644 > +++ b/drivers/infiniband/hw/cxgb4/cq.c > @@ -173,6 +173,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, > err4: > dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue, > dma_unmap_addr(cq, mapping)); > + kfree_skb(skb); > err3: > kfree(cq->sw_queue); > err2: This looks wrong to me: int c4iw_ofld_send(struct c4iw_rdev *rdev, struct sk_buff *skb) { int error = 0; if (c4iw_fatal_error(rdev)) { kfree_skb(skb); pr_err("%s - device in error state - dropping\n", __func__); return -EIO; } error = cxgb4_ofld_send(rdev->lldi.ports[0], skb); if (error < 0) kfree_skb(skb); return error < 0 ? error : 0; } Jason
Hi Jason, Thanks for the feedback. Yes, you are right if the skb release is moved under err4 label it will cause a double free as c4iw_ref_send_wait will release skb in case of error. So, in order to avoid leaking skb in case of c4iw_bar2_addrs failure, the kfree(skb) could be placed under the error check like the way patch v1 did. Do you see any mistake in version 1? https://lore.kernel.org/patchwork/patch/1128510/ Thanks, Navid On Tue, Oct 1, 2019 at 8:54 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Sep 23, 2019 at 10:52:59AM -0500, Navid Emamdoost wrote: > > In create_cq, the allocated skb buffer needs to be released on error > > path. > > Moved the kfree_skb(skb) under err4 label. > > This didn't move anything > > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > > drivers/infiniband/hw/cxgb4/cq.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > > index b1bb61c65f4f..1886c1af10bc 100644 > > +++ b/drivers/infiniband/hw/cxgb4/cq.c > > @@ -173,6 +173,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, > > err4: > > dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue, > > dma_unmap_addr(cq, mapping)); > > + kfree_skb(skb); > > err3: > > kfree(cq->sw_queue); > > err2: > > This looks wrong to me: > > int c4iw_ofld_send(struct c4iw_rdev *rdev, struct sk_buff *skb) > { > int error = 0; > > if (c4iw_fatal_error(rdev)) { > kfree_skb(skb); > pr_err("%s - device in error state - dropping\n", __func__); > return -EIO; > } > error = cxgb4_ofld_send(rdev->lldi.ports[0], skb); > if (error < 0) > kfree_skb(skb); > return error < 0 ? error : 0; > } > > Jason
On Wed, Oct 02, 2019 at 04:35:06PM -0500, Navid Emamdoost wrote: > Hi Jason, > > Thanks for the feedback. Yes, you are right if the skb release is > moved under err4 label it will cause a double free as > c4iw_ref_send_wait will release skb in case of error. > So, in order to avoid leaking skb in case of c4iw_bar2_addrs failure, > the kfree(skb) could be placed under the error check like the way > patch v1 did. Do you see any mistake in version 1? > https://lore.kernel.org/patchwork/patch/1128510/ No, it is not enough. c4iw_ref_send_wait() -> c4iw_wait_for_reply() -> return wr_waitp->ret; <--- can be -EIO Thanks
On Thursday, October 10/03/19, 2019 at 03:05:06 +0530, Navid Emamdoost wrote: > Hi Jason, > > Thanks for the feedback. Yes, you are right if the skb release is > moved under err4 label it will cause a double free as > c4iw_ref_send_wait will release skb in case of error. > So, in order to avoid leaking skb in case of c4iw_bar2_addrs failure, > the kfree(skb) could be placed under the error check like the way > patch v1 did. Do you see any mistake in version 1? > https://lore.kernel.org/patchwork/patch/1128510/ Hi Navid, Both the revisions of the patch are invalid. skb is freed in both the cases of failure and success through c4iw_ofld_send(). case success: in ctrl_xmit() case failure: in c4iw_ofld_send() Thanks, Bharat. > > > Thanks, > Navid > > On Tue, Oct 1, 2019 at 8:54 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Mon, Sep 23, 2019 at 10:52:59AM -0500, Navid Emamdoost wrote: > > > In create_cq, the allocated skb buffer needs to be released on error > > > path. > > > Moved the kfree_skb(skb) under err4 label. > > > > This didn't move anything > > > > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > > > drivers/infiniband/hw/cxgb4/cq.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > > > index b1bb61c65f4f..1886c1af10bc 100644 > > > +++ b/drivers/infiniband/hw/cxgb4/cq.c > > > @@ -173,6 +173,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, > > > err4: > > > dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue, > > > dma_unmap_addr(cq, mapping)); > > > + kfree_skb(skb); > > > err3: > > > kfree(cq->sw_queue); > > > err2: > > > > This looks wrong to me: > > > > int c4iw_ofld_send(struct c4iw_rdev *rdev, struct sk_buff *skb) > > { > > int error = 0; > > > > if (c4iw_fatal_error(rdev)) { > > kfree_skb(skb); > > pr_err("%s - device in error state - dropping\n", __func__); > > return -EIO; > > } > > error = cxgb4_ofld_send(rdev->lldi.ports[0], skb); > > if (error < 0) > > kfree_skb(skb); > > return error < 0 ? error : 0; > > } > > > > Jason > > > > -- > Navid.
Hi Leon and Potnuri, Based on the following call sequence, skb is passed along to uld_send(). c4iw_ref_send_wait c4iw_ofld_send cxgb4_ofld_send t4_ofld_send uld_send In uld_send() skb is consumed (released or added to queue) via ctrl_xmit() or ofld_xmit(), which assures no leak is happening. But in the condition check for txq_info the return value is NET_XMIT_DROP which means the skb should be released. Here I believe skb is being leaked: txq_info = adap->sge.uld_txq_info[tx_uld_type]; if (unlikely(!txq_info)) { WARN_ON(true); return NET_XMIT_DROP; } Please let me know what you think, then I can go ahead and fix the patch. Thank you, Navid. On Thu, Oct 3, 2019 at 5:25 AM Potnuri Bharat Teja <bharat@chelsio.com> wrote: > > On Thursday, October 10/03/19, 2019 at 03:05:06 +0530, Navid Emamdoost wrote: > > Hi Jason, > > > > Thanks for the feedback. Yes, you are right if the skb release is > > moved under err4 label it will cause a double free as > > c4iw_ref_send_wait will release skb in case of error. > > So, in order to avoid leaking skb in case of c4iw_bar2_addrs failure, > > the kfree(skb) could be placed under the error check like the way > > patch v1 did. Do you see any mistake in version 1? > > https://lore.kernel.org/patchwork/patch/1128510/ > > Hi Navid, > Both the revisions of the patch are invalid. skb is freed in both the cases of > failure and success through c4iw_ofld_send(). > case success: in ctrl_xmit() > case failure: in c4iw_ofld_send() > > Thanks, > Bharat. > > > > > > > > Thanks, > > Navid > > > > On Tue, Oct 1, 2019 at 8:54 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Mon, Sep 23, 2019 at 10:52:59AM -0500, Navid Emamdoost wrote: > > > > In create_cq, the allocated skb buffer needs to be released on error > > > > path. > > > > Moved the kfree_skb(skb) under err4 label. > > > > > > This didn't move anything > > > > > > > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> > > > > drivers/infiniband/hw/cxgb4/cq.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c > > > > index b1bb61c65f4f..1886c1af10bc 100644 > > > > +++ b/drivers/infiniband/hw/cxgb4/cq.c > > > > @@ -173,6 +173,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, > > > > err4: > > > > dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue, > > > > dma_unmap_addr(cq, mapping)); > > > > + kfree_skb(skb); > > > > err3: > > > > kfree(cq->sw_queue); > > > > err2: > > > > > > This looks wrong to me: > > > > > > int c4iw_ofld_send(struct c4iw_rdev *rdev, struct sk_buff *skb) > > > { > > > int error = 0; > > > > > > if (c4iw_fatal_error(rdev)) { > > > kfree_skb(skb); > > > pr_err("%s - device in error state - dropping\n", __func__); > > > return -EIO; > > > } > > > error = cxgb4_ofld_send(rdev->lldi.ports[0], skb); > > > if (error < 0) > > > kfree_skb(skb); > > > return error < 0 ? error : 0; > > > } > > > > > > Jason > > > > > > > > -- > > Navid.
diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c index b1bb61c65f4f..1886c1af10bc 100644 --- a/drivers/infiniband/hw/cxgb4/cq.c +++ b/drivers/infiniband/hw/cxgb4/cq.c @@ -173,6 +173,7 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, err4: dma_free_coherent(&rdev->lldi.pdev->dev, cq->memsize, cq->queue, dma_unmap_addr(cq, mapping)); + kfree_skb(skb); err3: kfree(cq->sw_queue); err2:
In create_cq, the allocated skb buffer needs to be released on error path. Moved the kfree_skb(skb) under err4 label. Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com> --- drivers/infiniband/hw/cxgb4/cq.c | 1 + 1 file changed, 1 insertion(+)