From: "J. Bruce Fields" Subject: Re: [PATCH 02/23] SUNRPC: Fix return type of svc_addr_len() Date: Tue, 24 Mar 2009 17:02:23 -0400 Message-ID: <20090324210223.GI19389@fieldses.org> References: <20090319004024.32404.68289.stgit@ingres.1015granger.net> <20090319004535.32404.37120.stgit@ingres.1015granger.net> <20090324203206.GH19389@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Tom Tucker , Trond Myklebust , Linux NFS Mailing List To: Chuck Lever Return-path: Received: from mail.fieldses.org ([141.211.133.115]:47350 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751388AbZCXVC1 (ORCPT ); Tue, 24 Mar 2009 17:02:27 -0400 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, Mar 24, 2009 at 04:37:49PM -0400, Chuck Lever wrote: > On Mar 24, 2009, at 4:32 PM, J. Bruce Fields wrote: >> 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; >>> } > > It's clearly a bug to return -EAFNOSUPPORT in a size_t. I agree. My comment is just that this patch doesn't necessarily fix the real bug; so as long as there's a bug here, I'd prefer it to be an obvious one. >>> 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. > > Tom needs respond to this, but I think it would be safe to simply BUG() > here. Could be.--b.