[PATCH] Fixing segfault in libibverbs

[PATCH] Fixing segfault in libibverbs

[Animesh K Trivedi1]

Hi,

This patch fixes SEGFAULT in libibverbs in case when there are no user
space drivers found. I've cloned the libibverbs from
git://git.openfabrics.org/ofed_1_1_5/libibverbs.git (I hope this is
correct)

Example: rping gets segmentation fault in libibverbs when there are no user
space drivers found. Here is the output
$ rping -s
libibverbs: Warning: no userspace device-specific driver found
for /sys/class/infiniband_verbs/uverbs0
Segmentation fault

rping[28471]: segfault at 3 ip 00007f20c5a51ca6 sp 00007fffddc22b80 error 4
in libibverbs.so.1.0.0[7f20c5a4b000+c000]

This is caused by corrupt pointers, driver_name_list and sysfs_dev_list.
They are used in maintaining the head of the link list but are not
re-initialized to NULL in between the invocations. The previous stale
address values are used in read_config() and find_sysfs_dev() calls.
Although I am not sure if calls to rdma_create_event_channel() and
rdma_create_id() (in rping) should succeed, if there are no user space
drivers. That is why there are multiple invocations to ibverbs_init().

Thanks,
--
Animesh


Signed-off-by: Animesh Trivedi <atr-OA+xvbQnYDHMbYB6QlFGEg@public.gmane.org>

diff --git a/src/init.c b/src/init.c
index 4f0130e..45c360f 100644
--- a/src/init.c
+++ b/src/init.c
@@ -232,6 +232,7 @@ static void load_drivers(void)
 		free(name->name);
 		free(name);
 	}
+	driver_name_list=NULL;
 }

 static void read_config_file(const char *path)
@@ -536,6 +537,7 @@ out:
 		}
 		free(sysfs_dev);
 	}
+	sysfs_dev_list=NULL;

 	return num_devices;
 }

--
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: [PATCH] Fixing segfault in libibverbs

[Yann Droneaud]

Hi,

Le vendredi 27 mai 2011 à 11:58 +0200, Animesh K Trivedi1 a écrit :
> Hi,
> 
> This patch fixes SEGFAULT in libibverbs in case when there are no user
> space drivers found. I've cloned the libibverbs from
> git://git.openfabrics.org/ofed_1_1_5/libibverbs.git (I hope this is
> correct)
> 

I sent some patches a few month ago to address the same problems (and
others).

See thread "Hardenize libibverbs initialisation"
Message-Id: <cover.1301562707.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Link: http://thread.gmane.org/gmane.linux.drivers.rdma/7790

Regards

-- 
Yann Droneaud
OPTEYA


--
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: [PATCH] Fixing segfault in libibverbs

[Roland Dreier]

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

>> This patch fixes SEGFAULT in libibverbs in case when there are no user
>> space drivers found. I've cloned the libibverbs from
>> git://git.openfabrics.org/ofed_1_1_5/libibverbs.git (I hope this is
>> correct)

Thanks for the patches.  I think I would prefer to fix this more like the
following (sorry for the attachment, using the gmail web interface ATM).
This actually simplifies things I think, and sticks to the original idea
that we should only be calling ibverbs_init once.

I only compile tested this, can one of you guys confirm this also fixes
the problem?

Thanks!
  Roland

[-- Attachment #2: ibverbs-multiple-init.diff --]
[-- Type: text/x-diff, Size: 1320 bytes --]

 src/device.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/device.c b/src/device.c
index 185f4a6..5798895 100644
--- a/src/device.c
+++ b/src/device.c
@@ -49,32 +49,34 @@
 
 #include "ibverbs.h"
 
-static pthread_mutex_t device_list_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_once_t device_list_once = PTHREAD_ONCE_INIT;
 static int num_devices;
 static struct ibv_device **device_list;
 
+static void count_devices(void)
+{
+	num_devices = ibverbs_init(&device_list);
+}
+
 struct ibv_device **__ibv_get_device_list(int *num)
 {
-	struct ibv_device **l = 0;
+	struct ibv_device **l;
 	int i;
 
 	if (num)
 		*num = 0;
 
-	pthread_mutex_lock(&device_list_lock);
-
-	if (!num_devices)
-		num_devices = ibverbs_init(&device_list);
+	pthread_once(&device_list_once, count_devices);
 
 	if (num_devices < 0) {
 		errno = -num_devices;
-		goto out;
+		return NULL;
 	}
 
 	l = calloc(num_devices + 1, sizeof (struct ibv_device *));
 	if (!l) {
 		errno = ENOMEM;
-		goto out;
+		return NULL;
 	}
 
 	for (i = 0; i < num_devices; ++i)
@@ -82,8 +84,6 @@ struct ibv_device **__ibv_get_device_list(int *num)
 	if (num)
 		*num = num_devices;
 
-out:
-	pthread_mutex_unlock(&device_list_lock);
 	return l;
 }
 default_symver(__ibv_get_device_list, ibv_get_device_list);

Re: [PATCH] Fixing segfault in libibverbs

[Yann Droneaud]

Hi, 

Le vendredi 27 mai 2011 à 09:22 -0700, Roland Dreier a écrit :
> >> This patch fixes SEGFAULT in libibverbs in case when there are no user
> >> space drivers found. I've cloned the libibverbs from
> >> git://git.openfabrics.org/ofed_1_1_5/libibverbs.git (I hope this is
> >> correct)
> 
> Thanks for the patches.  I think I would prefer to fix this more like the
> following (sorry for the attachment, using the gmail web interface ATM).
> This actually simplifies things I think, and sticks to the original idea
> that we should only be calling ibverbs_init once.
> 
> I only compile tested this, can one of you guys confirm this also fixes
> the problem?
> 

