From: "J. Bruce Fields" Subject: Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len() Date: Tue, 24 Mar 2009 16:32:06 -0400 Message-ID: <20090324203206.GH19389@fieldses.org> References: <20090319004024.32404.68289.stgit@ingres.1015granger.net> <20090319004535.32404.37120.stgit@ingres.1015granger.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: trond.myklebust@fys.uio.no, linux-nfs@vger.kernel.org, Tom Tucker To: Chuck Lever Return-path: Received: from mail.fieldses.org ([141.211.133.115]:44845 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754796AbZCXUcM (ORCPT ); Tue, 24 Mar 2009 16:32:12 -0400 In-Reply-To: <20090319004535.32404.37120.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Wed, Mar 18, 2009 at 08:45:36PM -0400, Chuck Lever wrote: > The svc_addr_len() helper function can return a negative errno value, > but its return type is size_t, which is unsigned. > > The RDMA transport code passes this return value directly to memset(), > without checking first if it's negative. This could cause memset() to > clobber a large piece of memory if svc_addr_len() has returned an > error. I'd still like to understand when this can happen, to better understand how the error should be handled. Also: > > Signed-off-by: Chuck Lever > --- > > include/linux/sunrpc/svc_xprt.h | 3 ++- > net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++++-- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h > index 0127dac..c2aa8cd 100644 > --- a/include/linux/sunrpc/svc_xprt.h > +++ b/include/linux/sunrpc/svc_xprt.h > @@ -113,7 +113,7 @@ static inline unsigned short svc_addr_port(struct sockaddr *sa) > return ret; > } > > -static inline size_t svc_addr_len(struct sockaddr *sa) > +static inline int svc_addr_len(const struct sockaddr *sa) > { > switch (sa->sa_family) { > case AF_INET: > @@ -121,6 +121,7 @@ static inline size_t svc_addr_len(struct sockaddr *sa) > case AF_INET6: > return sizeof(struct sockaddr_in6); > } > + > return -EAFNOSUPPORT; > } > > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c > index 3d810e7..d1ec6f9 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c > @@ -546,6 +546,7 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird) > struct svcxprt_rdma *listen_xprt = new_cma_id->context; > struct svcxprt_rdma *newxprt; > struct sockaddr *sa; > + int len; > > /* Create a new transport */ > newxprt = rdma_create_xprt(listen_xprt->sc_xprt.xpt_server, 0); > @@ -563,9 +564,20 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird) > > /* Set the local and remote addresses in the transport */ > sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr; > - svc_xprt_set_remote(&newxprt->sc_xprt, sa, svc_addr_len(sa)); > + len = svc_addr_len(sa); > + if (len < 0) { > + dprintk("svcrdma: dst_addr has a bad address family\n"); > + return; we're probably leaking something here. I don't want to fix this until it's understood well enough to fix it correctly. --b. > + } > + svc_xprt_set_remote(&newxprt->sc_xprt, sa, len); > + > sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.src_addr; > - svc_xprt_set_local(&newxprt->sc_xprt, sa, svc_addr_len(sa)); > + len = svc_addr_len(sa); > + if (len < 0) { > + dprintk("svcrdma: src_addr has a bad address family\n"); > + return; > + } > + svc_xprt_set_local(&newxprt->sc_xprt, sa, len); > > /* > * Enqueue the new transport on the accept queue of the listening >