From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH 01/19] SUNRPC: Fix error return value of svc_addr_len() Date: Tue, 28 Apr 2009 11:31:36 -0400 Message-ID: <20090428153136.GG17891@fieldses.org> References: <20090423231550.17283.24432.stgit@ingres.1015granger.net> <20090423233124.17283.40252.stgit@ingres.1015granger.net> <20090425221706.GD5088@fieldses.org> <130B5E68-8FE2-42F3-BA82-5B67FCAC6ACD@oracle.com> <20090427235147.GD5108@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-nfs@vger.kernel.org To: Chuck Lever Return-path: Received: from mail.fieldses.org ([141.211.133.115]:52604 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755084AbZD1Pbg (ORCPT ); Tue, 28 Apr 2009 11:31:36 -0400 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: On Tue, Apr 28, 2009 at 11:28:47AM -0400, Chuck Lever wrote: > > On Apr 27, 2009, at 7:51 PM, J. Bruce Fields wrote: > >> On Mon, Apr 27, 2009 at 12:49:07PM -0400, Chuck Lever wrote: >>> On Apr 25, 2009, at 6:17 PM, J. Bruce Fields wrote: >>>> On Thu, Apr 23, 2009 at 07:31:25PM -0400, Chuck Lever wrote: >>>>> The svc_addr_len() helper function returns -EAFNOSUPPORT if it >>>>> doesn't >>>>> recognize the address family of the passed-in socket address. >>>>> However, >>>>> the return type of this function is size_t, which means - >>>>> EAFNOSUPPORT >>>>> is turned into a very large positive value in this case. >>>>> >>>>> The check in svc_udp_recvfrom() to see if the return value is less >>>>> than zero therefore won't work at all. >>>>> >>>>> Additionally, handle_connect_req() passes this value directly to >>>>> memset(). This could cause memset() to clobber a large chunk of >>>>> memory >>>>> if svc_addr_len() has returned an error. Currently the address >>>>> family >>>>> of these addresses, however, is known to be supported long before >>>>> handle_connect_req() is called, so this isn't a real risk. >>>>> >>>>> Change the error return value of svc_addr_len() to zero, which fits >>>>> in >>>>> the range of size_t, and is safer to pass to memset() directly. >>>>> >>>>> Signed-off-by: Chuck Lever >>>>> --- >>>>> >>>>> include/linux/sunrpc/svc_xprt.h | 5 +++-- >>>>> net/sunrpc/svcsock.c | 7 ++++--- >>>>> 2 files changed, 7 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/ >>>>> sunrpc/ >>>>> svc_xprt.h >>>>> index 0d9cb6e..d790c52 100644 >>>>> --- a/include/linux/sunrpc/svc_xprt.h >>>>> +++ b/include/linux/sunrpc/svc_xprt.h >>>>> @@ -118,7 +118,7 @@ static inline unsigned short >>>>> svc_addr_port(const >>>>> struct sockaddr *sa) >>>>> return 0; >>>>> } >>>>> >>>>> -static inline size_t svc_addr_len(struct sockaddr *sa) >>>>> +static inline size_t svc_addr_len(const struct sockaddr *sa) >>>>> { >>>>> switch (sa->sa_family) { >>>>> case AF_INET: >>>>> @@ -126,7 +126,8 @@ static inline size_t svc_addr_len(struct >>>>> sockaddr *sa) >>>>> case AF_INET6: >>>>> return sizeof(struct sockaddr_in6); >>>>> } >>>>> - return -EAFNOSUPPORT; >>>>> + >>>> >>>> May as well stick a WARN() here too if only as a shorthand way of >>>> documenting that this isn't meant to happen. >>> >>> Sounds reasonable, but: >>> >>> 1. I thought BUG() was the way to document "shouldn't happen," and >> >> That'd be OK too. >> >>> >>> 2. What about all the other static inliney places this "shouldn't >>> happen" ? >> >> Such as? > > nlm_cmp_addr(), nlm_privileged_requester(), svc_addr_port(), > nfs_sockaddr_match_ipaddr(), nfs_sockaddr_cmp(), nfs_server_get_key(), > nfs_set_port(), nfs_verify_server_address(), and so on. I see. OK, I'm fine with leaving it as is.--b.