The iteration through the list of "iser_device"s during device
lookup/creation is broken - it might result in an infinite loop if more
than 1 HCA is used with iSER. Use list_for_each_entry() instead of the
custom, flawed list iteration code.
Signed-off-by: Arne Redlich <[email protected]>
---
drivers/infiniband/ulp/iser/iser_verbs.c | 36 ++++++++++++-----------------
1 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 714b8db..1c0f968 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -237,33 +237,27 @@ static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
static
struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
{
- struct list_head *p_list;
- struct iser_device *device = NULL;
+ struct iser_device *device;
mutex_lock(&ig.device_list_mutex);
- p_list = ig.device_list.next;
- while (p_list != &ig.device_list) {
- device = list_entry(p_list, struct iser_device, ig_list);
- /* find if there's a match using the node GUID */
+ list_for_each_entry(device, &ig.device_list, ig_list)
if (device->ib_device->node_guid == cma_id->device->node_guid)
- break;
- }
-
- if (device == NULL) {
- device = kzalloc(sizeof *device, GFP_KERNEL);
- if (device == NULL)
goto out;
- /* assign this device to the device */
- device->ib_device = cma_id->device;
- /* init the device and link it into ig device list */
- if (iser_create_device_ib_res(device)) {
- kfree(device);
- device = NULL;
- goto out;
- }
- list_add(&device->ig_list, &ig.device_list);
+
+ device = kzalloc(sizeof *device, GFP_KERNEL);
+ if (device == NULL)
+ goto out;
+
+ device->ib_device = cma_id->device;
+ /* init the device and link it into ig device list */
+ if (iser_create_device_ib_res(device)) {
+ kfree(device);
+ device = NULL;
+ goto out;
}
+ list_add(&device->ig_list, &ig.device_list);
+
out:
BUG_ON(device == NULL);
device->refcount++;
--
1.5.4.1
Arne Redlich wrote:
> The iteration through the list of "iser_device"s during device
> lookup/creation is broken - it might result in an infinite loop if more
> than 1 HCA is used with iSER. Use list_for_each_entry() instead of the
> custom, flawed list iteration code.
>
> Signed-off-by: Arne Redlich <[email protected]>
> ---
> drivers/infiniband/ulp/iser/iser_verbs.c | 36 ++++++++++++-----------------
> 1 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
> index 714b8db..1c0f968 100644
> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -237,33 +237,27 @@ static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
> static
> struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
> {
> - struct list_head *p_list;
> - struct iser_device *device = NULL;
> + struct iser_device *device;
>
> mutex_lock(&ig.device_list_mutex);
>
> - p_list = ig.device_list.next;
> - while (p_list != &ig.device_list) {
> - device = list_entry(p_list, struct iser_device, ig_list);
> - /* find if there's a match using the node GUID */
> + list_for_each_entry(device, &ig.device_list, ig_list)
I've just added the original comments that are missing in your patch.
> if (device->ib_device->node_guid == cma_id->device->node_guid)
> - break;
> - }
> -
> - if (device == NULL) {
> - device = kzalloc(sizeof *device, GFP_KERNEL);
> - if (device == NULL)
> goto out;
> - /* assign this device to the device */
> - device->ib_device = cma_id->device;
> - /* init the device and link it into ig device list */
> - if (iser_create_device_ib_res(device)) {
> - kfree(device);
> - device = NULL;
> - goto out;
> - }
> - list_add(&device->ig_list, &ig.device_list);
> +
> + device = kzalloc(sizeof *device, GFP_KERNEL);
> + if (device == NULL)
> + goto out;
> +
> + device->ib_device = cma_id->device;
> + /* init the device and link it into ig device list */
> + if (iser_create_device_ib_res(device)) {
> + kfree(device);
> + device = NULL;
> + goto out;
> }
> + list_add(&device->ig_list, &ig.device_list);
> +
> out:
> BUG_ON(device == NULL);
> device->refcount++;
The iteration through the list of "iser_device"s during device
lookup/creation is broken - it might result in an infinite loop if more
than 1 HCA is used with iSER. Use list_for_each_entry() instead of the
custom, flawed list iteration code.
Signed-off-by: Arne Redlich <[email protected]>
Signed-off-by: Erez Zilber <[email protected]>
---
drivers/infiniband/ulp/iser/iser_verbs.c | 36 +++++++++++++----------------
1 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 714b8db..768ba69 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -237,33 +237,29 @@ static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
static
struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
{
- struct list_head *p_list;
- struct iser_device *device = NULL;
+ struct iser_device *device;
mutex_lock(&ig.device_list_mutex);
- p_list = ig.device_list.next;
- while (p_list != &ig.device_list) {
- device = list_entry(p_list, struct iser_device, ig_list);
+ list_for_each_entry(device, &ig.device_list, ig_list)
/* find if there's a match using the node GUID */
if (device->ib_device->node_guid == cma_id->device->node_guid)
- break;
- }
-
- if (device == NULL) {
- device = kzalloc(sizeof *device, GFP_KERNEL);
- if (device == NULL)
goto out;
- /* assign this device to the device */
- device->ib_device = cma_id->device;
- /* init the device and link it into ig device list */
- if (iser_create_device_ib_res(device)) {
- kfree(device);
- device = NULL;
- goto out;
- }
- list_add(&device->ig_list, &ig.device_list);
+
+ device = kzalloc(sizeof *device, GFP_KERNEL);
+ if (device == NULL)
+ goto out;
+
+ /* assign this device to the device */
+ device->ib_device = cma_id->device;
+ /* init the device and link it into ig device list */
+ if (iser_create_device_ib_res(device)) {
+ kfree(device);
+ device = NULL;
+ goto out;
}
+ list_add(&device->ig_list, &ig.device_list);
+
out:
BUG_ON(device == NULL);
device->refcount++;
--
1.5.3.6
I agree with your patch. It seems that we forgot to add something like p_list=p_list->next. Anyway, using list_for_each_entry is better than what we had.
Erez Zilber <[email protected]> writes:
> Arne Redlich wrote:
>> The iteration through the list of "iser_device"s during device
>> lookup/creation is broken - it might result in an infinite loop if more
>> than 1 HCA is used with iSER. Use list_for_each_entry() instead of the
>> custom, flawed list iteration code.
>>
>> Signed-off-by: Arne Redlich <[email protected]>
>> ---
>> drivers/infiniband/ulp/iser/iser_verbs.c | 36 ++++++++++++-----------------
>> 1 files changed, 15 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
>> index 714b8db..1c0f968 100644
>> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>> @@ -237,33 +237,27 @@ static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
>> static
>> struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
>> {
>> - struct list_head *p_list;
>> - struct iser_device *device = NULL;
>> + struct iser_device *device;
>>
>> mutex_lock(&ig.device_list_mutex);
>>
>> - p_list = ig.device_list.next;
>> - while (p_list != &ig.device_list) {
>> - device = list_entry(p_list, struct iser_device, ig_list);
>> - /* find if there's a match using the node GUID */
>> + list_for_each_entry(device, &ig.device_list, ig_list)
>
> I've just added the original comments that are missing in your patch.
Ah well, I probably should've mentioned in the patch description that I
intentionally removed those comments as I think they're really
redundant, stating the obvious. But of course I won't insist. :)
Thanks,
Arne
Arne Redlich wrote:
> Erez Zilber <[email protected]> writes:
>
>
>> Arne Redlich wrote:
>>
>>> The iteration through the list of "iser_device"s during device
>>> lookup/creation is broken - it might result in an infinite loop if more
>>> than 1 HCA is used with iSER. Use list_for_each_entry() instead of the
>>> custom, flawed list iteration code.
>>>
>>> Signed-off-by: Arne Redlich <[email protected]>
>>> ---
>>> drivers/infiniband/ulp/iser/iser_verbs.c | 36 ++++++++++++-----------------
>>> 1 files changed, 15 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
>>> index 714b8db..1c0f968 100644
>>> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
>>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>>> @@ -237,33 +237,27 @@ static int iser_free_ib_conn_res(struct iser_conn *ib_conn)
>>> static
>>> struct iser_device *iser_device_find_by_ib_device(struct rdma_cm_id *cma_id)
>>> {
>>> - struct list_head *p_list;
>>> - struct iser_device *device = NULL;
>>> + struct iser_device *device;
>>>
>>> mutex_lock(&ig.device_list_mutex);
>>>
>>> - p_list = ig.device_list.next;
>>> - while (p_list != &ig.device_list) {
>>> - device = list_entry(p_list, struct iser_device, ig_list);
>>> - /* find if there's a match using the node GUID */
>>> + list_for_each_entry(device, &ig.device_list, ig_list)
>>>
>> I've just added the original comments that are missing in your patch.
>>
>
> Ah well, I probably should've mentioned in the patch description that I
> intentionally removed those comments as I think they're really
> redundant, stating the obvious. But of course I won't insist. :)
>
> Thanks,
> Arne
>
OK and thanks for catching the bugs.
Roland - will you be able to merge these 2 patches for 2.6.25? Both are
bug fixes.
Thanks,
Erez