[PATCH] coda: kill file_count abuse

[PATCH] coda: kill file_count abuse

[Christoph Hellwig]

->release is the proper way to detect the last close of a file,
file_count should never be used in filesystems.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/coda/dir.c
===================================================================
--- linux-2.6.orig/fs/coda/dir.c	2007-07-19 22:40:25.000000000 +0200
+++ linux-2.6/fs/coda/dir.c	2007-07-19 22:40:27.000000000 +0200
@@ -86,7 +86,6 @@ const struct file_operations coda_dir_op
 	.read		= generic_read_dir,
 	.readdir	= coda_readdir,
 	.open		= coda_open,
-	.flush		= coda_flush,
 	.release	= coda_release,
 	.fsync		= coda_fsync,
 };
Index: linux-2.6/fs/coda/file.c
===================================================================
--- linux-2.6.orig/fs/coda/file.c	2007-07-19 22:36:38.000000000 +0200
+++ linux-2.6/fs/coda/file.c	2007-07-19 22:40:13.000000000 +0200
@@ -163,58 +163,31 @@ int coda_open(struct inode *coda_inode, 
 	return 0;
 }
 
-int coda_flush(struct file *coda_file, fl_owner_t id)
+int coda_release(struct inode *coda_inode, struct file *coda_file)
 {
 	unsigned short flags = coda_file->f_flags & ~O_EXCL;
 	unsigned short coda_flags = coda_flags_to_cflags(flags);
-	struct coda_file_info *cfi;
-	struct inode *coda_inode;
-	int err = 0, fcnt;
+	struct coda_file_info *cfi = CODA_FTOC(coda_file);
+	struct coda_inode_info *cii;
+	struct inode *host_inode;
+	int err = 0;
 
 	lock_kernel();
 
-	/* last close semantics */
-	fcnt = file_count(coda_file);
-	if (fcnt > 1)
-		goto out;
-
 	/* No need to make an upcall when we have not made any modifications
 	 * to the file */
-	if ((coda_file->f_flags & O_ACCMODE) == O_RDONLY)
-		goto out;
-
-	if (use_coda_close)
-		goto out;
+	if (((coda_file->f_flags & O_ACCMODE) != O_RDONLY) && !use_coda_close) {
+		BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
 
-	cfi = CODA_FTOC(coda_file);
-	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
-
-	coda_inode = coda_file->f_path.dentry->d_inode;
-
-	err = venus_store(coda_inode->i_sb, coda_i2f(coda_inode), coda_flags,
-			  coda_file->f_uid);
+		err = venus_store(coda_inode->i_sb, coda_i2f(coda_inode), coda_flags,
+				  coda_file->f_uid);
 
-	if (err == -EOPNOTSUPP) {
-		use_coda_close = 1;
-		err = 0;
+		if (err == -EOPNOTSUPP) {
+			use_coda_close = 1;
+			err = 0;
+		}
 	}
 
-out:
-	unlock_kernel();
-	return err;
-}
-
-int coda_release(struct inode *coda_inode, struct file *coda_file)
-{
-	unsigned short flags = (coda_file->f_flags) & (~O_EXCL);
-	unsigned short coda_flags = coda_flags_to_cflags(flags);
-	struct coda_file_info *cfi;
-	struct coda_inode_info *cii;
-	struct inode *host_inode;
-	int err = 0;
-
-	lock_kernel();
-
 	if (!use_coda_close) {
 		err = venus_release(coda_inode->i_sb, coda_i2f(coda_inode),
 				    coda_flags);
@@ -224,7 +197,6 @@ int coda_release(struct inode *coda_inod
 		}
 	}
 
-	cfi = CODA_FTOC(coda_file);
 	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
 
 	if (use_coda_close)
@@ -288,7 +260,6 @@ const struct file_operations coda_file_o
 	.write		= coda_file_write,
 	.mmap		= coda_file_mmap,
 	.open		= coda_open,
-	.flush		= coda_flush,
 	.release	= coda_release,
 	.fsync		= coda_fsync,
 	.splice_read	= coda_file_splice_read,
Index: linux-2.6/include/linux/coda_linux.h
===================================================================
--- linux-2.6.orig/include/linux/coda_linux.h	2007-07-19 22:40:33.000000000 +0200
+++ linux-2.6/include/linux/coda_linux.h	2007-07-19 22:40:35.000000000 +0200
@@ -36,7 +36,6 @@ extern const struct file_operations coda
 
 /* operations shared over more than one file */
 int coda_open(struct inode *i, struct file *f);
-int coda_flush(struct file *f, fl_owner_t id);
 int coda_release(struct inode *i, struct file *f);
 int coda_permission(struct inode *inode, int mask, struct nameidata *nd);
 int coda_revalidate_inode(struct dentry *);

Re: [PATCH] coda: kill file_count abuse

[Jan Harkes]

On Thu, Jul 19, 2007 at 11:45:08PM +0200, Christoph Hellwig wrote:
> ->release is the proper way to detect the last close of a file,
> file_count should never be used in filesystems.

Has been tried, the problem with that once ->release is called it is too
late to pass the the error back to close(2).

Jan

Re: [PATCH] coda: kill file_count abuse

[David Chinner]

On Thu, Jul 19, 2007 at 06:16:00PM -0400, Jan Harkes wrote:
> On Thu, Jul 19, 2007 at 11:45:08PM +0200, Christoph Hellwig wrote:
> > ->release is the proper way to detect the last close of a file,
> > file_count should never be used in filesystems.
> 
> Has been tried, the problem with that once ->release is called it is too
> late to pass the the error back to close(2).

I think you'll find the problem is that fput() throws away the error
from ->release, not that it's too late....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] coda: kill file_count abuse

[Al Viro]

On Fri, Jul 20, 2007 at 10:45:34AM +1000, David Chinner wrote:
> On Thu, Jul 19, 2007 at 06:16:00PM -0400, Jan Harkes wrote:
> > On Thu, Jul 19, 2007 at 11:45:08PM +0200, Christoph Hellwig wrote:
> > > ->release is the proper way to detect the last close of a file,
> > > file_count should never be used in filesystems.
> > 
> > Has been tried, the problem with that once ->release is called it is too
> > late to pass the the error back to close(2).
> 
> I think you'll find the problem is that fput() throws away the error
> from ->release, not that it's too late....

Just where would that return value go?

BTW, the reason why checks for struct file refcount blow is not far
from that:

task A: write()
task B (sharing descriptor table with A): close()
task C (with another reference to struct file in question): close()
task A: return from write()

Now, the final fput() here happens in write().  In particular, no
call of close(2) sees refcount equal to 1.

Re: [PATCH] coda: kill file_count abuse

[David Chinner]

On Fri, Jul 20, 2007 at 01:53:16AM +0100, Al Viro wrote:
> On Fri, Jul 20, 2007 at 10:45:34AM +1000, David Chinner wrote:
> > On Thu, Jul 19, 2007 at 06:16:00PM -0400, Jan Harkes wrote:
> > > On Thu, Jul 19, 2007 at 11:45:08PM +0200, Christoph Hellwig wrote:
> > > > ->release is the proper way to detect the last close of a file,
> > > > file_count should never be used in filesystems.
> > > 
> > > Has been tried, the problem with that once ->release is called it is too
> > > late to pass the the error back to close(2).
> > 
> > I think you'll find the problem is that fput() throws away the error
> > from ->release, not that it's too late....
> 
> Just where would that return value go?

To the context that dropped the last reference. It can't be
reported to anything else....

I'd much prefer to get an error on a file to any process
that was using the file than have a potential indication of
data or filesystem corruption silently dropped in the
bit bucket.....

> BTW, the reason why checks for struct file refcount blow is not far
> from that:
> 
> task A: write()
> task B (sharing descriptor table with A): close()
> task C (with another reference to struct file in question): close()
> task A: return from write()
> 
> Now, the final fput() here happens in write().  In particular, no
> call of close(2) sees refcount equal to 1.

Sure, but you've given no reason why the write() is unable to
or should not return the error.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] coda: kill file_count abuse

[Al Viro]

On Fri, Jul 20, 2007 at 12:36:01PM +1000, David Chinner wrote:
> To the context that dropped the last reference. It can't be
> reported to anything else....

Oh, for fsck sake...

Send a datagram with SCM_RIGHTS in it.  Have all other references
to these files closed.  Now have close(2) kill the socket that
might eventually be used to receive the datagram.  Now, all these
files are closed.  Tell me, which error value would you report
to that caller of close() and how would it guess which files had
those been?

Consider munmap().  It can release the last reference to any number
of files (mmap them in different places, then munmap the entire
area).  Which error would you report?

Consider exit(), for crying out loud.  You have a file opened.
You fork a child.  You close the file in parent.  Child goes
away.  Who do you report that stuff?

[PATCH] coda: file count cannot be used to discover last close

[Jan Harkes]

On Fri, Jul 20, 2007 at 04:16:31AM +0100, Al Viro wrote:
> On Fri, Jul 20, 2007 at 12:36:01PM +1000, David Chinner wrote:
> > To the context that dropped the last reference. It can't be
> > reported to anything else....
> 
> Oh, for fsck sake...

Here is a patch which removes the file_count test from coda_flush. This
avoids the race condition described by Al Viro where we may never see
the file_count drop to 0 in ->flush, in which case userspace is never
notified that a writeback should occur.

Signed-off-by: Jan Harkes <jaharkes@cs.cmu.edu>

---

Going from here I can fix the problem in several ways in userspace. The
current implementation would end up making useless snapshots as a result
of every close, and still leave us with lost updates in the the
write-after-close case, but that isn't any worse than what it is now.

Seeing every fops->flush does allow us to perform preliminary checks so
we can return errors for common issues and we can flag the file so that
the final release upcall (where we can no longer return errors) will
start the writeback.

Really the problem for me is that Coda's semantics (write on last close)
are not compatible with UNIX semantics, where writes may occur even
after the application has closed all fd's. This is quite similar to
having an open fd to a removed file, UNIX obviously doesn't return an
error or block the unlink if there is an active reference it just
detaches the inode from the namespace.

I will try to find a clean way to block the close syscall until fput
drops the last reference. However I realize that such an implementation
would not be acceptable for other file systems, and there are some
interesting unresolved details such as 'which close is going to be the
last close'.

diff --git a/fs/coda/file.c b/fs/coda/file.c
index 7594962..0a300dd 100644
--- a/fs/coda/file.c
+++ b/fs/coda/file.c
@@ -169,15 +169,10 @@ int coda_flush(struct file *coda_file, fl_owner_t id)
 	unsigned short coda_flags = coda_flags_to_cflags(flags);
 	struct coda_file_info *cfi;
 	struct inode *coda_inode;
-	int err = 0, fcnt;
+	int err = 0;
 
 	lock_kernel();
 
-	/* last close semantics */
-	fcnt = file_count(coda_file);
-	if (fcnt > 1)
-		goto out;
-
 	/* No need to make an upcall when we have not made any modifications
 	 * to the file */
 	if ((coda_file->f_flags & O_ACCMODE) == O_RDONLY)
@@ -246,7 +241,7 @@ int coda_release(struct inode *coda_inode, struct file *coda_file)
 	coda_file->private_data = NULL;
 
 	unlock_kernel();
-	return err;
+	return 0; /* no point in returning err, VFS ignores returned value */
 }
 
 int coda_fsync(struct file *coda_file, struct dentry *coda_dentry, int datasync)

