* [PATCH v2] RDMA: release allocated skb [not found] <20190923050823.GL14368@unreal> @ 2019-09-23 15:52 ` Navid Emamdoost 2019-10-01 13:54 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Navid Emamdoost @ 2019-09-23 15:52 UTC (permalink / raw) To: leon Cc: emamd001, smccaman, kjlu, Navid Emamdoost, Potnuri Bharat Teja, Doug Ledford, Jason Gunthorpe, linux-rdma, linux-kernel 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(+) 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: -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA: release allocated skb 2019-09-23 15:52 ` [PATCH v2] RDMA: release allocated skb Navid Emamdoost @ 2019-10-01 13:54 ` Jason Gunthorpe 2019-10-02 21:35 ` Navid Emamdoost 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2019-10-01 13:54 UTC (permalink / raw) To: Navid Emamdoost Cc: leon, emamd001, smccaman, kjlu, Potnuri Bharat Teja, Doug Ledford, linux-rdma, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA: release allocated skb 2019-10-01 13:54 ` Jason Gunthorpe @ 2019-10-02 21:35 ` Navid Emamdoost 2019-10-03 8:20 ` Leon Romanovsky 2019-10-03 10:25 ` Potnuri Bharat Teja 0 siblings, 2 replies; 6+ messages in thread From: Navid Emamdoost @ 2019-10-02 21:35 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, Navid Emamdoost, Stephen McCamant, Kangjie Lu, Potnuri Bharat Teja, Doug Ledford, linux-rdma, LKML 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 -- Navid. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA: release allocated skb 2019-10-02 21:35 ` Navid Emamdoost @ 2019-10-03 8:20 ` Leon Romanovsky 2019-10-03 10:25 ` Potnuri Bharat Teja 1 sibling, 0 replies; 6+ messages in thread From: Leon Romanovsky @ 2019-10-03 8:20 UTC (permalink / raw) To: Navid Emamdoost Cc: Jason Gunthorpe, Navid Emamdoost, Stephen McCamant, Kangjie Lu, Potnuri Bharat Teja, Doug Ledford, linux-rdma, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA: release allocated skb 2019-10-02 21:35 ` Navid Emamdoost 2019-10-03 8:20 ` Leon Romanovsky @ 2019-10-03 10:25 ` Potnuri Bharat Teja 2019-10-04 18:12 ` Navid Emamdoost 1 sibling, 1 reply; 6+ messages in thread From: Potnuri Bharat Teja @ 2019-10-03 10:25 UTC (permalink / raw) To: Navid Emamdoost Cc: Jason Gunthorpe, Leon Romanovsky, Navid Emamdoost, Stephen McCamant, Kangjie Lu, Doug Ledford, linux-rdma, LKML 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA: release allocated skb 2019-10-03 10:25 ` Potnuri Bharat Teja @ 2019-10-04 18:12 ` Navid Emamdoost 0 siblings, 0 replies; 6+ messages in thread From: Navid Emamdoost @ 2019-10-04 18:12 UTC (permalink / raw) To: Potnuri Bharat Teja Cc: Jason Gunthorpe, Leon Romanovsky, Navid Emamdoost, Stephen McCamant, Kangjie Lu, Doug Ledford, linux-rdma, LKML 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. -- Navid. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-04 18:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190923050823.GL14368@unreal> 2019-09-23 15:52 ` [PATCH v2] RDMA: release allocated skb Navid Emamdoost 2019-10-01 13:54 ` Jason Gunthorpe 2019-10-02 21:35 ` Navid Emamdoost 2019-10-03 8:20 ` Leon Romanovsky 2019-10-03 10:25 ` Potnuri Bharat Teja 2019-10-04 18:12 ` Navid Emamdoost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).