| Message ID | 1528353172-16924-1-git-send-email-yanjun.zhu@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Jun 07, 2018 at 02:32:52AM -0400, Zhu Yanjun wrote: > In rxe_send, when network_type is not RDMA_NETWORK_IPV4 or > RDMA_NETWORK_IPV6, skb is freed and -EINVAL is returned. > Then rxe_xmit_packet will return -EINVAL, too. In rxe_requester, > this skb is double freed. > In rxe_requester, kfree_skb is needed only after fill_packet fails. > So kfree_skb is moved from label err to test fill_packet. The better solution is not to release skb in rxe_send(), but do it at the same place where you allocated/initialized skb. Thanks
On 2018/6/7 16:22, Leon Romanovsky wrote: > On Thu, Jun 07, 2018 at 02:32:52AM -0400, Zhu Yanjun wrote: >> In rxe_send, when network_type is not RDMA_NETWORK_IPV4 or >> RDMA_NETWORK_IPV6, skb is freed and -EINVAL is returned. >> Then rxe_xmit_packet will return -EINVAL, too. In rxe_requester, >> this skb is double freed. >> In rxe_requester, kfree_skb is needed only after fill_packet fails. >> So kfree_skb is moved from label err to test fill_packet. > The better solution is not to release skb in rxe_send(), but do it at > the same place where you allocated/initialized skb. Sure. I agree with you. But the function rxe_send is special. In this function, the following functions are called. rxe_send [rdma_rxe] ip_local_out __ip_local_out ip_output ip_finish_output ip_finish_output2 dev_queue_xmit __dev_queue_xmit dev_hard_start_xmit In the above functions, if error occurs in the above functions or iptables rules drop skb after ip_local_out, kfree_skb will be called. So it is not necessary to call kfree_skb in soft roce module again. Or else crash will occur. So if skb network_type is not RDMA_NETWORK_IPV4 or RDMA_NETWORK_IPV6, it had better be freed in rxe_send to keep compatible with RDMA_NETWORK_IPV4 or RDMA_NETWORK_IPV6 skb packets. int rxe_send(struct rxe_pkt_info *pkt, struct sk_buff *skb) { ... if (av->network_type == RDMA_NETWORK_IPV4) { err = ip_local_out(dev_net(skb_dst(skb)->dev), skb->sk, skb); <--if error, skb is freed } else if (av->network_type == RDMA_NETWORK_IPV6) { err = ip6_local_out(dev_net(skb_dst(skb)->dev), skb->sk, skb); <--if error, skb is freed } else { pr_err("Unknown layer 3 protocol: %d\n", av->network_type); atomic_dec(&pkt->qp->skb_out); rxe_drop_ref(pkt->qp); kfree_skb(skb); <---So it had better be freed here. return -EINVAL; } ... return 0; } Zhu Yanjun > > Thanks -- 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 Thu, Jun 07, 2018 at 04:39:49PM +0800, Yanjun Zhu wrote: > > > On 2018/6/7 16:22, Leon Romanovsky wrote: > > On Thu, Jun 07, 2018 at 02:32:52AM -0400, Zhu Yanjun wrote: > > > In rxe_send, when network_type is not RDMA_NETWORK_IPV4 or > > > RDMA_NETWORK_IPV6, skb is freed and -EINVAL is returned. > > > Then rxe_xmit_packet will return -EINVAL, too. In rxe_requester, > > > this skb is double freed. > > > In rxe_requester, kfree_skb is needed only after fill_packet fails. > > > So kfree_skb is moved from label err to test fill_packet. > > The better solution is not to release skb in rxe_send(), but do it at > > the same place where you allocated/initialized skb. > Sure. I agree with you. But the function rxe_send is special. In this > function, the following functions are called. > rxe_send [rdma_rxe] > ip_local_out > __ip_local_out > ip_output > ip_finish_output > ip_finish_output2 > dev_queue_xmit > __dev_queue_xmit > dev_hard_start_xmit > > In the above functions, if error occurs in the above functions or iptables > rules drop skb after ip_local_out, > kfree_skb will be called. So it is not necessary to call kfree_skb in soft > roce module again. Or else crash will occur. > > So if skb network_type is not RDMA_NETWORK_IPV4 or RDMA_NETWORK_IPV6, it had > better be freed in rxe_send to > keep compatible with RDMA_NETWORK_IPV4 or RDMA_NETWORK_IPV6 skb packets. > > int rxe_send(struct rxe_pkt_info *pkt, struct sk_buff *skb) > { > ... > if (av->network_type == RDMA_NETWORK_IPV4) { > err = ip_local_out(dev_net(skb_dst(skb)->dev), skb->sk, > skb); <--if error, skb is freed > } else if (av->network_type == RDMA_NETWORK_IPV6) { > err = ip6_local_out(dev_net(skb_dst(skb)->dev), skb->sk, > skb); <--if error, skb is freed > } else { > pr_err("Unknown layer 3 protocol: %d\n", av->network_type); > atomic_dec(&pkt->qp->skb_out); > rxe_drop_ref(pkt->qp); > kfree_skb(skb); <---So it had better be freed here. > return -EINVAL; > } > ... > > return 0; > } Thanks for the explanation. Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Thu, Jun 07, 2018 at 02:32:52AM -0400, Zhu Yanjun wrote: > In rxe_send, when network_type is not RDMA_NETWORK_IPV4 or > RDMA_NETWORK_IPV6, skb is freed and -EINVAL is returned. > Then rxe_xmit_packet will return -EINVAL, too. In rxe_requester, > this skb is double freed. > In rxe_requester, kfree_skb is needed only after fill_packet fails. > So kfree_skb is moved from label err to test fill_packet. > > Fixes: 5793b46 ("IB/rxe: remove unnecessary skb_clone in xmit") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > CC: Srinivas Eeda <srinivas.eeda@oracle.com> > CC: Junxiao Bi <junxiao.bi@oracle.com> > Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > --- > V1->V2: add the reporter > --- > drivers/infiniband/sw/rxe/rxe_req.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied to for-rc, thanks Jason -- 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
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c index 7851999..78210c1 100644 --- a/drivers/infiniband/sw/rxe/rxe_req.c +++ b/drivers/infiniband/sw/rxe/rxe_req.c @@ -709,6 +709,7 @@ int rxe_requester(void *arg) if (fill_packet(qp, wqe, &pkt, skb, payload)) { pr_debug("qp#%d Error during fill packet\n", qp_num(qp)); + kfree_skb(skb); goto err; } @@ -740,7 +741,6 @@ int rxe_requester(void *arg) goto next_wqe; err: - kfree_skb(skb); wqe->status = IB_WC_LOC_PROT_ERR; wqe->state = wqe_state_error; __rxe_do_task(&qp->comp.task);
In rxe_send, when network_type is not RDMA_NETWORK_IPV4 or RDMA_NETWORK_IPV6, skb is freed and -EINVAL is returned. Then rxe_xmit_packet will return -EINVAL, too. In rxe_requester, this skb is double freed. In rxe_requester, kfree_skb is needed only after fill_packet fails. So kfree_skb is moved from label err to test fill_packet. Fixes: 5793b46 ("IB/rxe: remove unnecessary skb_clone in xmit") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> CC: Srinivas Eeda <srinivas.eeda@oracle.com> CC: Junxiao Bi <junxiao.bi@oracle.com> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com> --- V1->V2: add the reporter --- drivers/infiniband/sw/rxe/rxe_req.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)