rxm: closed msg_ep are still referenced by rx_buf->rx_ep

[jreniecki]

Hello,
I am quite new to libfabric and not yet very familiar with its architecture, so please forgive me if I use some terms incorrectly.

There used to be code in libfaric that apparently took care of "buffers binded to closed msg ep" (see the patch here)
I am not sure if the current libfabric code has any equivalent of that patch, but I am observing some very similar issue. I am attaching a relevant log snippet. Here's what I am observing:
msg_ep_issue.txt

Here's a sequence of events that I noticed in that log:

1. rx_buf=0x7f979136ac20 is first posted with rx_buf->rx_ep=0x517000151200 and when that msg_ep was opened, the rx_buf was reposted with rx_buf->rx_ep=0x517000151200 and rx_buf->conn->msg_ep=0x517000151200
2. msg_ep=0x517000151200 was closed
3. msg_ep=0x51700019a300 was opened
4. our rx_buf=0x7f979136ac20 was reposted but now rx_buf->conn->msg_ep=0x51700019a300 (which is the new msg_ep) but rx_buf->rx_ep=0x517000151200 (which is the old/closed msg_ep).
5. because rx_buf->rx_ep->msg=NULL, I observe segmentation fault (null pointer).

So, as quick check I did this modification in rxm_replace_rx_buf():

 static void rxm_replace_rx_buf(struct rxm_rx_buf *rx_buf)
 {
        struct rxm_rx_buf *new_rx_buf;
+       struct fid_ep *rx_ep;
        int ret;
 
-       new_rx_buf = rxm_rx_buf_alloc(rx_buf->ep, rx_buf->rx_ep);
+       if (!rx_buf->ep->msg_srx && rx_buf->conn) {
+               if (!rx_buf->conn->msg_ep)
+                       return;  // closed conn, do not repost
+
+               rx_ep = rx_buf->conn->msg_ep;
+       } else {
+               rx_ep = rx_buf->rx_ep;
+       }
+
+       new_rx_buf = rxm_rx_buf_alloc(rx_buf->ep, rx_ep);
        if (!new_rx_buf)
                return;
 	rx_buf->repost = false;
	ret = rxm_post_recv(new_rx_buf);
	if (ret)
		ofi_buf_free(new_rx_buf);
}

And it does work for my case, it seems to fix my issue, but I am not sure if it's entirely valid - especially if there's something else needed to be done for SRX.

Thank you in advance for any comments/suggestions.

[aingerson]

Thanks for opening an issue @jreniecki !
I don't see anything inherently wrong with your fix. I am curious why the msg_ep is being closed while there are still pending messages to be processed on it. replace_rx_buf is only called on the message processing path so that means you're in the middle of processing a message and are reposting the buffer so if the connection changes in the middle of a sar or rendezvous message, I'm not sure what the behavior would be...

[jreniecki]

Thank you @aingerson for your quick reply. Yes, I wasn't sure if it's correct behaviour that the msg_ep is being closed when there are pending messages... so I assumed that RXM is allowed to do that for some reason... (I don't know, limited resources?)
Currently, I don't see in my logs any hints why this happens. Do you think I should try to find the reason why the msg_ep is being closed in the first place?

[aingerson]

It's probably coming from the app closing the RxM RDM endpoint. As long as that's happening after all send completions are being read to make sure all pending messages have been sent, it should be ok. I'd be curious what the last message being sent is. If you can share that or a reproducer app, I'd be interested in taking a look at the flow that causes this but I don't think you need to spend too much time digging into it.
Please open a PR with your change upstream and we will get it merged in!

[jreniecki]

Sure, I will prepare a PR with my change.
My reproduction environment is a bit complex at the moment, so it might be actually less time-consuming for me if I try to dig deeper myself rather than prepare a simpler reproducer. I will share any information I find.
Thank you for your support!

[jreniecki]

Hi @aingerson
So I looked at the origin of the rxm_close_conn() call, and it starts in rxm_conn_progress()

void rxm_conn_progress(struct rxm_ep *ep)
{
	struct rxm_eq_cm_entry cm_entry;
	uint32_t event;
	ssize_t ret;

	assert(ofi_genlock_held(&ep->util_ep.lock));
	do {
		ret = fi_eq_read(ep->msg_eq, &event, &cm_entry,
				 sizeof(cm_entry), 0);
		if (ret > 0) {
			rxm_handle_event(ep, event, &cm_entry, ret);
		} else if (ret == -FI_EAVAIL) {
			rxm_handle_error(ep);
			ret = 1;
		}
	} while (ret > 0);
}

fi_eq_read() returns ret=16 and event=3 (FI_SHUTDOWN) which is translated in rxm_handle_event() to call to rxm_process_shutdown() which calls rxm_close_conn().

So it seems that the source of "FI_SHUTDOWN" is the verbs provider that implements the msg endpoint functionality, but I don't know yet what went wrong down there.

[aingerson]

Thanks for looking into it! Can you tell if the remote side closed the connection? My guess is this path:
rxm_ep_close -> rxm_freeall_conns ->rxm_close_conn -> fi_close -> vrb_ep_close -> rdma_destroy_ep

[jreniecki]

Hello @aingerson
Regarding the patch and my PR: Unfortunately, I am not convinced that we should go ahead with it. It did seem like a good work-around for handling some problematic cases, but after increasing size of transmitted data, I encountered cases where the "new connection" was not established yet etc. which basically always led to a null-pointer somewhere. So basically, my patch works if we're lucky.

So I decided to look for the reason of closed connections and I found quite what I suspected.
On rxm level I can see these messages:

libfabric:1958558:1774522776::ofi_rxm:cq:rxm_handle_comp_error():1657<warn> fi_cq_readerr: err: Input/output error (5), prov_err: transport retry counter exceeded (12)

And in core verbs provider in vrb_poll_cq() here:

		if (wc->status != IBV_WC_SUCCESS && wc->status != IBV_WC_WR_FLUSH_ERR)
			vrb_shutdown_ep(ctx->ep);
		}

I see wc->status=IBV_WC_RETRY_EXC_ERR with wc->vendor_err=0x81

If I understand correctly, this looks like an infrastructure problem. My team and I use VMs in a private cloud for testing, and we suspect that the whole infrastructure might not be optimal in terms of available resources/bandwidth for each VM. We regularly see timeout issues on "pure" MSG endpoints (no rxm).

Assuming my understanding is correct, it still bothers me that we hit null pointer dereference (rx_buf->rx_ep->msg=NULL) instead of handling this situation gracefully and with some meaningful message.

Do you think that that might be an rxm flaw that should be addressed?

Thanks!