Re: [PATCH] coda: file count cannot be used to discover last close

[Al Viro]

On Fri, Jul 20, 2007 at 12:10:00AM -0400, Jan Harkes wrote:
> I will try to find a clean way to block the close syscall until fput
> drops the last reference. However I realize that such an implementation
> would not be acceptable for other file systems, and there are some
> interesting unresolved details such as 'which close is going to be the
> last close'.

Simply impossible.

	fd = open(...);
	fd2 = dup(fd);
	....
	close(fd2);  <-- deadlock
	close(fd);

Or
	fd = open(...);
	p = mmap(..., fd, ...);
	close(fd);  <-- deadlock
	...
	munmap(p, ...);

The problem is that you are mixing two different operations:
	* closing descriptor: descriptor doesn't refer to this opened file
anymore.  Can be done by close(), can be done by dup2(), can be done by
fcntl() analog of dup2(), can be done by execve(), can be done by exit().
And can be done by death-by-signal.
	* closing opened file: no references (including descriptors) exist
anymore, no IO of any kind will be done on that IO channel.  Can happen
when descriptor gets closed, can happen when mapping gets destroyed (munmap,
etc.), can happen when SCM_RIGHTS datagram gets destroyed (including the
garbage collection), can happen when any syscall on that file finishes
after another thread has closed descriptor passed to that syscall, etc.

	You _can't_ expect the errors from the latter to be reliably
