[PATCH 0/3] Ibacm/ibacmp bug fixes

[PATCH 0/3] Ibacm/ibacmp bug fixes

[kaike.wan-ral2JQCrhuEAvxtiuMwx3w]

From: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch series fixes a print format error and two bugs related to SM restart.


Kaike Wan (3):
  ibacmp: missing '%' in acm_log format
  ibacm: close the provider endpoint when it fails to assign a name to
    a core endpoint
  ibacm/ibacmp: fix a crash when SM restarts

 prov/acmp/src/acmp.c |    6 +++++-
 src/acm.c            |    9 ++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

--
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 1/3] ibacmp: missing '%' in acm_log format

[kaike.wan-ral2JQCrhuEAvxtiuMwx3w]

From: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This debugging print is in function acmp_get_ep().

Signed-off-by: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 prov/acmp/src/acmp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/prov/acmp/src/acmp.c b/prov/acmp/src/acmp.c
index 2dd356d..7568b9c 100644
--- a/prov/acmp/src/acmp.c
+++ b/prov/acmp/src/acmp.c
@@ -2374,7 +2374,7 @@ acmp_get_ep(struct acmp_port *port, struct acm_endpoint *endpoint)
 	struct acmp_ep *ep;
 	DLIST_ENTRY *entry;
 
-	acm_log(1, "dev 0xllx port %d pkey 0x%x\n",
+	acm_log(1, "dev 0x%llx port %d pkey 0x%x\n",
 		endpoint->port->dev->dev_guid, endpoint->port->port_num, endpoint->pkey);
 	for (entry = port->ep_list.Next; entry != &port->ep_list;
 	     entry = entry->Next) {
-- 
1.7.1

--
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 2/3] ibacm: close the provider endpoint when it fails to assign a name to a core endpoint

[kaike.wan-ral2JQCrhuEAvxtiuMwx3w]

From: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

In function acm_ep_up(), when it fails to assign any name to an endpoint, the
endpoint in the provider is not properly closed before the core endpoint is
freed. This may cause segfault when ibacmp tries to join multicast group with 
a stale core endpoint pointer.

Signed-off-by: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 src/acm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/src/acm.c b/src/acm.c
index 11fda4c..d807c73 100644
--- a/src/acm.c
+++ b/src/acm.c
@@ -1744,12 +1744,15 @@ static void acm_ep_up(struct acmc_port *port, uint16_t pkey)
 	ret = acm_assign_ep_names(ep);
 	if (ret) {
 		acm_log(0, "ERROR - unable to assign EP name for pkey 0x%x\n", pkey);
-		goto err;
+		goto ep_close;
 	}
 
 	DListInsertHead(&ep->entry, &port->ep_list);
 	return;
 
+ep_close:
+	port->prov->close_endpoint(ep->prov_ep_context);
+
 err:
 	free(ep);
 }
-- 
1.7.1

--
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 3/3] ibacm/ibacmp: fix a crash when SM restarts

[kaike.wan-ral2JQCrhuEAvxtiuMwx3w]

From: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Ibacm may cause segfault when the SM restarts: when the SM restarts, ibacm will
receive P_Key change event and instruct ibacmp to close all endpoints. However,
ibacmp only resets the core endpoint pointer in its ep structure and keeps the ep
in the port's ep_list. Afterwards, the ibacm core will ask ibacmp to create
an ep for each pkey enumerated from the local port. The ep will be found
from the port's ep_list if it exists. However, if an old pkey is not present
in the new SM configuration, the old ep will still be linked in the port's
ep_list with the ep->endpoint being set to NULL. When the ibacm core forwards
the client reregistration event to ibacmp, ibacmp will enumerate the ep_list and
try to join multicast group for each ep, including any one with ep->endpoint
set to NULL. In this case, it will cause segfault in acm_send_sa_mad().
Additional check should be able to avoid the crash.

