All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <error27@gmail.com>
To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: [patch 2/2] cifs: dereferencing first then checking
Date: Wed, 27 Oct 2010 21:22:53 +0000	[thread overview]
Message-ID: <20101027212253.GL6062@bicker> (raw)

Smatch complained about a couple checking for NULL after dereferencing
bugs.  I'm not super familiar with the code so I did the conservative
thing and move the dereferences after the checks.

The dereferences in cifs_lock() and cifs_fsync() were added in
ba00ba64cf0 "cifs: make various routines use the cifsFileInfo->tcon
pointer".  The dereference in find_writable_file() was added in 
6508d904e6f "cifs: have find_readable/writable_file filter by fsuid".
The comments there say it's possible to trigger the NULL dereference
under stress.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06a006b..33a19b4 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -775,13 +775,13 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 		cFYI(1, "Unknown type of lock");
 
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
-	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 
 	if (file->private_data = NULL) {
 		rc = -EBADF;
 		FreeXid(xid);
 		return rc;
 	}
+	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&
@@ -1179,7 +1179,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 					bool fsuid_only)
 {
 	struct cifsFileInfo *open_file;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+	struct cifs_sb_info *cifs_sb;
 	bool any_available = false;
 	int rc;
 
@@ -1192,6 +1192,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 		dump_stack();
 		return NULL;
 	}
+	cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
 
 	/* only filter by fsuid on multiuser mounts */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
@@ -1628,15 +1629,26 @@ int cifs_fsync(struct file *file, int datasync)
 		file->f_path.dentry->d_name.name, datasync);
 
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc = 0) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!rc && tcon && smbfile &&
-		   !(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	if (rc)
+		goto err_out;
+
+	rc = CIFS_I(inode)->write_behind_rc;
+	CIFS_I(inode)->write_behind_rc = 0;
+	if (rc)
+		goto err_out;
+
+	if (CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)
+		goto done;
+	if (!smbfile)
+		goto done;
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!tcon)
+		goto done;
+
+	rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
+err_out:
+done:
 	FreeXid(xid);
 	return rc;
 }

WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org,
	kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: [patch 2/2] cifs: dereferencing first then checking
Date: Wed, 27 Oct 2010 23:22:53 +0200	[thread overview]
Message-ID: <20101027212253.GL6062@bicker> (raw)

Smatch complained about a couple checking for NULL after dereferencing
bugs.  I'm not super familiar with the code so I did the conservative
thing and move the dereferences after the checks.

The dereferences in cifs_lock() and cifs_fsync() were added in
ba00ba64cf0 "cifs: make various routines use the cifsFileInfo->tcon
pointer".  The dereference in find_writable_file() was added in 
6508d904e6f "cifs: have find_readable/writable_file filter by fsuid".
The comments there say it's possible to trigger the NULL dereference
under stress.

Signed-off-by: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 06a006b..33a19b4 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -775,13 +775,13 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 		cFYI(1, "Unknown type of lock");
 
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
-	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 
 	if (file->private_data == NULL) {
 		rc = -EBADF;
 		FreeXid(xid);
 		return rc;
 	}
+	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&
@@ -1179,7 +1179,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 					bool fsuid_only)
 {
 	struct cifsFileInfo *open_file;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+	struct cifs_sb_info *cifs_sb;
 	bool any_available = false;
 	int rc;
 
@@ -1192,6 +1192,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 		dump_stack();
 		return NULL;
 	}
+	cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
 
 	/* only filter by fsuid on multiuser mounts */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
@@ -1628,15 +1629,26 @@ int cifs_fsync(struct file *file, int datasync)
 		file->f_path.dentry->d_name.name, datasync);
 
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc == 0) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!rc && tcon && smbfile &&
-		   !(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	if (rc)
+		goto err_out;
+
+	rc = CIFS_I(inode)->write_behind_rc;
+	CIFS_I(inode)->write_behind_rc = 0;
+	if (rc)
+		goto err_out;
+
+	if (CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)
+		goto done;
+	if (!smbfile)
+		goto done;
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!tcon)
+		goto done;
+
+	rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
+err_out:
+done:
 	FreeXid(xid);
 	return rc;
 }

             reply	other threads:[~2010-10-27 21:22 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-10-27 21:22 Dan Carpenter [this message]
2010-10-27 21:22 ` [patch 2/2] cifs: dereferencing first then checking Dan Carpenter
2010-10-27 23:51 ` Jeff Layton
2010-10-27 23:51   ` Jeff Layton
     [not found]   ` <20101027195123.0a712a53-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2010-10-28  3:44     ` Dan Carpenter
2010-10-28  3:44       ` Dan Carpenter
2010-11-02 20:22       ` Jeff Layton
2010-11-02 20:22         ` Jeff Layton
     [not found]         ` <20101102162250.3a2aa670-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2010-11-03 16:40           ` Dan Carpenter
2010-11-03 16:40             ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20101027212253.GL6062@bicker \
    --to=error27@gmail.com \
    --cc=kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=samba-technical-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org \
    --cc=sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.