Re: [PATCH for-next] RDMA/rxe: Fix ib_device reference counting (again)

[Bob Pearson <rpearsonhpe@gmail.com>]

On 2/14/21 4:26 PM, Bob Pearson wrote:
> Three errors occurred in the fix referenced below.
> 
> 1) The on and off again 'if (skb)' got dropped but was really
> needed in rxe_rcv_mcast_pkt() to prevent calling ib_device_put()
> on the non-error path.
> 
> 2) Extending the reference taken by rxe_get_dev_from_net() in
> rxe_udp_encap_recv() until each skb is freed was not matched by
> a reference in the loopback path resulting in underflows.
> 
> 3) In rxe_comp.c the function free_pkt() did not clear skb which
> triggered a warning at done: and could possibly at exit: in
> rxe_completer(). The WARN_ONCE() calls are not required at done:
> and only in one place before going to exit.
> 
> This patch fixes these errors.
> 
> Fixes: 899aba891cab ("RDMA/rxe: Fix FIXME in rxe_udp_encap_recv()")
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_comp.c | 5 +++--
>  drivers/infiniband/sw/rxe/rxe_net.c  | 7 ++++++-
>  drivers/infiniband/sw/rxe/rxe_recv.c | 6 ++++--
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_comp.c b/drivers/infiniband/sw/rxe/rxe_comp.c
> index a8ac791a1bb9..13fc5a1cced1 100644
> --- a/drivers/infiniband/sw/rxe/rxe_comp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_comp.c
> @@ -671,6 +671,9 @@ int rxe_completer(void *arg)
>  			 * it down the road or let it expire
>  			 */
>  
> +			/* warn if we did receive a packet */
> +			WARN_ON_ONCE(skb);
> +
>  			/* there is nothing to retry in this case */
>  			if (!wqe || (wqe->state == wqe_state_posted))
>  				goto exit;
> @@ -750,7 +753,6 @@ int rxe_completer(void *arg)
>  	/* we come here if we are done with processing and want the task to
>  	 * exit from the loop calling us
>  	 */
> -	WARN_ON_ONCE(skb);
>  	rxe_drop_ref(qp);
>  	return -EAGAIN;
>  
> @@ -758,7 +760,6 @@ int rxe_completer(void *arg)
>  	/* we come here if we have processed a packet we want the task to call
>  	 * us again to see if there is anything else to do
>  	 */
> -	WARN_ON_ONCE(skb);
>  	rxe_drop_ref(qp);
>  	return 0;
>  }
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 36d56163afac..8e81df578552 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -406,12 +406,17 @@ int rxe_send(struct rxe_pkt_info *pkt, struct sk_buff *skb)
>  
>  void rxe_loopback(struct sk_buff *skb)
>  {
> +	struct rxe_pkt_info *pkt = SKB_TO_PKT(skb);
> +
>  	if (skb->protocol == htons(ETH_P_IP))
>  		skb_pull(skb, sizeof(struct iphdr));
>  	else
>  		skb_pull(skb, sizeof(struct ipv6hdr));
>  
> -	rxe_rcv(skb);
> +	if (WARN_ON(!ib_device_try_get(&pkt->rxe->ib_dev)))
> +		kfree_skb(skb);
> +	else
> +		rxe_rcv(skb);
>  }
>  
>  struct sk_buff *rxe_init_packet(struct rxe_dev *rxe, struct rxe_av *av,
> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> index 8a48a33d587b..a5e330e3bbce 100644
> --- a/drivers/infiniband/sw/rxe/rxe_recv.c
> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> @@ -299,8 +299,10 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
>  
>  err1:
>  	/* free skb if not consumed */
> -	kfree_skb(skb);
> -	ib_device_put(&rxe->ib_dev);
> +	if (unlikely(skb)) {
> +		kfree_skb(skb);
> +		ib_device_put(&rxe->ib_dev);
> +	}
>  }
>  
>  /**
> 
Just a reminder. rxe in for-next is broken until this gets done.
thanks

bob