From: "J. Bruce Fields" Subject: Re: [PATCH] nfs: Fix misparsing of nfsv4 fs_locations attribute Date: Fri, 9 May 2008 21:06:59 -0400 Message-ID: <20080510010659.GN1907@fieldses.org> References: <20080509011918.GK12690@fieldses.org> <1210309839.8657.0.camel@localhost> <20080509152750.GA325@fieldses.org> <20080509165204.GB1907@fieldses.org> <20080509171208.GC1907@fieldses.org> <20080509235930.GM1907@fieldses.org> <4824E933.1000108@panasas.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Trond Myklebust , Trond Myklebust , linux-nfs@vger.kernel.org, Manoj Naik To: Benny Halevy Return-path: Received: from mail.fieldses.org ([66.93.2.214]:59860 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751750AbYEJBHS (ORCPT ); Fri, 9 May 2008 21:07:18 -0400 In-Reply-To: <4824E933.1000108@panasas.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, May 09, 2008 at 05:15:47PM -0700, Benny Halevy wrote: > On May. 09, 2008, 16:59 -0700, "J. Bruce Fields" wrote: > > From: J. Bruce Fields > > > > The code incorrectly assumes here that the server name (or ip address) > > is null-terminated. This can cause referrals to fail in some cases. > > > > Signed-off-by: J. Bruce Fields > > --- > > fs/nfs/nfs4namespace.c | 12 +++++++++--- > > 1 files changed, 9 insertions(+), 3 deletions(-) > > > > I think this was the bug causing the referral failures. There must be a > > less ugly solution, though. > > > > --b. > > > > diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c > > index 5f9ba41..2684c65 100644 > > --- a/fs/nfs/nfs4namespace.c > > +++ b/fs/nfs/nfs4namespace.c > > @@ -96,11 +96,17 @@ static int nfs4_validate_fspath(const struct vfsmount *mnt_parent, > > /* > > * Check if the string represents a "valid" IPv4 address > > */ > > -static inline int valid_ipaddr4(const char *buf) > > +static inline int valid_ipaddr4(const struct nfs4_string *buf) > > { > > int rc, count, in[4]; > > > > - rc = sscanf(buf, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]); > > + /* > > + * XXX: Depending on the knowledge that the following byte is > > + * either xdr padding or an xdr length that has already been > > + * copied: > > + */ > > + buf->data[buf->len] = '\0'; > > Hmm, can't that overflow if buf->len is word aligned? The xdr here is: struct fs_location4 { utf8str_cis server<>; pathname4 rootpath; }; and we're adding the padding to the end of one of the elements of the server<> array. So it's guaranteed to be followed by a 4-byte length. Oh, and we're also keeping the result in a single page, so there's no crossing of a page boundary. So it's probably correct. It's too ugly to actually apply, though. --b. > > Benny > > > + rc = sscanf(buf->data, "%d.%d.%d.%d", &in[0], &in[1], &in[2], &in[3]); > > if (rc != 4) > > return -EINVAL; > > for (count = 0; count < 4; count++) { > > @@ -178,7 +184,7 @@ static struct vfsmount *nfs_follow_referral(const struct vfsmount *mnt_parent, > > }; > > > > if (location->servers[s].len <= 0 || > > - valid_ipaddr4(location->servers[s].data) < 0) { > > + valid_ipaddr4(&location->servers[s]) < 0) { > > s++; > > continue; > > } > > > -- > Benny Halevy > Software Architect > Tel/Fax: +972-3-647-8340 > Mobile: +972-54-802-8340 > US: +1-412-203-3187 > bhalevy@panasas.com > > Panasas, Inc. > Leader in Parallel Storage > www.panasas.com