| Message ID | 20170308052152.GA31503@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Mar 08, 2017 at 08:21:52AM +0300, Dan Carpenter wrote: > "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 <dan.carpenter@oracle.com> > --- > Static analysis. Not tested. Please review carefully. > > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c > index dbfde0dc6ff7..9f95f50b2909 100644 > --- a/drivers/infiniband/sw/rxe/rxe_req.c > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > @@ -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? > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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? > > 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 >> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 8, 2017 at 7:21 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > "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 <dan.carpenter@oracle.com> > --- > Static analysis. Not tested. Please review carefully. Ack-by: Moni Shoua <monis@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-03-08 at 08:21 +0300, Dan Carpenter wrote: > "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 <dan.carpenter@oracle.com> Thanks, applied to -rc.
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c index dbfde0dc6ff7..9f95f50b2909 100644 --- a/drivers/infiniband/sw/rxe/rxe_req.c +++ b/drivers/infiniband/sw/rxe/rxe_req.c @@ -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; }
"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 <dan.carpenter@oracle.com> --- Static analysis. Not tested. Please review carefully. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html