Skip to contentSkip to conversation

IB/rxe: double free on error

PATCH Applied Dan Carpenter 4 participants 5 messages Mar 8, 2017 – Mar 25, 2017 1 message not in the archive

Subsystems (3)
Kernel janitors InfiniBand Soft-RoCE driver (rxe)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 IB/rxe: double free on error

"goto err;" has it's own kfree_skb() call so it's a double free. We only need to free on the "goto exit;" path.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested. Please review carefully.

drivers/infiniband/sw/rxe/rxe_req.c +1 -1
index dbfde0dc6ff7..9f95f50b2909 100644
@@ -729,11 +729,11 @@ int rxe_requester(void *arg)
 	ret = rxe_xmit_packet(to_rdev(qp->ibqp.device), qp, &pkt, skb);
 	if (ret) {
 		qp->need_req_skb = 1;
-		kfree_skb(skb);
 
 		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
 
 		if (ret = -EAGAIN) {
+			kfree_skb(skb);
 			rxe_run_task(&qp->req.task, 1);
 			goto exit;
 		}
#1 lore raw
Quoted 13 lines · Dan Carpenter

"goto err;" has it's own kfree_skb() call so it's a double free. We only need to free on the "goto exit;" path.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested. Please review carefully.

drivers/infiniband/sw/rxe/rxe_req.c +0 -0
index dbfde0dc6ff7..9f95f50b2909 100644

drivers/infiniband/sw/rxe/rxe_req.c int rxe_requester(void *arg) · line 729

Quoted 1 line
drivers/infiniband/sw/rxe/rxe_req.c +0 -0
@@ -729,11 +729,11 @@ int rxe_requester(void *arg)
 	ret = rxe_xmit_packet(to_rdev(qp->ibqp.device), qp, &pkt, skb);

This rxe_xmit_packet() looks a little bit awkward. It calls to kfree_skb and returns ret = 0 after drop decision. It doesn't free on error (ret != 0), but this rxe_requester does.

However in case of not error, the skb won't be released and goto next_cqe will be called, which has a lot of exit paths without freeing skb.

Moni, Yonatan
Is it done on purpose?

Quoted 15 lines
drivers/infiniband/sw/rxe/rxe_req.c +1 -2
 	if (ret) {
 		qp->need_req_skb = 1;
-		kfree_skb(skb);
 
 		rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
 
 		if (ret == -EAGAIN) {
+			kfree_skb(skb);
 			rxe_run_task(&qp->req.task, 1);
 			goto exit;
 		}
--

To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170308065829.GV14379-U/[email protected]

#3 lore raw
Quoted 4 lines

This rxe_xmit_packet() looks a little bit awkward. It calls to kfree_skb and returns ret = 0 after drop decision. It doesn't free on error (ret != 0), but this rxe_requester does.

Quoted 8 lines

However in case of not error, the skb won't be released and goto next_cqe will be called, which has a lot of exit paths without freeing skb.

Moni, Yonatan
Is it done on purpose?

In good flow skb will be freed when going down the chain call from rxe_xmit_packet(). This is the design.
In bad flow we can immediately free the skb

Quoted 15 lines
Quoted 15 lines
      if (ret) {
              qp->need_req_skb = 1;
drivers/infiniband/sw/rxe/rxe_req.c +1 -2
-             kfree_skb(skb);
 
              rollback_state(wqe, qp, &rollback_wqe, rollback_psn);
 
              if (ret = -EAGAIN) {
+                     kfree_skb(skb);
                      rxe_run_task(&qp->req.task, 1);
                      goto exit;
              }
--

To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Applied
#5 lore raw
In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

"goto err;" has it's own kfree_skb() call so it's a double free.  We only need to free on the "goto exit;" path.

Fixes 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by Dan Carpenter <[email protected]>

Thanks, applied to -rc.

Signature · 4 lines
Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD