re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

[Dan Carpenter]

Hello Nicholas Bellinger,

The patch b8d26b3be8b3: "iser-target: Add iSCSI Extensions for RDMA
(iSER) target driver" from Mar 7, 2013, leads to the following static
checker warning:

	drivers/infiniband/ulp/isert/ib_isert.c:423 isert_device_get()
	error: passing non negative 1 to ERR_PTR

drivers/infiniband/ulp/isert/ib_isert.c
   417  
   418          device->ib_device = cma_id->device;
   419          ret = isert_create_device_ib_res(device);
   420          if (ret) {
   421                  kfree(device);
   422                  mutex_unlock(&device_list_mutex);
   423                  return ERR_PTR(ret);

The warning here is because isert_create_device_ib_res() returns either
a negative error code, zero or one.  The documentation is not clear what
that means.  AHAHAHAHAHAHAHAH.  I joke.  There is no documentation.

Anyway, it's definitely a bug and it leads to a NULL dereference in the
caller.

   424          }
   425  
   426          device->refcount++;
   427          list_add_tail(&device->dev_node, &device_list);
   428          isert_info("Created a new iser device %p refcount %d\n",
   429                     device, device->refcount);
   430          mutex_unlock(&device_list_mutex);
   431  
   432          return device;
   433  }

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: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

[Nicholas A. Bellinger]

Hi Dan,

(Adding Sagi CC')

On Tue, 2016-01-05 at 00:07 +0300, Dan Carpenter wrote:
> Hello Nicholas Bellinger,
> 
> The patch b8d26b3be8b3: "iser-target: Add iSCSI Extensions for RDMA
> (iSER) target driver" from Mar 7, 2013, leads to the following static
> checker warning:
> 
> 	drivers/infiniband/ulp/isert/ib_isert.c:423 isert_device_get()
> 	error: passing non negative 1 to ERR_PTR
> 
> drivers/infiniband/ulp/isert/ib_isert.c
>    417  
>    418          device->ib_device = cma_id->device;
>    419          ret = isert_create_device_ib_res(device);
>    420          if (ret) {
>    421                  kfree(device);
>    422                  mutex_unlock(&device_list_mutex);
>    423                  return ERR_PTR(ret);
> 
> The warning here is because isert_create_device_ib_res() returns either
> a negative error code, zero or one.  The documentation is not clear what
> that means.  AHAHAHAHAHAHAHAH.  I joke.  There is no documentation.
> 
> Anyway, it's definitely a bug and it leads to a NULL dereference in the
> caller.
> 
>    424          }
>    425  
>    426          device->refcount++;
>    427          list_add_tail(&device->dev_node, &device_list);
>    428          isert_info("Created a new iser device %p refcount %d\n",
>    429                     device, device->refcount);
>    430          mutex_unlock(&device_list_mutex);
>    431  
>    432          return device;
>    433  }
> 

Looking at the current code, AFAICT isert_alloc_comps() ->
ib_req_notify_cq() is the one case where this can happen, right..?

Here's a quick patch that I'm applying to target-pending/for-next, that
always returns negative upon isert_create_device_ib_res() failure.

Thanks Dan!

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.
index 91eb22c..8954e12 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -356,7 +356,7 @@ isert_create_device_ib_res(struct isert_device *device)
        dev_attr = &device->dev_attr;
        ret = isert_query_device(device->ib_device, dev_attr);
        if (ret)
-               return ret;
+               goto out;
 
        /* asign function handlers */
        if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS &&
@@ -372,7 +372,7 @@ isert_create_device_ib_res(struct isert_device *device)
 
        ret = isert_alloc_comps(device, dev_attr);
        if (ret)
-               return ret;
+               goto out;
 
        device->pd = ib_alloc_pd(device->ib_device);
        if (IS_ERR(device->pd)) {
@@ -390,6 +390,9 @@ isert_create_device_ib_res(struct isert_device *device)
 
 out_cq:
        isert_free_comps(device);
+out:
+       if (ret > 0)
+               ret = -EINVAL;
        return ret;
 }

Re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

[Nicholas A. Bellinger]

That silences the warning, thanks!

regards,
dan carpenter

Re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

[Dan Carpenter]

I see now that there is documentation in ib_req_notify_cq().

regards,
dan carpenter

Re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver

[Sagi Grimberg]

> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.
> index 91eb22c..8954e12 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -356,7 +356,7 @@ isert_create_device_ib_res(struct isert_device *device)
>          dev_attr = &device->dev_attr;
>          ret = isert_query_device(device->ib_device, dev_attr);
>          if (ret)
> -               return ret;
> +               goto out;
>
>          /* asign function handlers */
>          if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS &&
> @@ -372,7 +372,7 @@ isert_create_device_ib_res(struct isert_device *device)
>
>          ret = isert_alloc_comps(device, dev_attr);
>          if (ret)
> -               return ret;
> +               goto out;
>
>          device->pd = ib_alloc_pd(device->ib_device);
>          if (IS_ERR(device->pd)) {
> @@ -390,6 +390,9 @@ isert_create_device_ib_res(struct isert_device *device)
>
>   out_cq:
>          isert_free_comps(device);
> +out:
> +       if (ret > 0)
> +               ret = -EINVAL;
>          return ret;
>   }
>
> --

Looks fine,

Acked-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Thanks Nic, and thanks Dan for reporting!
--
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