[linux-cifs-client] Re: [PATCH] cifs: when renaming don't try to unlink negative dentry
diff mbox

Message ID 20090418060558.016e3ef0@tleilax.poochiereds.net
State New, archived
Headers show

Commit Message

Jeff Layton April 18, 2009, 10:05 a.m. UTC
On Fri, 17 Apr 2009 21:31:50 -0500
Steve French <smfrench@gmail.com> wrote:

> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Fri, 17 Apr 2009 16:16:23 -0500
> > Steve French <smfrench@gmail.com> 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?

Comments

Steve French April 18, 2009, 2:43 p.m. UTC | #1
On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 17 Apr 2009 21:31:50 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> > On Fri, 17 Apr 2009 16:16:23 -0500
>> > Steve French <smfrench@gmail.com> 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?

Even if in some strange error path (the kind of cleanup code you added in
various rename paths for example) an internal caller of course has to
have a path (dentry) and should have an inode but I am not convinced
that we should force having a cached inode in all error/cleanup paths -
why is having a local cached copy of the inode fundamentally
required to do unlink (the server state, not the client state is what
is current).   For example, doing this check prevents doing
unlink when there is a negative dentry - but the file still may exist
on the server
(a negative dentry is just a "cached" directory entry - but the file may have
been recently created on the server).  I don't really see a reason why
why we should assume that a negative dentry or dentry without an inode
 (for example, perhaps there are other error cases) should always
prevent us from trying unlink.

I don't mind throwing the cERROR on this case, but it seems odd to require
(in all potential internal unlink uses) an inode if all we are doing
with the inode is
updating cached inode information (if we don't have cached inode information,
we can still send the SMB and do the unlink).

I do agree that today we do have a cached inode in all paths
that call into cifs_unlink in current cifs (presumably that was not
always the case)
(except the negative dentry case which your patch now forbids), but
the existence (or not) of the dentry->d_inode does not really have
anything to do with whether SMB Delete File would work or not.
Jeff Layton April 18, 2009, 7:32 p.m. UTC | #2
On Sat, 18 Apr 2009 09:43:18 -0500
Steve French <smfrench@gmail.com> wrote:

> On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Fri, 17 Apr 2009 21:31:50 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >> > On Fri, 17 Apr 2009 16:16:23 -0500
> >> > Steve French <smfrench@gmail.com> 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?
> 
> Even if in some strange error path (the kind of cleanup code you added in
> various rename paths for example) an internal caller of course has to
> have a path (dentry) and should have an inode but I am not convinced
> that we should force having a cached inode in all error/cleanup paths -
> why is having a local cached copy of the inode fundamentally
> required to do unlink (the server state, not the client state is what
> is current).   For example, doing this check prevents doing
> unlink when there is a negative dentry - but the file still may exist
> on the server
> (a negative dentry is just a "cached" directory entry - but the file may have
> been recently created on the server).  I don't really see a reason why
> why we should assume that a negative dentry or dentry without an inode
>  (for example, perhaps there are other error cases) should always
> prevent us from trying unlink.
> 
> I don't mind throwing the cERROR on this case, but it seems odd to require
> (in all potential internal unlink uses) an inode if all we are doing
> with the inode is
> updating cached inode information (if we don't have cached inode information,
> we can still send the SMB and do the unlink).
> 
> I do agree that today we do have a cached inode in all paths
> that call into cifs_unlink in current cifs (presumably that was not
> always the case)
> (except the negative dentry case which your patch now forbids), but
> the existence (or not) of the dentry->d_inode does not really have
> anything to do with whether SMB Delete File would work or not.

Why would we want the kernel to attempt to delete a file that it
doesn't believe exists? It seems reasonable to expect that the caller
have an up to date dcache for this.

I don't think we can reasonably have cifs_unlink handle the case where
we have a negative dentry. That will add a lot of complexity to this
codepath (and it's already very complex as it is). We have to recognize
that cifs_unlink does more than just SMB_Delete_File. It'll try to do a
busy file rename in some cases, the delete on close bit, etc.

If the caller just wants a simple SMB_Delete_File attempt then it
should just do that call itself.

That said, I don't feel terribly strongly about the BUG() call. If you
think it's better to just pop a printk and have cifs_unlink immediately
return an error then I can live with that.
Christoph Hellwig May 11, 2009, 9:04 a.m. UTC | #3
On Sat, Apr 18, 2009 at 03:32:02PM -0400, Jeff Layton wrote:
> I don't think we can reasonably have cifs_unlink handle the case where
> we have a negative dentry. That will add a lot of complexity to this
> codepath (and it's already very complex as it is). We have to recognize
> that cifs_unlink does more than just SMB_Delete_File. It'll try to do a
> busy file rename in some cases, the delete on close bit, etc.
> 
> If the caller just wants a simple SMB_Delete_File attempt then it
> should just do that call itself.
> 
> That said, I don't feel terribly strongly about the BUG() call. If you
> think it's better to just pop a printk and have cifs_unlink immediately
> return an error then I can live with that.

Please put the BUG_ON in.  The current code is typical cargo-cult
programming mess, and the newly added comment ontop of cifs_unlink
is highly confusing.

Patch
diff mbox

>From 7364188d2637bf58dca7c4ecdf6e73b5c281b4ad Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
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 <jlayton@redhat.com>
---
 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