Signed-off-by: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 prov/acmp/src/acmp.c |    4 ++++
 src/acm.c            |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/prov/acmp/src/acmp.c b/prov/acmp/src/acmp.c
index 7568b9c..2b85958 100644
--- a/prov/acmp/src/acmp.c
+++ b/prov/acmp/src/acmp.c
@@ -1446,6 +1446,10 @@ static int acmp_port_join(void *port_context)
 	for (ep_entry = port->ep_list.Next; ep_entry != &port->ep_list;
 		 ep_entry = ep_entry->Next) {
 		ep = container_of(ep_entry, struct acmp_ep, entry);
+		if (!ep->endpoint) {
+			/* Stale endpoint */
+			continue;
+		}
 		acmp_ep_join(ep);
 	}
 	acm_log(1, "joins for device %s port %d complete\n",
diff --git a/src/acm.c b/src/acm.c
index d807c73..2d0d2e1 100644
--- a/src/acm.c
+++ b/src/acm.c
@@ -2352,6 +2352,10 @@ acm_alloc_sa_mad(const struct acm_endpoint *endpoint, void *context,
 {
 	struct acmc_sa_req *req;
 
+	if (!endpoint) {
+		acm_log(0, "Error: NULL endpoint\n");
+		return NULL;
+	}
 	req = calloc(1, sizeof (*req));
 	if (!req) {
 		acm_log(0, "Error: failed to allocate sa request\n");
-- 
1.7.1

--
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 3/3] ibacm/ibacmp: fix a crash when SM restarts

[Hefty, Sean]

> Ibacm may cause segfault when the SM restarts: when the SM restarts, ibacm
> will
> receive P_Key change event and instruct ibacmp to close all endpoints.
> However,
> ibacmp only resets the core endpoint pointer in its ep structure and keeps
> the ep
> in the port's ep_list. Afterwards, the ibacm core will ask ibacmp to
> create
> an ep for each pkey enumerated from the local port. The ep will be found
> from the port's ep_list if it exists. However, if an old pkey is not
> present
> in the new SM configuration, the old ep will still be linked in the port's
> ep_list with the ep->endpoint being set to NULL. When the ibacm core
> forwards
> the client reregistration event to ibacmp, ibacmp will enumerate the
> ep_list and
> try to join multicast group for each ep, including any one with ep-
> >endpoint
> set to NULL. In this case, it will cause segfault in acm_send_sa_mad().
> Additional check should be able to avoid the crash.

I'm having trouble following this.  Is the problem that the provider is not cleaning up properly/completely when being told to close its endpoint?


--
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 3/3] ibacm/ibacmp: fix a crash when SM restarts

[Wan, Kaike]

> > Ibacm may cause segfault when the SM restarts: when the SM restarts,
> > ibacm will receive P_Key change event and instruct ibacmp to close all
> > endpoints.
> > However,
> > ibacmp only resets the core endpoint pointer in its ep structure and
> > keeps the ep in the port's ep_list. Afterwards, the ibacm core will
> > ask ibacmp to create an ep for each pkey enumerated from the local
> > port. The ep will be found from the port's ep_list if it exists.
> > However, if an old pkey is not present in the new SM configuration,
> > the old ep will still be linked in the port's ep_list with the
> > ep->endpoint being set to NULL. When the ibacm core forwards the
> > client reregistration event to ibacmp, ibacmp will enumerate the
> > ep_list and try to join multicast group for each ep, including any one
> > with ep-
> > >endpoint
> > set to NULL. In this case, it will cause segfault in acm_send_sa_mad().
> > Additional check should be able to avoid the crash.
> 
> I'm having trouble following this.  Is the problem that the provider is not
> cleaning up properly/completely when being told to close its endpoint?
> 
[Wan, Kaike] Yes, it's a problem for the current implementation of the default provider ibacmp. If the provider can clean up properly, it will not have such a problem. Nevertheless, the ibacm core should validate incoming parameters for a SA request from any provider.

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