[PATCH RESEND] cifs: file: initialize oparms.reconnect before using it

[PATCH RESEND] cifs: file: initialize oparms.reconnect before using it

[Andi Shyti]

In the cifs_reopen_file function, if the following statement is
asserted:

(tcon->unix_ext && cap_unix(tcon->ses) &&
		(CIFS_UNIX_POSIX_PATH_OPS_CAP &
		(tcon->fsUnixInfo.Capability)))

and we succeed to open with cifs_posix_open, the function jumps
to the label reopen_success and checks for oparms.reconnect
which is not initialized.

To avoid this the oparms structure initialization is anticipated
before the if statement.

This issue has been reported by scan.coverity.com

Signed-off-by: Andi Shyti <andi@etezian.org>
---
 fs/cifs/file.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..fbeaf45 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -632,6 +632,15 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
 	else
 		oplock = 0;
 
+	oparms.tcon = tcon;
+	oparms.cifs_sb = cifs_sb;
+	oparms.desired_access = desired_access;
+	oparms.create_options = create_options;
+	oparms.disposition = disposition;
+	oparms.path = full_path;
+	oparms.fid = &cfile->fid;
+	oparms.reconnect = true;
+
 	if (tcon->unix_ext && cap_unix(tcon->ses) &&
 	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
 				le64_to_cpu(tcon->fsUnixInfo.Capability))) {
@@ -663,15 +672,6 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
 	if (server->ops->get_lease_key)
 		server->ops->get_lease_key(inode, &cfile->fid);
 
-	oparms.tcon = tcon;
-	oparms.cifs_sb = cifs_sb;
-	oparms.desired_access = desired_access;
-	oparms.create_options = create_options;
-	oparms.disposition = disposition;
-	oparms.path = full_path;
-	oparms.fid = &cfile->fid;
-	oparms.reconnect = true;
-
 	/*
 	 * Can not refresh inode by passing in file_info buf to be returned by
 	 * CIFSSMBOpen and then calling get_inode_info with returned buf since
-- 
1.8.3.2

Re: [PATCH RESEND] cifs: file: initialize oparms.reconnect before using it

[Jeff Layton]

On Mon, 29 Jul 2013 10:58:13 +0200
Andi Shyti <andi@etezian.org> wrote:

> In the cifs_reopen_file function, if the following statement is
> asserted:
> 
> (tcon->unix_ext && cap_unix(tcon->ses) &&
> 		(CIFS_UNIX_POSIX_PATH_OPS_CAP &
> 		(tcon->fsUnixInfo.Capability)))
> 
> and we succeed to open with cifs_posix_open, the function jumps
> to the label reopen_success and checks for oparms.reconnect
> which is not initialized.
> 
> To avoid this the oparms structure initialization is anticipated
> before the if statement.
> 
> This issue has been reported by scan.coverity.com
> 
> Signed-off-by: Andi Shyti <andi@etezian.org>
> ---
>  fs/cifs/file.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 1e57f36..fbeaf45 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -632,6 +632,15 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
>  	else
>  		oplock = 0;
>  
> +	oparms.tcon = tcon;
> +	oparms.cifs_sb = cifs_sb;
> +	oparms.desired_access = desired_access;
> +	oparms.create_options = create_options;

This patch just moves the brokenness around. You're
setting .desired_access here to an unintialized variable.
create_options also looks like it may potentially be wrong at this
point.

It may be that the code won't trip over these bugs in its current form,
but it's not really doing much to "future-proof" it. I think this
function needs a bit more refactoring instead of increasing the level
of spaghetti.

> +	oparms.disposition = disposition;
> +	oparms.path = full_path;
> +	oparms.fid = &cfile->fid;
> +	oparms.reconnect = true;
> +
>  	if (tcon->unix_ext && cap_unix(tcon->ses) &&
>  	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
>  				le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> @@ -663,15 +672,6 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
>  	if (server->ops->get_lease_key)
>  		server->ops->get_lease_key(inode, &cfile->fid);
>  
> -	oparms.tcon = tcon;
> -	oparms.cifs_sb = cifs_sb;
> -	oparms.desired_access = desired_access;
> -	oparms.create_options = create_options;
> -	oparms.disposition = disposition;
> -	oparms.path = full_path;
> -	oparms.fid = &cfile->fid;
> -	oparms.reconnect = true;
> -
>  	/*
>  	 * Can not refresh inode by passing in file_info buf to be returned by
>  	 * CIFSSMBOpen and then calling get_inode_info with returned buf since


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH RESEND] cifs: file: initialize oparms.reconnect before using it

[Andi Shyti]

> > +	oparms.tcon = tcon;
> > +	oparms.cifs_sb = cifs_sb;
> > +	oparms.desired_access = desired_access;
> > +	oparms.create_options = create_options;
> 
> This patch just moves the brokenness around. You're
> setting .desired_access here to an unintialized variable.
> create_options also looks like it may potentially be wrong at this
> point.

Urrrca! This is what I achieve when I do one last fix before
going to sleep.

I spent a bit of time more going through the cifs/smb code and
the most sensful fix looks this

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..7e36ae3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flu
                                     oflags, &oplock, &cfile->fid.netfid, xid
                if (rc == 0) {
                        cifs_dbg(FYI, "posix reopen succeeded\n");
+                       oparms.reconnect = true;
                        goto reopen_success;
                }
                /*

There is only one case when reconnect becames false, that is when
open = smb2_open_file and calls SMB2_open() that calls
add_durable_context() that sets reconnect = false with some
nested ifs in between, and still only in case
everything succeeds. We are checking reconnect only for this
case, otherwise we could get rid of the if (oparms.reconnect) and
not falling into the unknown state.

If it makes sense, I can send the above suggestion.

Thanks,
Andi

[PATCH v2] cifs: file: initialize oparms.reconnect before using it

[Andi Shyti]

In the cifs_reopen_file function, if the following statement is
asserted:

(tcon->unix_ext && cap_unix(tcon->ses) &&
            (CIFS_UNIX_POSIX_PATH_OPS_CAP &
            (tcon->fsUnixInfo.Capability)))

and we succeed to open with cifs_posix_open, the function jumps
to the label reopen_success and checks for oparms.reconnect
which is not initialized.

This issue has been reported by scan.coverity.com

Signed-off-by: Andi Shyti <andi@etezian.org>
---
 fs/cifs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..7e36ae3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
 				     oflags, &oplock, &cfile->fid.netfid, xid);
 		if (rc == 0) {
 			cifs_dbg(FYI, "posix reopen succeeded\n");
+			oparms.reconnect = true;
 			goto reopen_success;
 		}
 		/*
-- 
1.8.3.2

Re: [PATCH v2] cifs: file: initialize oparms.reconnect before using it

[Jeff Layton]

On Mon, 29 Jul 2013 20:04:35 +0200
Andi Shyti <andi@etezian.org> wrote:

> In the cifs_reopen_file function, if the following statement is
> asserted:
> 
> (tcon->unix_ext && cap_unix(tcon->ses) &&
>             (CIFS_UNIX_POSIX_PATH_OPS_CAP &
>             (tcon->fsUnixInfo.Capability)))
> 
> and we succeed to open with cifs_posix_open, the function jumps
> to the label reopen_success and checks for oparms.reconnect
> which is not initialized.
> 
> This issue has been reported by scan.coverity.com
> 
> Signed-off-by: Andi Shyti <andi@etezian.org>
> ---
>  fs/cifs/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 1e57f36..7e36ae3 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
>  				     oflags, &oplock, &cfile->fid.netfid, xid);
>  		if (rc == 0) {
>  			cifs_dbg(FYI, "posix reopen succeeded\n");
> +			oparms.reconnect = true;
>  			goto reopen_success;
>  		}
>  		/*

Still doesn't do much to improve this code, but that fix shouldn't
break anything.

Reviewed-by: Jeff Layton <jlayton@redhat.com>