RDMA: release allocated skb

PATCH In review Navid Emamdoost 4 participants 6 messages Sep 23, 2019 – Oct 4, 2019 1 message not in the archive

Subsystems (2)

InfiniBand CXGB4 iWARP RNIC driver (IW_CXGB4)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20190923050823.GL14368@unreal

Navid Emamdoost

author

2019-09-23 15:53 UTC #1 lore raw

PATCH v2 1/1 RDMA: release allocated skb

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 <[email protected]>

---
 drivers/infiniband/hw/cxgb4/cq.c | 1 +
 1 file changed, 1 insertion(+)

drivers/infiniband/hw/cxgb4/cq.c +1 -0

index b1bb61c65f4f..1886c1af10bc 100644
@@ -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:

Signature · 1 line

2.17.1

Navid Emamdoost

author

2019-10-02 21:35 UTC #3 lore raw

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

Quoted 42 lines · Jason Gunthorpe

Quoted 3 lines · Navid Emamdoost
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
Quoted 8 lines
Signed-off-by Navid Emamdoost <[email protected]>
 drivers/infiniband/hw/cxgb4/cq.c | 1 +
 1 file changed, 1 insertion(+)
drivers/infiniband/hw/cxgb4/cq.c +1 -0
index b1bb61c65f4f..1886c1af10bc 100644
+++ b/drivers/infiniband/hw/cxgb4/cq.c
drivers/infiniband/hw/cxgb4/cq.c static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, · line 179
Quoted 7 lines
drivers/infiniband/hw/cxgb4/cq.c +1 -0
@@ -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

Signature · 1 line

Navid.

Leon Romanovsky

2019-10-03 08:20 UTC #4 lore raw

Quoted 9 lines · Navid Emamdoost

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

Potnuri Bharat Teja

maintainer

2019-10-03 10:25 UTC #5 lore raw

In reply to Navid Emamdoost

Quoted 9 lines · Navid Emamdoost

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.

Quoted 53 lines

Thanks,
Navid
Quoted 42 lines · Jason Gunthorpe
Quoted 3 lines · Navid Emamdoost
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
Quoted 8 lines
Signed-off-by Navid Emamdoost <[email protected]>
 drivers/infiniband/hw/cxgb4/cq.c | 1 +
 1 file changed, 1 insertion(+)
drivers/infiniband/hw/cxgb4/cq.c +1 -0
index b1bb61c65f4f..1886c1af10bc 100644
+++ b/drivers/infiniband/hw/cxgb4/cq.c
drivers/infiniband/hw/cxgb4/cq.c static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, · line 179
Quoted 7 lines
drivers/infiniband/hw/cxgb4/cq.c +1 -0
@@ -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
Signature · 1 line
Navid.

Navid Emamdoost

author

2019-10-04 18:13 UTC #6 lore raw

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.

Quoted 75 lines · Potnuri Bharat Teja

Quoted 9 lines · Navid Emamdoost
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.
Quoted 53 lines
Thanks,
Navid
Quoted 41 lines · Jason Gunthorpe
Quoted 3 lines · Navid Emamdoost
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
Quoted 8 lines
Signed-off-by Navid Emamdoost <[email protected]>
 drivers/infiniband/hw/cxgb4/cq.c | 1 +
 1 file changed, 1 insertion(+)
drivers/infiniband/hw/cxgb4/cq.c +1 -0
index b1bb61c65f4f..1886c1af10bc 100644
+++ b/drivers/infiniband/hw/cxgb4/cq.c
drivers/infiniband/hw/cxgb4/cq.c static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq, · line 179
Quoted 7 lines
drivers/infiniband/hw/cxgb4/cq.c +1 -0
@@ -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;
}
Quoted 1 line
Jason
--
Navid.

Signature · 1 line

Navid.