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@xxxxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
