small patches to librdmacm

* small patches to librdmacm
@ 2017-08-22 19:47 Jeff Inman
       [not found] ` <C50D9A8A-457A-4DA6-B727-43B77117ED15-YOWKrPYUwWM@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Jeff Inman @ 2017-08-22 19:47 UTC (permalink / raw)
  To: linux-rdma-u79uwXL29TY76Z2rM5mHXA

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

Hi folks,

I’m new to this.  Please let me know, if you need a different format, etc.

Of the two of these, 0001 is the one I care about.


0001:  The patch comment explains the details of the fix, but here’s some context:

Access to entries in the rsocket index 'idm' is effectively serialized by creation/destruction of file-descriptors for /dev/infiniband/rdma_cm.  New fds are created in rsocket -> rdma_create_id, and released in rclose -> rs_free -> rdma_destroy_id.  rs_free() was releasing the index with rs_remove() *after* calling rdma_destroy_id().  But closing the file-descriptor in rdma_destroy_id() yields to the scheduler, making it easy for another thread to get into rsocket() during the break.  This thread can get its own fd (because the first thread’s close has succeed) and store its own rs into the index, before the first thread has gotten around to calling rs_remove().  After the first thread does call rs_remove(), the second thread will segfault in most rsocket-related functions.

Note that I’m not assuming an rsocket can be simultaneously operated by different threads.  I just have separate threads that open/operate/close their own rsockets.  This seems to be working fine, except for this patched problem.  Am I mistaken that this is reasonable?


0002:  Removing the potential for segfaults when a legitimate rs is not found in the index.  This should never happen unless there is a bug like the one fixed in 0001 (or someone stupidly makes a call with a closed rsocket …), so the tests should almost always be wasted.  Made this a separate patch, so you could ignore it, if you want.


Thanks,
Jeff

--
Jeff Inman
High-Performance Computing (HPC-ENV)
MS-B295, Los Alamos National Laboratory
Los Alamos, NM 87545







Jeff Inman (2):
  rsocket: fix a race-condition in rs_free()
  rsocket: fix segfaults when rs is not found in the index

 librdmacm/rsocket.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)



[-- Attachment #2: 0001-rsocket-fix-a-race-condition-in-rs_free.patch --]
[-- Type: application/octet-stream, Size: 1695 bytes --]

From 73d3e0cf6f4816981ef7e5fc0a113888ad9b1f25 Mon Sep 17 00:00:00 2001
From: Jeff Inman <jti-YOWKrPYUwWM@public.gmane.org>
Date: Thu, 17 Aug 2017 13:14:57 -0600
Subject: [rsocket 1/2] rsocket: fix a race-condition in rs_free()

If rs_free() releases the fd before calling rs_remove(), a second
thread in rsocket() may acquire the same fd and store its own rs in
the corresponding index-element.  When the first thread then gets
around to calling rs_remove() it ends up removing the rs of the second
thread, and storing a NULL there.

Several functions still do not check for NULL after retrieving an rs
from the index for an open rsocket.  Thus, the second thread would get
a segfault in any of the following functions: rrecv, rrecvfrom, rsend,
rsendto, rsendv, riomap, riounmap, riowrite.

Fixes:  cf7aae3 "rsocket: Index map item is cleaned before it is used in iomapping cleanup"

Signed-off-by: Jeff Inman <jti-YOWKrPYUwWM@public.gmane.org>
---
 librdmacm/rsocket.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/librdmacm/rsocket.c b/librdmacm/rsocket.c
index f28529c..77a6df4 100644
--- a/librdmacm/rsocket.c
+++ b/librdmacm/rsocket.c
@@ -1012,6 +1012,9 @@ static void rs_free(struct rsocket *rs)
 		free(rs->target_buffer_list);
 	}
 
+	if (rs->index >= 0)
+		rs_remove(rs);
+
 	if (rs->cm_id) {
 		rs_free_iomappings(rs);
 		if (rs->cm_id->qp) {
@@ -1021,9 +1024,6 @@ static void rs_free(struct rsocket *rs)
 		rdma_destroy_id(rs->cm_id);
 	}
 
-	if (rs->index >= 0)
-		rs_remove(rs);
-
 	fastlock_destroy(&rs->map_lock);
 	fastlock_destroy(&rs->cq_wait_lock);
 	fastlock_destroy(&rs->cq_lock);
-- 
1.7.1


[-- Attachment #3: 0002-rsocket-fix-segfaults-when-rs-is-not-found-in-the-in.patch --]
[-- Type: application/octet-stream, Size: 2943 bytes --]

From 89f14b35b2fcf9b20b485be9d6c8280133c2dec9 Mon Sep 17 00:00:00 2001
From: Jeff Inman <jti-YOWKrPYUwWM@public.gmane.org>
Date: Tue, 22 Aug 2017 13:01:31 -0600
Subject: [rsocket 2/2] rsocket: fix segfaults when rs is not found in the index

Fixes: 0b6aff4 librdmacm: Define streaming over RDMA interface (rsockets)
Fixes: bb9fcba rsocket: Add APIs for direct data placement

Signed-off-by: Jeff Inman <jti-YOWKrPYUwWM@public.gmane.org>
---
 librdmacm/rsocket.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/librdmacm/rsocket.c b/librdmacm/rsocket.c
index 77a6df4..ca0b9bf 100644
--- a/librdmacm/rsocket.c
+++ b/librdmacm/rsocket.c
@@ -2420,6 +2420,8 @@ ssize_t rrecv(int socket, void *buf, size_t len, int flags)
 	int ret = 0;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (rs->type == SOCK_DGRAM) {
 		fastlock_acquire(&rs->rlock);
 		ret = ds_recvfrom(rs, buf, len, flags, NULL, NULL);
@@ -2488,6 +2490,8 @@ ssize_t rrecvfrom(int socket, void *buf, size_t len, int flags,
 	int ret;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (rs->type == SOCK_DGRAM) {
 		fastlock_acquire(&rs->rlock);
 		ret = ds_recvfrom(rs, buf, len, flags, src_addr, addrlen);
@@ -2691,6 +2695,8 @@ ssize_t rsend(int socket, const void *buf, size_t len, int flags)
 	int ret = 0;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (rs->type == SOCK_DGRAM) {
 		fastlock_acquire(&rs->slock);
 		ret = dsend(rs, buf, len, flags);
@@ -2776,6 +2782,8 @@ ssize_t rsendto(int socket, const void *buf, size_t len, int flags,
 	int ret;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (rs->type == SOCK_STREAM) {
 		if (dest_addr || addrlen)
 			return ERR(EISCONN);
@@ -2831,6 +2839,8 @@ static ssize_t rsendv(int socket, const struct iovec *iov, int iovcnt, int flags
 	int i, ret = 0;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (rs->state & rs_opening) {
 		ret = rs_do_connect(rs);
 		if (ret) {
@@ -3773,6 +3783,8 @@ off_t riomap(int socket, void *buf, size_t len, int prot, int flags, off_t offse
 	int access = IBV_ACCESS_LOCAL_WRITE;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	if (!rs->cm_id->pd || (prot & ~(PROT_WRITE | PROT_NONE)))
 		return ERR(EINVAL);
 
@@ -3821,6 +3833,8 @@ int riounmap(int socket, void *buf, size_t len)
 	int ret = 0;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	fastlock_acquire(&rs->map_lock);
 
 	for (entry = rs->iomap_list.next; entry != &rs->iomap_list;
@@ -3868,6 +3882,8 @@ size_t riowrite(int socket, const void *buf, size_t count, off_t offset, int fla
 	int ret = 0;
 
 	rs = idm_at(&idm, socket);
+	if (!rs)
+		return ERR(EBADF);
 	fastlock_acquire(&rs->slock);
 	if (rs->iomap_pending) {
 		ret = rs_send_iomaps(rs, flags);
-- 
1.7.1


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



^ permalink raw reply related	[flat|nested] 7+ messages in thread