[bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Dan Carpenter]

Hello Devesh Sharma,

The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
adapters" from Jan 11, 2018, leads to the following static checker
warning:

	drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
	warn: 'srq->umem' isn't an ERR_PTR

drivers/infiniband/hw/bnxt_re/ib_verbs.c
  1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
  1314                  return rc;
  1315          }
  1316  
  1317          if (srq->umem && !IS_ERR(srq->umem))
                                  ^^^^^^^^^^^^^^^^
We never store error pointers to srq->umem.  It's pretty consistently
checked for error pointers though so maybe that's fine.  It causes a
static checker warning because error pointer confusion is a pretty
common source of bugs.  Anyway, feel free to ignore if you want...

  1318                  ib_umem_release(srq->umem);
  1319          kfree(srq);
  1320          atomic_dec(&rdev->srq_count);
  1321          if (nq)
  1322                  nq->budget--;
  1323          return 0;
  1324  }
  1325  
  1326  static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
  1327                                   struct bnxt_re_pd *pd,
  1328                                   struct bnxt_re_srq *srq,
  1329                                   struct ib_udata *udata)
  1330  {
  1331          struct bnxt_re_srq_req ureq;
  1332          struct bnxt_qplib_srq *qplib_srq = &srq->qplib_srq;
  1333          struct ib_umem *umem;
  1334          int bytes = 0;
  1335          struct ib_ucontext *context = pd->ib_pd.uobject->context;
  1336          struct bnxt_re_ucontext *cntx = container_of(context,
  1337                                                       struct bnxt_re_ucontext,
  1338                                                       ib_uctx);
  1339          if (ib_copy_from_udata(&ureq, udata, sizeof(ureq)))
  1340                  return -EFAULT;
  1341  
  1342          bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
  1343          bytes = PAGE_ALIGN(bytes);
  1344          umem = ib_umem_get(context, ureq.srqva, bytes,
  1345                             IB_ACCESS_LOCAL_WRITE, 1);
  1346          if (IS_ERR(umem))
  1347                  return PTR_ERR(umem);
  1348  
  1349          srq->umem = umem;
                ^^^^^^^^^^^^^^^^
Set here, I guess.

  1350          qplib_srq->nmap = umem->nmap;

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Devesh Sharma]

On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> Hello Devesh Sharma,
>
> The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> adapters" from Jan 11, 2018, leads to the following static checker
> warning:
>
>         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
>         warn: 'srq->umem' isn't an ERR_PTR
>
> drivers/infiniband/hw/bnxt_re/ib_verbs.c
>   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
>   1314                  return rc;
>   1315          }
>   1316
>   1317          if (srq->umem && !IS_ERR(srq->umem))
>                                   ^^^^^^^^^^^^^^^^
> We never store error pointers to srq->umem.  It's pretty consistently
> checked for error pointers though so maybe that's fine.  It causes a
> static checker warning because error pointer confusion is a pretty
> common source of bugs.  Anyway, feel free to ignore if you want...
Thanks for reporting Dan,

Is there a way out, I want to call ib_umem_release only if it was valid.
I think if ib_umem_release checks for the validity of pointer then I
can get rid of this?
There are other places also in bnxt_re driver where such checks are present.
>
>   1318                  ib_umem_release(srq->umem);
>   1319          kfree(srq);
>   1320          atomic_dec(&rdev->srq_count);
>   1321          if (nq)
>   1322                  nq->budget--;
>   1323          return 0;
>   1324  }
>   1325
>   1326  static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
>   1327                                   struct bnxt_re_pd *pd,
>   1328                                   struct bnxt_re_srq *srq,
>   1329                                   struct ib_udata *udata)
>   1330  {
>   1331          struct bnxt_re_srq_req ureq;
>   1332          struct bnxt_qplib_srq *qplib_srq = &srq->qplib_srq;
>   1333          struct ib_umem *umem;
>   1334          int bytes = 0;
>   1335          struct ib_ucontext *context = pd->ib_pd.uobject->context;
>   1336          struct bnxt_re_ucontext *cntx = container_of(context,
>   1337                                                       struct bnxt_re_ucontext,
>   1338                                                       ib_uctx);
>   1339          if (ib_copy_from_udata(&ureq, udata, sizeof(ureq)))
>   1340                  return -EFAULT;
>   1341
>   1342          bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
>   1343          bytes = PAGE_ALIGN(bytes);
>   1344          umem = ib_umem_get(context, ureq.srqva, bytes,
>   1345                             IB_ACCESS_LOCAL_WRITE, 1);
>   1346          if (IS_ERR(umem))
>   1347                  return PTR_ERR(umem);
>   1348
>   1349          srq->umem = umem;
>                 ^^^^^^^^^^^^^^^^
> Set here, I guess.
Yeah, the checker is confused due to this.
>
>   1350          qplib_srq->nmap = umem->nmap;
>
> regards,
> dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Dan Carpenter]

On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > Hello Devesh Sharma,
> >
> > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > adapters" from Jan 11, 2018, leads to the following static checker
> > warning:
> >
> >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> >         warn: 'srq->umem' isn't an ERR_PTR
> >
> > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> >   1314                  return rc;
> >   1315          }
> >   1316
> >   1317          if (srq->umem && !IS_ERR(srq->umem))
> >                                   ^^^^^^^^^^^^^^^^
> > We never store error pointers to srq->umem.  It's pretty consistently
> > checked for error pointers though so maybe that's fine.  It causes a
> > static checker warning because error pointer confusion is a pretty
> > common source of bugs.  Anyway, feel free to ignore if you want...
> Thanks for reporting Dan,
> 
> Is there a way out, I want to call ib_umem_release only if it was valid.
> I think if ib_umem_release checks for the validity of pointer then I
> can get rid of this?
> There are other places also in bnxt_re driver where such checks are present.

Yeah.  Those places generate warnings as well, but I thought one was
enough.  It's fine if you want to ignore the warning, no one will be
upset.  :P

[ snip ]

> >   1342          bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
> >   1343          bytes = PAGE_ALIGN(bytes);
> >   1344          umem = ib_umem_get(context, ureq.srqva, bytes,
> >   1345                             IB_ACCESS_LOCAL_WRITE, 1);
> >   1346          if (IS_ERR(umem))
> >   1347                  return PTR_ERR(umem);
> >   1348
> >   1349          srq->umem = umem;
> >                 ^^^^^^^^^^^^^^^^
> > Set here, I guess.
> Yeah, the checker is confused due to this.

It does bother me that you're saying the "checker is confused".  The
checker is printing a 100% accurate, factual warning...  :/  We have an
IS_ERR() check when the pointer can not possibly be an error pointer.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > > Hello Devesh Sharma,
> > >
> > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > adapters" from Jan 11, 2018, leads to the following static checker
> > > warning:
> > >
> > >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > >         warn: 'srq->umem' isn't an ERR_PTR
> > >
> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > >   1314                  return rc;
> > >   1315          }
> > >   1316
> > >   1317          if (srq->umem && !IS_ERR(srq->umem))
> > >                                   ^^^^^^^^^^^^^^^^
> > > We never store error pointers to srq->umem.  It's pretty consistently
> > > checked for error pointers though so maybe that's fine.  It causes a
> > > static checker warning because error pointer confusion is a pretty
> > > common source of bugs.  Anyway, feel free to ignore if you want...
> > Thanks for reporting Dan,
> >
> > Is there a way out, I want to call ib_umem_release only if it was valid.
> > I think if ib_umem_release checks for the validity of pointer then I
> > can get rid of this?
> > There are other places also in bnxt_re driver where such checks are present.
>
> Yeah.  Those places generate warnings as well, but I thought one was
> enough.  It's fine if you want to ignore the warning, no one will be
> upset.  :P

Not really, we are trying to clean the subsystem from the warnings
and driver authors who ignore such warnings simply and very effective
sabotage it.

