From patchwork Sat Apr 18 10:05:58 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 18810 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 n3IA6mgh014108 for ; Sat, 18 Apr 2009 10:06:48 GMT Received: from dp.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id 5BCF1163C35 for ; Sat, 18 Apr 2009 10:06:27 +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 B48CD163C0E for ; Sat, 18 Apr 2009 10:05:48 +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 n3IA64Gc017278; Sat, 18 Apr 2009 06:06:04 -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 n3IA62Mk012553; Sat, 18 Apr 2009 06:06:03 -0400 Received: from tleilax.poochiereds.net (vpn-12-20.rdu.redhat.com [10.11.12.20]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id n3IA61A2009667; Sat, 18 Apr 2009 06:06:01 -0400 Date: Sat, 18 Apr 2009 06:05:58 -0400 From: Jeff Layton To: Steve French Message-ID: <20090418060558.016e3ef0@tleilax.poochiereds.net> In-Reply-To: <524f69650904171931q76b2e968s2174604378fa9b90@mail.gmail.com> References: <1239983130-4341-1-git-send-email-jlayton@redhat.com> <524f69650904171416y3807d6d2w7de57a711c5e5428@mail.gmail.com> <20090417190203.68229e6c@tleilax.poochiereds.net> <524f69650904171931q76b2e968s2174604378fa9b90@mail.gmail.com> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.58 on 172.16.27.26 Cc: galquezar@tv-wan.es, linux-cifs-client@lists.samba.org, shirishp@us.ibm.com Subject: [linux-cifs-client] Re: [PATCH] cifs: when renaming don't try to unlink negative dentry 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 Fri, 17 Apr 2009 21:31:50 -0500 Steve French wrote: > On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton wrote: > > On Fri, 17 Apr 2009 16:16:23 -0500 > > Steve French wrote: > > > >> I merged this, adding the CC: stable, but think we need to also > >> consistently check for inode == NULL in cifs_unlink (we only check in > >> two branches now) > >> > >> Any objections if I also add the following check: > >> > > > > I think it would be preferable to just check once for inode==NULL in > > cifs_unlink near the top and BUG() if it is. Note that vfs_unlink takes > > the i_mutex on this before calling the .unlink inode op, so we're > > guaranteed that the d_inode won't be NULL from that codepath. > > > > I think if we get an unlink on a negative dentry then we should > > probably consider that a BUG(). > > > > Other .unlink ops also seem to assume that you can't call .unlink with > > a negative dentry. > > I am more worried about internal calls to unlink from cifs slipping > through with inode null. If we check in one branch we should check in > the other, or as you suggest move it to the top > Yep, internal callers are my worry too. That's why I think a BUG() is appropriate to help catch this when it occurs. Maybe something like this patch? >From 7364188d2637bf58dca7c4ecdf6e73b5c281b4ad Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Sat, 18 Apr 2009 05:56:11 -0400 Subject: [PATCH] cifs: don't allow cifs_unlink to be called on negative dentry External callers (the VFS and knfsd) will never call the unlink inode op on a negative dentry. Internal callers must not do so either. Check for it and BUG() if it occurs. Signed-off-by: Jeff Layton --- fs/cifs/inode.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-) diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index f36b4e4..5476e0f 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -977,6 +977,11 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry) cFYI(1, ("cifs_unlink, dir=0x%p, dentry=0x%p", dir, dentry)); + if (!inode) { + cERROR(1, ("cifs_unlink called on negative dentry!")); + BUG(); + } + xid = GetXid(); /* Unlink can be called from rename so we can not take the @@ -1004,8 +1009,7 @@ retry_std_delete: psx_del_no_retry: if (!rc) { - if (inode) - drop_nlink(inode); + drop_nlink(inode); } else if (rc == -ENOENT) { d_drop(dentry); } else if (rc == -ETXTBSY) { @@ -1040,15 +1044,15 @@ psx_del_no_retry: cifs_set_file_info(inode, attrs, xid, full_path, origattr); out_reval: - if (inode) { - cifsInode = CIFS_I(inode); - cifsInode->time = 0; /* will force revalidate to get info - when needed */ - inode->i_ctime = current_fs_time(sb); - } + /* forces a revalidate when next needed */ + cifsInode = CIFS_I(inode); + cifsInode->time = 0; + + /* force revalidate of dir as well */ + inode->i_ctime = current_fs_time(sb); dir->i_ctime = dir->i_mtime = current_fs_time(sb); cifsInode = CIFS_I(dir); - CIFS_I(dir)->time = 0; /* force revalidate of dir as well */ + CIFS_I(dir)->time = 0; kfree(full_path); kfree(attrs); -- 1.6.0.6