passed to some process; for one thing, many files can get closed by
single system call (e.g. closing an AF_UNIX socket with pending SCM_RIGHTS
datagrams will flush those datagrams, dropping references to files we
tried to pass via them).

	Why does CODA need special warranties in that area, anyway?
Related question: does fsync() force the writeback?

Re: [PATCH] coda: file count cannot be used to discover last close

[Jan Harkes]

On Fri, Jul 20, 2007 at 06:38:07AM +0100, Al Viro wrote:
> On Fri, Jul 20, 2007 at 12:10:00AM -0400, Jan Harkes wrote:
> > I will try to find a clean way to block the close syscall until fput
> > drops the last reference. However I realize that such an implementation
> > would not be acceptable for other file systems, and there are some
> > interesting unresolved details such as 'which close is going to be the
> > last close'.
> 
> Simply impossible.

As usual you are correct.

Originally Coda only used the CODA_CLOSE upcall which was called from
fops->release (fput). The problem was that we had various errors that
never managed to make it back to the user. So I split up the operation
into the part that does the write back and is the source of most errors.
(CODA_STORE, called from fops->flush) and the part that drops the last
reference (CODA_RELEASE, called from fops->release).

I've actually only used the _STORE/_RELEASE upcalls in a development
version, so the old code is not only still around, it is the typically
used variant.

I'll submit a patch that removes those upcalls, they will never work the
way I hoped.

> Why does CODA need special warranties in that area, anyway?

I don't think it really needs special warranties. The intent is to write
back data only when the last reference has been released. Ideally we
would like to return errors that occur during this writeback to the
application. Clearly the latter isn't possible.

> Related question: does fsync() force the writeback?

I think it should, but at the moment it does not. My guess on why the
implementation isn't there is that it mesh well with the last-writer
close model. The cache manager actually doesn't trigger any write back
operations until the owrite counter drops to 0 and this happens only
after all open for write descriptors for a file have been released. As a
result it was a bit of a hack to get the _STORE/_RELEASE variant to work
because it couldn't rely on the counter dropping to 0, which is one of
the reasons why I never released a Coda client that actually uses those
new upcalls.

We do have an upcall that is sent when fsync is called and could use
that. For fsync we could ignore that owrite counter. I am not sure if
the in-kernel implementation is correct, I would expect that it should
be an atomic operation wrt. other writers to be able to guarantee that
the resulting file actually contains what we expect but right now it
looks like it does grab the inode mutex when it syncs the file to disk
but releases the lock before it sends an upcall to the cache manager.

Jan

Re: [PATCH] coda: kill file_count abuse

[David Chinner]

On Fri, Jul 20, 2007 at 04:16:31AM +0100, Al Viro wrote:
> On Fri, Jul 20, 2007 at 12:36:01PM +1000, David Chinner wrote:
> > To the context that dropped the last reference. It can't be
> > reported to anything else....
> 
> Oh, for fsck sake...
> 
> Send a datagram with SCM_RIGHTS in it.  Have all other references
> to these files closed.  Now have close(2) kill the socket that
> might eventually be used to receive the datagram.  Now, all these
> files are closed.  Tell me, which error value would you report
> to that caller of close() and how would it guess which files had
> those been?
>
> Consider munmap().  It can release the last reference to any number
> of files (mmap them in different places, then munmap the entire
> area).  Which error would you report?
>
> Consider exit(), for crying out loud.  You have a file opened.
> You fork a child.  You close the file in parent.  Child goes
> away.  Who do you report that stuff?

