From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761352Ab0J0QMn (ORCPT ); Wed, 27 Oct 2010 12:12:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57358 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761158Ab0J0QMl (ORCPT ); Wed, 27 Oct 2010 12:12:41 -0400 Date: Wed, 27 Oct 2010 12:13:38 -0400 From: Jeff Layton To: Jiri Slaby Cc: sfrench@samba.org, linux-kernel@vger.kernel.org, jirislaby@gmail.com, linux-cifs@vger.kernel.org Subject: Re: [PATCH 2/4] FS: cifs, remove unneeded NULL tests Message-ID: <20101027121338.2984b9d8@corrin.poochiereds.net> In-Reply-To: <1288180220-19869-1-git-send-email-jslaby@suse.cz> References: <1288180220-19869-1-git-send-email-jslaby@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 27 Oct 2010 13:50:20 +0200 Jiri Slaby wrote: > Stanse found that pSMBFile in cifs_ioctl and file->f_path.dentry in > cifs_user_write are dereferenced prior their test to NULL. > > The alternative is not to dereference them before the tests. The patch is > to point out the problem, you have to decide. > > Signed-off-by: Jiri Slaby > Cc: Steve French > Cc: linux-cifs@vger.kernel.org > --- > fs/cifs/file.c | 2 +- > fs/cifs/ioctl.c | 4 ---- > 2 files changed, 1 insertions(+), 5 deletions(-) > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c > index 45af003..db7eaf7 100644 > --- a/fs/cifs/file.c > +++ b/fs/cifs/file.c > @@ -1031,7 +1031,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data, > cifs_stats_bytes_written(pTcon, total_written); > > /* since the write may have blocked check these pointers again */ > - if ((file->f_path.dentry) && (file->f_path.dentry->d_inode)) { > + if (file->f_path.dentry->d_inode) { ^^^^^^ This check is bogus too. An open filp on a negative dentry isn't possible, right? > struct inode *inode = file->f_path.dentry->d_inode; > /* Do not update local mtime - server will set its actual value on write > * inode->i_ctime = inode->i_mtime = > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > index 077bf75..2fa22f2 100644 > --- a/fs/cifs/ioctl.c > +++ b/fs/cifs/ioctl.c > @@ -63,8 +63,6 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) > #ifdef CONFIG_CIFS_POSIX > case FS_IOC_GETFLAGS: > if (CIFS_UNIX_EXTATTR_CAP & caps) { > - if (pSMBFile == NULL) > - break; > rc = CIFSGetExtAttr(xid, tcon, pSMBFile->netfid, > &ExtAttrBits, &ExtAttrMask); > if (rc == 0) > @@ -80,8 +78,6 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) > rc = -EFAULT; > break; > } > - if (pSMBFile == NULL) > - break; > /* rc= CIFSGetExtAttr(xid,tcon,pSMBFile->netfid, > extAttrBits, &ExtAttrMask);*/ > } Acked-by: Jeff Layton