| Message ID | 20221020151345.412731-1-haris.phnx@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [for-next] RDMA/rxe: rxe_get_av always receives ahp hence no put is needed | expand |
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
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.
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; }
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(-)