Currently my checks print ~400 warnings for the drivers/infiniband/* +
drivers/net/ethernet/mellanox/*

So please don't increase this number, or fix the driver or fix the tool :)

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3387 bytes --]

On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
> On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > Hello Devesh Sharma,
> > > > 
> > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > warning:
> > > > 
> > > >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > >         warn: 'srq->umem' isn't an ERR_PTR
> > > > 
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > >   1314                  return rc;
> > > >   1315          }
> > > >   1316
> > > >   1317          if (srq->umem && !IS_ERR(srq->umem))
> > > >                                   ^^^^^^^^^^^^^^^^
> > > > We never store error pointers to srq->umem.  It's pretty consistently
> > > > checked for error pointers though so maybe that's fine.  It causes a
> > > > static checker warning because error pointer confusion is a pretty
> > > > common source of bugs.  Anyway, feel free to ignore if you want...
> > > 
> > > Thanks for reporting Dan,
> > > 
> > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > I think if ib_umem_release checks for the validity of pointer then I
> > > can get rid of this?
> > > There are other places also in bnxt_re driver where such checks are present.
> > 
> > Yeah.  Those places generate warnings as well, but I thought one was
> > enough.  It's fine if you want to ignore the warning, no one will be
> > upset.  :P
> 
> Not really, we are trying to clean the subsystem from the warnings
> and driver authors who ignore such warnings simply and very effective
> sabotage it.
> 
> Currently my checks print ~400 warnings for the drivers/infiniband/* +
> drivers/net/ethernet/mellanox/*
> 
> So please don't increase this number, or fix the driver or fix the tool :)
> 
> Thanks

Looking at the code, the proper fix for this is:

[dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 9b8fa77b8831..ae9e9ff54826 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
                return rc;
        }
 
-       if (srq->umem && !IS_ERR(srq->umem))
+       if (srq->umem)
                ib_umem_release(srq->umem);
        kfree(srq);
        atomic_dec(&rdev->srq_count);
@@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
*ib_pd,
        return &srq->ib_srq;
 
 fail:
-       if (udata && srq->umem && !IS_ERR(srq->umem)) {
+       if (srq->umem)
                ib_umem_release(srq->umem);
-               srq->umem = NULL;
-       }
-
        kfree(srq);
 exit:
        return ERR_PTR(rc);
[dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ 

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

On Wed, 2018-01-31 at 11:04 -0500, Doug Ledford wrote:
> On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
> > On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > Hello Devesh Sharma,
> > > > > 
> > > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > > warning:
> > > > > 
> > > > >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > > >         warn: 'srq->umem' isn't an ERR_PTR
> > > > > 
> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > > >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > > >   1314                  return rc;
> > > > >   1315          }
> > > > >   1316
> > > > >   1317          if (srq->umem && !IS_ERR(srq->umem))
> > > > >                                   ^^^^^^^^^^^^^^^^
> > > > > We never store error pointers to srq->umem.  It's pretty consistently
> > > > > checked for error pointers though so maybe that's fine.  It causes a
> > > > > static checker warning because error pointer confusion is a pretty
> > > > > common source of bugs.  Anyway, feel free to ignore if you want...
> > > > 
> > > > Thanks for reporting Dan,
> > > > 
> > > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > > I think if ib_umem_release checks for the validity of pointer then I
> > > > can get rid of this?
> > > > There are other places also in bnxt_re driver where such checks are present.
> > > 
> > > Yeah.  Those places generate warnings as well, but I thought one was
> > > enough.  It's fine if you want to ignore the warning, no one will be
> > > upset.  :P
> > 
> > Not really, we are trying to clean the subsystem from the warnings
> > and driver authors who ignore such warnings simply and very effective
> > sabotage it.
> > 
> > Currently my checks print ~400 warnings for the drivers/infiniband/* +
> > drivers/net/ethernet/mellanox/*
> > 
> > So please don't increase this number, or fix the driver or fix the tool :)
> > 
> > Thanks
> 
> Looking at the code, the proper fix for this is:
> 
> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 9b8fa77b8831..ae9e9ff54826 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
>                 return rc;
>         }
>  
> -       if (srq->umem && !IS_ERR(srq->umem))
> +       if (srq->umem)
>                 ib_umem_release(srq->umem);
>         kfree(srq);
>         atomic_dec(&rdev->srq_count);
> @@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
> *ib_pd,
>         return &srq->ib_srq;
>  
>  fail:
> -       if (udata && srq->umem && !IS_ERR(srq->umem)) {
> +       if (srq->umem)
>                 ib_umem_release(srq->umem);
> -               srq->umem = NULL;
> -       }
> -
>         kfree(srq);
>  exit:
>         return ERR_PTR(rc);
> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ 
> 

This was committed in my tree for the next merge pull request.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Devesh Sharma]

On Thu, Feb 1, 2018 at 2:37 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2018-01-31 at 11:04 -0500, Doug Ledford wrote:
>> On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
>> > On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
>> > > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
>> > > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
>> > > > > Hello Devesh Sharma,
>> > > > >
>> > > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
>> > > > > adapters" from Jan 11, 2018, leads to the following static checker
>> > > > > warning:
>> > > > >
>> > > > >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
>> > > > >         warn: 'srq->umem' isn't an ERR_PTR
>> > > > >
>> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> > > > >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
>> > > > >   1314                  return rc;
>> > > > >   1315          }
>> > > > >   1316
>> > > > >   1317          if (srq->umem && !IS_ERR(srq->umem))
>> > > > >                                   ^^^^^^^^^^^^^^^^
>> > > > > We never store error pointers to srq->umem.  It's pretty consistently
>> > > > > checked for error pointers though so maybe that's fine.  It causes a
>> > > > > static checker warning because error pointer confusion is a pretty
>> > > > > common source of bugs.  Anyway, feel free to ignore if you want...
>> > > >
>> > > > Thanks for reporting Dan,
>> > > >
>> > > > Is there a way out, I want to call ib_umem_release only if it was valid.
>> > > > I think if ib_umem_release checks for the validity of pointer then I
>> > > > can get rid of this?
>> > > > There are other places also in bnxt_re driver where such checks are present.
>> > >
>> > > Yeah.  Those places generate warnings as well, but I thought one was
>> > > enough.  It's fine if you want to ignore the warning, no one will be
>> > > upset.  :P
>> >
>> > Not really, we are trying to clean the subsystem from the warnings
>> > and driver authors who ignore such warnings simply and very effective
>> > sabotage it.
>> >
>> > Currently my checks print ~400 warnings for the drivers/infiniband/* +
>> > drivers/net/ethernet/mellanox/*
>> >
>> > So please don't increase this number, or fix the driver or fix the tool :)
>> >
>> > Thanks
>>
>> Looking at the code, the proper fix for this is:
>>
>> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
>> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> index 9b8fa77b8831..ae9e9ff54826 100644
>> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> @@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
>>                 return rc;
>>         }
>>
>> -       if (srq->umem && !IS_ERR(srq->umem))
>> +       if (srq->umem)
>>                 ib_umem_release(srq->umem);
>>         kfree(srq);
>>         atomic_dec(&rdev->srq_count);
>> @@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
>> *ib_pd,
>>         return &srq->ib_srq;
>>
>>  fail:
>> -       if (udata && srq->umem && !IS_ERR(srq->umem)) {
>> +       if (srq->umem)
>>                 ib_umem_release(srq->umem);
>> -               srq->umem = NULL;
>> -       }
>> -
>>         kfree(srq);
>>  exit:
>>         return ERR_PTR(rc);
>> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$
>>
>
> This was committed in my tree for the next merge pull request.

Thanks Doug, I will review the driver code once and see if I can
supply the fix for rest of the occurrences.
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

[Jason Gunthorpe]

On Wed, Jan 31, 2018 at 08:48:29AM +0200, Leon Romanovsky wrote:
> On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > > > Hello Devesh Sharma,
> > > >
> > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > warning:
> > > >
> > > >         drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > >         warn: 'srq->umem' isn't an ERR_PTR
> > > >
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > >   1313                  dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > >   1314                  return rc;
> > > >   1315          }
> > > >   1316
> > > >   1317          if (srq->umem && !IS_ERR(srq->umem))
> > > >                                   ^^^^^^^^^^^^^^^^
> > > > We never store error pointers to srq->umem.  It's pretty consistently
> > > > checked for error pointers though so maybe that's fine.  It causes a
> > > > static checker warning because error pointer confusion is a pretty
> > > > common source of bugs.  Anyway, feel free to ignore if you want...
> > > Thanks for reporting Dan,
> > >
> > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > I think if ib_umem_release checks for the validity of pointer then I
> > > can get rid of this?
> > > There are other places also in bnxt_re driver where such checks are present.
> >
> > Yeah.  Those places generate warnings as well, but I thought one was
> > enough.  It's fine if you want to ignore the warning, no one will be
> > upset.  :P
> 
> Not really, we are trying to clean the subsystem from the warnings
> and driver authors who ignore such warnings simply and very effective
> sabotage it.
> 
> Currently my checks print ~400 warnings for the drivers/infiniband/* +
> drivers/net/ethernet/mellanox/*
> 
> So please don't increase this number, or fix the driver or fix the tool :)

Yes..

Generally speaking, in rdma, I discourage storing ERR_PTR in kalloc
memory, structures, etc. Should be 0 or a valid value, or need a
really good reason why ERR_PTR should be stored.

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html