It fixes the problem of segfault on empty driver list.

But __ibv_get_device_list() returns a non NULL pointer when there's no
device.

> -       if (!num_devices)
> -               num_devices = ibverbs_init(&device_list);
> +       pthread_once(&device_list_once, count_devices);
 
It miss

        if (!num_devices)
               return NULL;

>        if (num_devices < 0) {
>                errno = -num_devices;
>-               goto out;
>+               return NULL;
>        }

Tested-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

BTW, my others patches are still interesting.

Regards.

-- 
Yann Droneaud
OPTEYA



--
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: [PATCH] Fixing segfault in libibverbs

[Yann Droneaud]

Hi,

Le vendredi 27 mai 2011 à 19:06 +0200, Yann Droneaud a écrit :

> But __ibv_get_device_list() returns a non NULL pointer when there's no
> device.
> 

Forget about my comment, I haven't checked the manpage for the function:

ibv_get_device_list(3) :

RETURN VALUE

ibv_get_device_list()  returns  the array of available RDMA devices, or
sets errno and returns NULL if the request fails.  If  no  devices  are
found then num_devices is set to 0, and non-NULL is returned.

I'm sending a patch against rdmacm to address the problem.

Regards.

-- 
Yann Droneaud
OPTEYA


--
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

[PATCH:rdmacm] ucma_init: fail if ibv_get_device_list() returns empty list

[Yann Droneaud]

As stated in ibv_get_device_list() man page:

  ibv_get_device_list() returns the array of available RDMA devices, or
  sets errno and returns NULL if the request fails. If no devices are
  found then num_devices is set to 0, and non-NULL is returned.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 src/cma.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/cma.c b/src/cma.c
index 846b5ce..c8d9b9f 100755
--- a/src/cma.c
+++ b/src/cma.c
@@ -246,6 +246,7 @@ int ucma_init(void)
 	if (ret)
 		goto err1;
 
+	dev_cnt = 0;
 	dev_list = ibv_get_device_list(&dev_cnt);
 	if (!dev_list) {
 		printf("CMA: unable to get RDMA device list\n");
@@ -253,6 +254,12 @@ int ucma_init(void)
 		goto err1;
 	}
 
+	if (!dev_cnt) {
+		printf("CMA: unable to get RDMA device list\n");
+		ret = ERR(ENODEV);
+		goto err2;
+	}
+
 	cma_dev_array = malloc(sizeof *cma_dev * dev_cnt);
 	if (!cma_dev_array) {
 		ret = ERR(ENOMEM);
-- 
1.7.5.2

--
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: [PATCH:rdmacm] ucma_init: fail if ibv_get_device_list() returns empty list

[Hefty, Sean]

>   ibv_get_device_list() returns the array of available RDMA devices, or
>   sets errno and returns NULL if the request fails. If no devices are
>   found then num_devices is set to 0, and non-NULL is returned.

Thanks.  I think the following fixes the same issue.  If this looks okay to you, I'll will commit.

Signed-off-by: Sean Hefty <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

---

diff --git a/src/cma.c b/src/cma.c
index 846b5ce..86cb455 100755
--- a/src/cma.c
+++ b/src/cma.c
@@ -247,7 +247,7 @@ int ucma_init(void)
                goto err1;

        dev_list = ibv_get_device_list(&dev_cnt);
-       if (!dev_list) {
+       if (!dev_cnt) {
                printf("CMA: unable to get RDMA device list\n");
                ret = ERR(ENODEV);
                goto err1;
--
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: [PATCH] Fixing segfault in libibverbs

[Roland Dreier]

On Fri, May 27, 2011 at 10:06 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Tested-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Thanks, committed and pushed out.

> BTW, my others patches are still interesting.

Yes, I need to look at them.  Sorry for being so slow.

 - R.
--
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: [PATCH:rdmacm] ucma_init: fail if ibv_get_device_list() returns empty list

[Yann Droneaud]

Hi, 

Le vendredi 27 mai 2011 à 18:01 +0000, Hefty, Sean a écrit :
> >   ibv_get_device_list() returns the array of available RDMA devices, or
> >   sets errno and returns NULL if the request fails. If no devices are
> >   found then num_devices is set to 0, and non-NULL is returned.
> 
> Thanks.  I think the following fixes the same issue.  If this looks okay to you, I'll will commit.
> 

You're not handling the error case when ibv_get_device_list() returns
NULL.
We need two different error path: one for NULL and one for empty list.
In the latter case, the list must be freed.

Regards.
-- 
Yann Droneaud
OPTEYA


--
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: [PATCH:rdmacm] ucma_init: fail if ibv_get_device_list() returns empty list

[Hefty, Sean]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 394 bytes --]

> You're not handling the error case when ibv_get_device_list() returns
> NULL.
> We need two different error path: one for NULL and one for empty list.
> In the latter case, the list must be freed.

Yep - you're right.  I'll pull in your patch.  Thanks.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš