[openib-general] Re: SDP: still getting sk_alloc() panic, any ideas?
Libor Michalek
libor at topspin.com
Tue Jun 28 11:19:13 PDT 2005
On Mon, Jun 27, 2005 at 04:06:11PM -0700, Libor Michalek wrote:
> On Mon, Jun 27, 2005 at 02:27:54PM -0700, Tom Duffy wrote:
> > On Mon, 2005-06-27 at 11:17 -0700, Libor Michalek wrote:
> > > The problem is that each call to sk_alloc() is grabbing a reference to
> > > the module, but it checks to make sure that there already is at least one
> > > reference, if not the top BUG is triggered. In the case of the passive
> > > connection there are no other references to the module. You can see that
> > > the problem goes away if you open just one socket, even if you don't
> > > listen on it, and then try the failing passive connect. When a socket is
> > > created it actually grabs two references to the module, one at the sock
> > > level and one at the sk level. The first reference at the sock level does
> > > not trigger the BUG since it's through another code path. (try_module_get
> > > vs. __module_get) This is why we only hit this during passive connect
> > > to a system that has no active SDP sockets.
> >
> > Hrm. That seems ugly. How about a patch to upstream changing
> > sk_alloc() to use try_module_get().
>
> I'm thinking the listen_lookup needs to be moved earlier in the
> req_handler ahead of the sk_alloc, since it makes no sense to do
> the alloc if we are not going to queue the new incomming connection,
> since it just leads to a destroy in the same function.
Here's a patch to reorder listen_lookup() in req_handler() and a few
other fixups, such as making a couple int return functions void.
-Libor
Index: sdp_proto.h
===================================================================
--- sdp_proto.h (revision 2731)
+++ sdp_proto.h (working copy)
@@ -282,8 +282,8 @@
/*
* port/queue managment
*/
-int sdp_inet_accept_q_put(struct sdp_opt *listen_conn,
- struct sdp_opt *accept_conn);
+void sdp_inet_accept_q_put(struct sdp_opt *listen_conn,
+ struct sdp_opt *accept_conn);
struct sdp_opt *sdp_inet_accept_q_get(struct sdp_opt *listen_conn);
@@ -299,7 +299,7 @@
int sdp_inet_port_put(struct sdp_opt *conn);
-int sdp_inet_port_inherit(struct sdp_opt *parent, struct sdp_opt *child);
+void sdp_inet_port_inherit(struct sdp_opt *parent, struct sdp_opt *child);
/*
* active connect functions
Index: sdp_conn.c
===================================================================
--- sdp_conn.c (revision 2732)
+++ sdp_conn.c (working copy)
@@ -176,16 +176,14 @@
/*
* sdp_inet_accept_q_put - put a conn into a listen conn's accept Q.
*/
-int sdp_inet_accept_q_put(struct sdp_opt *listen_conn,
- struct sdp_opt *accept_conn)
+void sdp_inet_accept_q_put(struct sdp_opt *listen_conn,
+ struct sdp_opt *accept_conn)
{
struct sdp_opt *next_conn;
- if (listen_conn->parent ||
- accept_conn->parent ||
- !listen_conn->accept_next ||
- !listen_conn->accept_prev)
- return -EFAULT;
+ BUG_ON(listen_conn->parent);
+ BUG_ON(accept_conn->parent);
+ BUG_ON(!listen_conn->accept_next || !listen_conn->accept_prev);
next_conn = listen_conn->accept_next;
@@ -200,8 +198,6 @@
* up ref until we release. One ref for GW and one for INET.
*/
sdp_conn_hold(accept_conn); /* AcceptQueue INET reference */
-
- return 0;
}
/*
@@ -545,9 +541,8 @@
/*
* sdp_inet_port_inherit - inherit a port from another socket (accept)
*/
-int sdp_inet_port_inherit(struct sdp_opt *parent, struct sdp_opt *child)
+void sdp_inet_port_inherit(struct sdp_opt *parent, struct sdp_opt *child)
{
- int result;
unsigned long flags;
/*
@@ -555,13 +550,8 @@
*/
spin_lock_irqsave(&dev_root_s.bind_lock, flags);
- if (child->bind_p_next ||
- child->src_port != parent->src_port) {
- sdp_dbg_warn(child, "child already bound. <%d:%d>",
- parent->src_port, child->src_port);
- result = -EADDRNOTAVAIL;
- goto done;
- }
+ BUG_ON(child->bind_p_next);
+ BUG_ON(child->src_port != parent->src_port);
/*
* insert into listening list.
*/
@@ -572,10 +562,7 @@
if (child->bind_next)
child->bind_next->bind_p_next = &child->bind_next;
- result = 0;
-done:
spin_unlock_irqrestore(&dev_root_s.bind_lock, flags);
- return result;
}
/*
Index: sdp_pass.c
===================================================================
--- sdp_pass.c (revision 2732)
+++ sdp_pass.c (working copy)
@@ -229,121 +229,54 @@
return result;
}
-static int sdp_cm_listen_lookup(struct sdp_opt *conn)
+static void sdp_cm_listen_inherit(struct sdp_opt *parent,
+ struct sdp_opt *child)
{
- struct sdp_opt *listen_conn;
- struct sock *listen_sk;
- struct sock *sk;
- int result;
- /*
- * match a listener with an incoming conn, and generate
- * accept message and state on success.
- */
- sdp_dbg_ctrl(conn,
- "listen match for conn. src <%08x:%04x> dst <%08x:%04x>",
- conn->src_addr, conn->src_port,
- conn->dst_addr, conn->dst_port);
- /*
- * first find a listening connection
- */
- listen_conn = sdp_inet_listen_lookup(conn->src_addr, conn->src_port);
- if (!listen_conn) {
- /*
- * no connection, reject
- */
- sdp_dbg_warn(conn, "no listener for connection. <%08x:%04x>",
- conn->src_addr, conn->src_port);
- result = -ECONNREFUSED;
- goto lookup_err;
- }
+ struct sock *psk;
+ struct sock *csk;
- sdp_conn_lock(listen_conn);
- /*
- * check backlog
- */
- listen_sk = sk_sdp(listen_conn);
- sk = sk_sdp(conn);
+ sdp_inet_port_inherit(parent, child);
- if (listen_conn->backlog_cnt > listen_conn->backlog_max) {
- sdp_dbg_warn(listen_conn,
- "Listen backlog <%d> too big to accept new conn",
- listen_conn->backlog_cnt);
- result = -ECONNREFUSED;
- goto locked_err;
- }
-
- result = sdp_inet_port_inherit(listen_conn, conn);
- if (result < 0) {
- sdp_dbg_warn(listen_conn, "Error <%d> listen port inherit.",
- result);
- result = -EFAULT;
- goto locked_err;
- }
+ psk = sk_sdp(parent);
+ csk = sk_sdp(child);
/*
* insert accept socket into listen sockets list.
* TODO: needs to be a FIFO not a LIFO, as is now.
*/
- inet_sk(sk)->num = conn->src_port;
- inet_sk(sk)->sport = htons(conn->src_port);
- inet_sk(sk)->rcv_saddr = htonl(conn->src_addr);
- inet_sk(sk)->saddr = htonl(conn->src_addr);
- inet_sk(sk)->daddr = htonl(conn->dst_addr);
- inet_sk(sk)->dport = htons(conn->dst_port);
+ inet_sk(csk)->num = child->src_port;
+ inet_sk(csk)->sport = htons(child->src_port);
+ inet_sk(csk)->rcv_saddr = htonl(child->src_addr);
+ inet_sk(csk)->saddr = htonl(child->src_addr);
+ inet_sk(csk)->daddr = htonl(child->dst_addr);
+ inet_sk(csk)->dport = htons(child->dst_port);
/*
* relevant options, and others... TCP does a full copy, I'd like to
* know what I'm inheriting.
*/
- sk->sk_lingertime = listen_sk->sk_lingertime;
- sk->sk_rcvlowat = listen_sk->sk_rcvlowat;
- if (sock_flag(listen_sk, SOCK_DBG))
- sock_set_flag(sk, SOCK_DBG);
- if (sock_flag(listen_sk, SOCK_LOCALROUTE))
- sock_set_flag(sk, SOCK_LOCALROUTE);
- sk->sk_sndbuf = listen_sk->sk_sndbuf;
- sk->sk_rcvbuf = listen_sk->sk_rcvbuf;
- sk->sk_no_check = listen_sk->sk_no_check;
- sk->sk_priority = listen_sk->sk_priority;
- if (sock_flag(listen_sk, SOCK_RCVTSTAMP))
- sock_set_flag(sk, SOCK_RCVTSTAMP);
- sk->sk_rcvtimeo = listen_sk->sk_rcvtimeo;
- sk->sk_sndtimeo = listen_sk->sk_sndtimeo;
- sk->sk_reuse = listen_sk->sk_reuse;
- sk->sk_bound_dev_if = listen_sk->sk_bound_dev_if;
- sk->sk_userlocks |= (listen_sk->sk_userlocks & ~SOCK_BINDPORT_LOCK);
- sk->sk_flags = ((SOCK_URGINLINE|SOCK_LINGER|SOCK_BROADCAST) &
- listen_sk->sk_flags);
+ csk->sk_lingertime = psk->sk_lingertime;
+ csk->sk_rcvlowat = psk->sk_rcvlowat;
+ csk->sk_sndbuf = psk->sk_sndbuf;
+ csk->sk_rcvbuf = psk->sk_rcvbuf;
+ csk->sk_no_check = psk->sk_no_check;
+ csk->sk_priority = psk->sk_priority;
+ csk->sk_rcvtimeo = psk->sk_rcvtimeo;
+ csk->sk_sndtimeo = psk->sk_sndtimeo;
+ csk->sk_reuse = psk->sk_reuse;
+ csk->sk_bound_dev_if = psk->sk_bound_dev_if;
+ csk->sk_userlocks |= (psk->sk_userlocks & ~SOCK_BINDPORT_LOCK);
+ csk->sk_flags = ((SOCK_URGINLINE|SOCK_LINGER|SOCK_BROADCAST) &
+ psk->sk_flags);
- conn->src_zthresh = listen_conn->src_zthresh;
- conn->snk_zthresh = listen_conn->snk_zthresh;
- conn->nodelay = listen_conn->nodelay;
- /*
- * initiate a CM response message.
- */
- result = sdp_cm_accept(conn);
- if (result < 0) {
- sdp_dbg_warn(conn, "Error <%d> CM connect accept", result);
- goto locked_err;
- }
- /*
- * place connection into the listen connections accept queue.
- */
- result = sdp_inet_accept_q_put(listen_conn, conn);
- if (result < 0) {
- sdp_dbg_warn(conn,
- "Error <%d> adding socket to accept queue",
- result);
- result = -EFAULT;
- goto locked_err;
- }
+ if (sock_flag(psk, SOCK_DBG))
+ sock_set_flag(csk, SOCK_DBG);
+ if (sock_flag(psk, SOCK_LOCALROUTE))
+ sock_set_flag(csk, SOCK_LOCALROUTE);
+ if (sock_flag(psk, SOCK_RCVTSTAMP))
+ sock_set_flag(csk, SOCK_RCVTSTAMP);
- sdp_inet_wake_recv(listen_sk, 0);
-
- result = 0;
-locked_err:
- sdp_conn_unlock(listen_conn);
- sdp_conn_put(listen_conn); /* ListenLookup reference. */
-lookup_err:
- return result;
+ child->src_zthresh = parent->src_zthresh;
+ child->snk_zthresh = parent->snk_zthresh;
+ child->nodelay = parent->nodelay;
}
static int sdp_cm_hello_check(struct sdp_msg_hello *msg_hello)
@@ -415,8 +348,11 @@
int sdp_cm_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
{
struct sdp_msg_hello *msg_hello = event->private_data;
+ struct sdp_opt *listen_conn;
struct sdp_opt *conn;
int result;
+ u16 port;
+ u32 addr;
sdp_dbg_ctrl(NULL,
"CM REQ. comm <%08x> SID <%016llx> ca <%s> port <%d>",
@@ -430,7 +366,36 @@
if (result < 0) {
sdp_dbg_warn(NULL, "Error <%d> validating hello msg. <%08x>",
result, cm_id->local_id);
+ goto empty;
+ }
+
+ port = SDP_SID_TO_PORT(be64_to_cpu(cm_id->service_id));
+ addr = msg_hello->hh.dst.ipv4.addr;
+ /*
+ * first find a listening connection, and check backlog
+ */
+ result = -ECONNREFUSED;
+
+ listen_conn = sdp_inet_listen_lookup(addr, port);
+ if (!listen_conn) {
+ /*
+ * no connection, reject
+ */
+ sdp_dbg_ctrl(NULL, "no listener for connection. <%08x:%04x>",
+ addr, port);
+ goto empty;
+ }
+
+ sdp_conn_lock(listen_conn);
+
+ if (listen_conn->state != SDP_CONN_ST_LISTEN)
goto done;
+
+ if (listen_conn->backlog_cnt > listen_conn->backlog_max) {
+ sdp_dbg_ctrl(listen_conn,
+ "Listen backlog <%d> too big to accept new conn",
+ listen_conn->backlog_cnt);
+ goto done;
}
/*
* Create a connection for this request.
@@ -451,8 +416,8 @@
*/
SDP_CONN_ST_SET(conn, SDP_CONN_ST_REQ_RECV);
- conn->src_addr = msg_hello->hh.dst.ipv4.addr;
- conn->src_port = SDP_SID_TO_PORT(be64_to_cpu(cm_id->service_id));
+ conn->src_addr = addr;
+ conn->src_port = port;
conn->dst_addr = msg_hello->hh.src.ipv4.addr;
conn->dst_port = msg_hello->hh.port;
@@ -473,7 +438,7 @@
&event->param.req_rcvd.remote_ca_guid,
sizeof(conn->d_gid));
/*
- * update CM context to refer to the connection.
+ * update CM context to refer to the connection, before alloc_ib()
*/
conn->cm_id = cm_id;
conn->cm_id->context = hashent_arg(conn->hashent);
@@ -490,7 +455,7 @@
goto error;
}
/*
- * Save connect request info for QP modify.
+ * Save connect request info for QP modify in cm_accept().
*/
conn->d_lid = event->param.req_rcvd.primary_path->dlid;
conn->s_lid = event->param.req_rcvd.primary_path->slid;
@@ -498,19 +463,29 @@
conn->path_mtu = event->param.req_rcvd.primary_path->mtu;
/*
- * Find a matching listening socket, and insert new connection
- * into listeners accept queue.
+ * inherit listener properties
*/
- result = sdp_cm_listen_lookup(conn);
+ sdp_cm_listen_inherit(listen_conn, conn);
+ /*
+ * initiate a CM response message.
+ */
+ result = sdp_cm_accept(conn);
if (result < 0) {
- sdp_dbg_warn(conn, "Error <%d> matching listen socket queue",
- result);
+ sdp_dbg_warn(conn, "Error <%d> CM connect accept", result);
goto error;
}
/*
+ * place connection into the listen connections accept queue.
+ */
+ sdp_inet_accept_q_put(listen_conn, conn);
+ sdp_inet_wake_recv(sk_sdp(listen_conn), 0);
+ /*
* unlock
*/
sdp_conn_unlock(conn);
+ sdp_conn_unlock(listen_conn);
+ sdp_conn_put(listen_conn); /* ListenLookup reference. */
+
return 0;
error:
SDP_CONN_ST_SET(conn, SDP_CONN_ST_CLOSED);
@@ -518,6 +493,9 @@
sdp_conn_unlock(conn);
sdp_conn_put(conn); /* CM reference */
done:
+ sdp_conn_unlock(listen_conn);
+ sdp_conn_put(listen_conn); /* ListenLookup reference. */
+empty:
(void)ib_send_cm_rej(cm_id,
IB_CM_REJ_CONSUMER_DEFINED,
NULL, 0, NULL, 0);
More information about the general
mailing list