From patchwork Tue May 19 14:00:37 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 24721 Received: from lists.samba.org (mail.samba.org [66.70.73.150]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n4JE12Se029274 for ; Tue, 19 May 2009 14:01:03 GMT Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id EF32A163CB4 for ; Tue, 19 May 2009 14:00:34 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.1.7 (2006-10-05) on dp.samba.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.8 tests=AWL,BAYES_00, FORGED_RCVD_HELO,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.1.7 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from mx2.redhat.com (mx2.redhat.com [66.187.237.31]) by lists.samba.org (Postfix) with ESMTP id D004E163C24 for ; Tue, 19 May 2009 14:00:14 +0000 (GMT) Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n4JE0f7x003565; Tue, 19 May 2009 10:00:41 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n4JE0egr017449; Tue, 19 May 2009 10:00:40 -0400 Received: from tlielax.poochiereds.net (vpn-12-178.rdu.redhat.com [10.11.12.178]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n4JE0dGn014286; Tue, 19 May 2009 10:00:39 -0400 Date: Tue, 19 May 2009 10:00:37 -0400 From: Jeff Layton To: Jeff Moyer Message-ID: <20090519100037.7fd35727@tlielax.poochiereds.net> In-Reply-To: References: <1242681230-11490-1-git-send-email-jlayton@redhat.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.58 on 172.16.27.26 Cc: smfrench@gmail.com, linux-cifs-client@lists.samba.org Subject: [linux-cifs-client] Re: [PATCH] cifs: fix pointer initialization and checks in cifs_follow_symlink (try #3) X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces+patchwork-cifs-client=patchwork.kernel.org@lists.samba.org Errors-To: linux-cifs-client-bounces+patchwork-cifs-client=patchwork.kernel.org@lists.samba.org On Tue, 19 May 2009 09:42:02 -0400 Jeff Moyer wrote: > Well, you've sacrificed your logging for this. See below. > > > index ea9d11e..737a386 100644 > > --- a/fs/cifs/link.c > > +++ b/fs/cifs/link.c > > @@ -107,48 +107,48 @@ void * > > cifs_follow_link(struct dentry *direntry, struct nameidata *nd) > > { > > struct inode *inode = direntry->d_inode; > > - int rc = -EACCES; > > + int rc = -ENOMEM; > > int xid; > > char *full_path = NULL; > > - char *target_path = ERR_PTR(-ENOMEM); > > - struct cifs_sb_info *cifs_sb; > > - struct cifsTconInfo *pTcon; > > + char *target_path = NULL; > > + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb); > > + struct cifsTconInfo *tcon = cifs_sb->tcon; > > > > xid = GetXid(); > > > > - full_path = build_path_from_dentry(direntry); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > - > > - if (!full_path) > > - goto out; > > - > > cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Aside from that, the patch looks good. > > Cheers, > Jeff Good catch. There's still logging done on the function entry and exit (via the GetXid/FreeXid side effects), so it's not a complete loss. How's this instead? The only difference is that I moved the cFYI statement down. Reviewed-by: Jeff Moyer >From 5b8001cf69914faa6338074a45a1285f2761a7d8 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Tue, 19 May 2009 09:57:03 -0400 Subject: [PATCH] cifs: fix pointer initialization and checks in cifs_follow_symlink (try #4) This is the third respin of the patch posted yesterday to fix the error handling in cifs_follow_symlink. It also includes a fix for a bogus NULL pointer check in CIFSSMBQueryUnixSymLink that Jeff Moyer spotted. It's possible for CIFSSMBQueryUnixSymLink to return without setting target_path to a valid pointer. If that happens then the current value to which we're initializing this pointer could cause an oops when it's kfree'd. This patch is a little more comprehensive than the last patches. It reorganizes cifs_follow_link a bit for (hopefully) better readability. It should also eliminate the uneeded allocation of full_path on servers without unix extensions (assuming they can get to this point anyway, of which I'm not convinced). On a side note, I'm not sure I agree with the logic of enabling this query even when unix extensions are disabled on the client. It seems like that should disable this as well. But, changing that is outside the scope of this fix, so I've left it alone for now. Reported-by: Jeff Moyer Signed-off-by: Jeff Layton --- fs/cifs/cifssmb.c | 2 +- fs/cifs/link.c | 52 ++++++++++++++++++++++++++-------------------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 5759ba5..d062602 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -2475,7 +2475,7 @@ querySymLinkRetry: /* BB FIXME investigate remapping reserved chars here */ *symlinkinfo = cifs_strndup_from_ucs(data_start, count, is_unicode, nls_codepage); - if (!symlinkinfo) + if (!*symlinkinfo) rc = -ENOMEM; } } diff --git a/fs/cifs/link.c b/fs/cifs/link.c index ea9d11e..cd83c53 100644 --- a/fs/cifs/link.c +++ b/fs/cifs/link.c @@ -107,48 +107,48 @@ void * cifs_follow_link(struct dentry *direntry, struct nameidata *nd) { struct inode *inode = direntry->d_inode; - int rc = -EACCES; + int rc = -ENOMEM; int xid; char *full_path = NULL; - char *target_path = ERR_PTR(-ENOMEM); - struct cifs_sb_info *cifs_sb; - struct cifsTconInfo *pTcon; + char *target_path = NULL; + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb); + struct cifsTconInfo *tcon = cifs_sb->tcon; xid = GetXid(); - full_path = build_path_from_dentry(direntry); + /* + * For now, we just handle symlinks with unix extensions enabled. + * Eventually we should handle NTFS reparse points, and MacOS + * symlink support. For instance... + * + * rc = CIFSSMBQueryReparseLinkInfo(...) + * + * For now, just return -EACCES when the server doesn't support posix + * extensions. Note that we still allow querying symlinks when posix + * extensions are manually disabled. We could disable these as well + * but there doesn't seem to be any harm in allowing the client to + * read them. + */ + if (!(tcon->ses->capabilities & CAP_UNIX)) { + rc = -EACCES; + goto out; + } + full_path = build_path_from_dentry(direntry); if (!full_path) goto out; cFYI(1, ("Full path: %s inode = 0x%p", full_path, inode)); - cifs_sb = CIFS_SB(inode->i_sb); - pTcon = cifs_sb->tcon; - - /* We could change this to: - if (pTcon->unix_ext) - but there does not seem any point in refusing to - get symlink info if we can, even if unix extensions - turned off for this mount */ - - if (pTcon->ses->capabilities & CAP_UNIX) - rc = CIFSSMBUnixQuerySymLink(xid, pTcon, full_path, - &target_path, - cifs_sb->local_nls); - else { - /* BB add read reparse point symlink code here */ - /* rc = CIFSSMBQueryReparseLinkInfo */ - /* BB Add code to Query ReparsePoint info */ - /* BB Add MAC style xsymlink check here if enabled */ - } + rc = CIFSSMBUnixQuerySymLink(xid, tcon, full_path, &target_path, + cifs_sb->local_nls); + kfree(full_path); +out: if (rc != 0) { kfree(target_path); target_path = ERR_PTR(rc); } - kfree(full_path); -out: FreeXid(xid); nd_set_link(nd, target_path); return NULL; -- 1.6.0.6