[linux-cifs-client] Re: [PATCH] cifs: fix pointer initialization and checks in cifs_follow_symlink (try #3)

[Jeff Moyer]

Jeff Layton <jlayton at redhat.com> writes:

> On Tue, 19 May 2009 09:42:02 -0400
> Jeff Moyer <jmoyer at redhat.com> 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.
>
> -- 
> Jeff Layton <jlayton at redhat.com>
> From 5b8001cf69914faa6338074a45a1285f2761a7d8 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton at redhat.com>
> 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 <jmoyer at redhat.com>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>

Reviewed-by: Jeff Moyer <jmoyer at redhat.com>