Yup, all good cases where we can't report an error.

OTOH, consider a single thread doing an open(), write(), close().
That's a case where we could report an error, and it's far, far more
common than any of the scenarios you list above.

Effectively, that is my point - it's the fput() calling context
that knows whether it can return a meaningful error or not.
And ->release being non-void implies that you can return
meaningful errors to the caller.

So either fput() needs to propagate errors or ->release() should
be void, right?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH] coda: kill file_count abuse

[Jan Harkes]

On Fri, Jul 20, 2007 at 01:53:16AM +0100, Al Viro wrote:
> On Fri, Jul 20, 2007 at 10:45:34AM +1000, David Chinner wrote:
> > On Thu, Jul 19, 2007 at 06:16:00PM -0400, Jan Harkes wrote:
> > > On Thu, Jul 19, 2007 at 11:45:08PM +0200, Christoph Hellwig wrote:
> > > > ->release is the proper way to detect the last close of a file,
> > > > file_count should never be used in filesystems.
> > > 
> > > Has been tried, the problem with that once ->release is called it is too
> > > late to pass the the error back to close(2).
> > 
> > I think you'll find the problem is that fput() throws away the error
> > from ->release, not that it's too late....
>
> Just where would that return value go?

Right, there are actually quite a few places where different drivers and
file systems are returning non-zero errors from ->release. I just built
a kernel with the prototype changed to void to see how many places are
affected,  118 files changed, 211 insertions(+), 391 deletions(-)

That's with my default config on i386, so there are probably quite a few
more. The change also break the ABI quite badly (several EXPORT_SYMBOL
functions are affected) There were a few places where the error handling
path seems to lead to a possible struct file or private_data memory leak.

> BTW, the reason why checks for struct file refcount blow is not far
> from that:
> 
> task A: write()
> task B (sharing descriptor table with A): close()
> task C (with another reference to struct file in question): close()
> task A: return from write()
> 
> Now, the final fput() here happens in write().  In particular, no
> call of close(2) sees refcount equal to 1.

I see.

So if I want last-writer semantics, to let the last close trigger
writeback (upcall to userspace) combined with the ability to return an
error from the writeback back to close(2). To return an error I have to
use fops->flush, but as you've shown, I cannot reliably test if this
close dropped the last reference. In fact in your example there was
write activity even after the last close.

I guess I should find a way to delay the close (or at least the
userspace notification/return from syscall) until there are no active
writes.

Jan


=====
Some of the possibly suspect error handling cases I found while building
a kernel with void release() in fops, I haven't really looked at these
too deeply (i.e. only looked at the deallocation, not the allocations so
these may very well be harmless),

drivers/char/ipmi/ipmi_devintf.c: ipmi_release
    returns error when ipmi_destroy_user fails, does not call ipmi_fasync
    and seems to leak memory referenced by file->private_data

drivers/scsi/sg.c: sg_release
    may return ENXIO. leaks file->private_data when sfp->parentdp is NULL.

fs/autofs4/root.c: autofs4_dir_close
    returns EBUSY or ENOENT. In the case it returns busy it isn't
    releasing a struct file.

fs/bad_inode.c: bad_file_release
    returns EIO. Since the return code is ignored either way, there is
    no need to even implement this function.

fs/dlm/user.c: device_close
    may return ENOENT, doesn't free memory referenced by
    file->private_data if it fails this way.

fs/ecryptfs/file.c: ecryptfs_release
    returns error when it fails to close the lower file, it also seems
    to leak memory in this case.