[PATCH for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed

[PATCH for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed

[Md Haris Iqbal]

The function rxe_get_av is only used by rxe_requester, and the ahp double
pointer is always sent. Hence there is no need to do the check.
rxe_requester also always performs the put for ah, hence that is also not
needed.

Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_av.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c
index 3b05314ca739..0780ffcf24a6 100644
--- a/drivers/infiniband/sw/rxe/rxe_av.c
+++ b/drivers/infiniband/sw/rxe/rxe_av.c
@@ -130,11 +130,7 @@ struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt, struct rxe_ah **ahp)
 			rxe_put(ah);
 			return NULL;
 		}
-
-		if (ahp)
-			*ahp = ah;
-		else
-			rxe_put(ah);
+		*ahp = ah;
 
 		return &ah->av;
 	}
-- 
2.25.1

Re: [PATCH for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed

[Bob Pearson]

On 10/20/22 10:13, Md Haris Iqbal wrote:
> The function rxe_get_av is only used by rxe_requester, and the ahp double
> pointer is always sent. Hence there is no need to do the check.
> rxe_requester also always performs the put for ah, hence that is also not
> needed.
> 
> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_av.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c
> index 3b05314ca739..0780ffcf24a6 100644
> --- a/drivers/infiniband/sw/rxe/rxe_av.c
> +++ b/drivers/infiniband/sw/rxe/rxe_av.c
> @@ -130,11 +130,7 @@ struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt, struct rxe_ah **ahp)
>  			rxe_put(ah);
>  			return NULL;
>  		}
> -
> -		if (ahp)
> -			*ahp = ah;
> -		else
> -			rxe_put(ah);
> +		*ahp = ah;
>  
>  		return &ah->av;
>  	}

That doesn't sound right. There are several cases depending on the version of the user library
and whether the QP is UD or RC/UC. The old driver/library computed the address vector in
user space and passed it back to the kernel in the WR. If both the kernel and user library are using
the new API the user space passes back the AH# in the WR for UD commands. In both cases for
connected QPs the AV is stored in the rxe_struct_qp and there is no AH. At the point where
rxe_get_av is called the requester needs the AV it gets it from one of the three places:
the QP, the WR (old), or the kernel AH after looking up the AH#. If the kernel AH was involved
its pointer is returned so the requester can continue to hold a reference to it until it
is through sending the packet and then it can drop the reference. This to protect from
someone calling destroy_ah in a race with the send queue. Hope this makes it clearer.

Bob

[kbuild] Re: [PATCH for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed

[Dan Carpenter]

Hi Md,

https://git-scm.com/docs/git-format-patch#_base_tree_information  ]

url:    https://github.com/intel-lab-lkp/linux/commits/Md-Haris-Iqbal/RDMA-rxe-rxe_get_av-always-receives-ahp-hence-no-put-is-needed/20221020-231859  
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git   for-next
patch link:    https://lore.kernel.org/r/20221020151345.412731-1-haris.phnx%40gmail.com  
patch subject: [PATCH for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed
config: openrisc-randconfig-m041-20221024
compiler: or1k-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/infiniband/sw/rxe/rxe_av.c:133 rxe_get_av() error: we previously assumed 'ahp' could be null (see line 107)

vim +/ahp +133 drivers/infiniband/sw/rxe/rxe_av.c

63221acb0c6314 Bob Pearson 2022-03-03  102  struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt, struct rxe_ah **ahp)
8700e3e7c4857d Moni Shoua  2016-06-16  103  {
e2fe06c9080694 Bob Pearson 2021-10-07  104  	struct rxe_ah *ah;
e2fe06c9080694 Bob Pearson 2021-10-07  105  	u32 ah_num;
e2fe06c9080694 Bob Pearson 2021-10-07  106  
63221acb0c6314 Bob Pearson 2022-03-03 @107  	if (ahp)
                                                    ^^^
Check for NULL.  Maybe this check can be deleted.  It's never NULL in
the current code.

63221acb0c6314 Bob Pearson 2022-03-03  108  		*ahp = NULL;
63221acb0c6314 Bob Pearson 2022-03-03  109  
8700e3e7c4857d Moni Shoua  2016-06-16  110  	if (!pkt || !pkt->qp)
8700e3e7c4857d Moni Shoua  2016-06-16  111  		return NULL;
8700e3e7c4857d Moni Shoua  2016-06-16  112  
8700e3e7c4857d Moni Shoua  2016-06-16  113  	if (qp_type(pkt->qp) == IB_QPT_RC || qp_type(pkt->qp) == IB_QPT_UC)
8700e3e7c4857d Moni Shoua  2016-06-16  114  		return &pkt->qp->pri_av;
8700e3e7c4857d Moni Shoua  2016-06-16  115  
e2fe06c9080694 Bob Pearson 2021-10-07  116  	if (!pkt->wqe)
e2fe06c9080694 Bob Pearson 2021-10-07  117  		return NULL;
e2fe06c9080694 Bob Pearson 2021-10-07  118  
e2fe06c9080694 Bob Pearson 2021-10-07  119  	ah_num = pkt->wqe->wr.wr.ud.ah_num;
e2fe06c9080694 Bob Pearson 2021-10-07  120  	if (ah_num) {
                                                    ^^^^^^
Perhaps it is a false positive if checking "ah_num" is intended to be
equivalent to checking "ahp"?

e2fe06c9080694 Bob Pearson 2021-10-07  121  		/* only new user provider or kernel client */
e2fe06c9080694 Bob Pearson 2021-10-07  122  		ah = rxe_pool_get_index(&pkt->rxe->ah_pool, ah_num);
63221acb0c6314 Bob Pearson 2022-03-03  123  		if (!ah) {
e2fe06c9080694 Bob Pearson 2021-10-07  124  			pr_warn("Unable to find AH matching ah_num\n");
e2fe06c9080694 Bob Pearson 2021-10-07  125  			return NULL;
e2fe06c9080694 Bob Pearson 2021-10-07  126  		}
63221acb0c6314 Bob Pearson 2022-03-03  127  
63221acb0c6314 Bob Pearson 2022-03-03  128  		if (rxe_ah_pd(ah) != pkt->qp->pd) {
63221acb0c6314 Bob Pearson 2022-03-03  129  			pr_warn("PDs don't match for AH and QP\n");
3197706abd0532 Bob Pearson 2022-03-03  130  			rxe_put(ah);
63221acb0c6314 Bob Pearson 2022-03-03  131  			return NULL;
63221acb0c6314 Bob Pearson 2022-03-03  132  		}
63221acb0c6314 Bob Pearson 2022-03-03 @133  		*ahp = ah;
                                                        ^^^^
Unchecked dereference.

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp