Re: [PATCH v3] Check for immutable/append flag in fallocate path

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 07, 2011 at 10:38:37PM -0700, Andreas Dilger wrote:
> On 2011-03-07, at 10:11 PM, Dave Chinner wrote:
> > On Sat, Mar 05, 2011 at 10:37:45AM +0100, Marco Stornelli wrote:
> >> From: Marco Stornelli <marco.stornelli@gmail.com>
> >> 
> >> In the fallocate path the kernel doesn't check for the immutable/append
> >> flag. It's possible to have a race condition in this scenario: an
> >> application open a file in read/write and it does something, meanwhile
> >> root set the immutable flag on the file, the application at that point
> >> can call fallocate with success. In addition, we don't allow to do any
> >> unreserve operation on an append only file but only the reserve one.
> >> 
> >> Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
> >> ---
> >> Patch is against 2.6.38-rc7
> >> 
> >> ChangeLog:
> >> v3: Modified do_fallocate instead of every single fs
> >> v2: Added the check for append-only file for XFS
> >> v1: First draft
> >> 
> >> --- open.c.orig	2011-03-01 22:55:12.000000000 +0100
> >> +++ open.c	2011-03-04 15:28:43.000000000 +0100
> >> @@ -233,6 +233,14 @@ int do_fallocate(struct file *file, int
> >> 
> >> 	if (!(file->f_mode & FMODE_WRITE))
> >> 		return -EBADF;
> >> +
> >> +	/* It's not possible punch hole on append only file */
> >> +	if (mode & FALLOC_FL_PUNCH_HOLE && IS_APPEND(inode))
> >> +		return -EPERM;
> > 
> > Seeing as I didn't get an answer in before you reposted, I still
> > think punching an append-only file is a valid thing to want to do.
> > 
> > I've seen this done in the past for application-level transaction
> > journal files. The journal file is append only so new transactions
> > can only be written at the end of the file i.e. you cannot overwrite
> > (and therefore corrupt) existing transactions. However, once a
> > transaction is complete and the changes flushed to disk, the
> > transaction is punched out of the file to zero the range so it
> > doesn't get replayed during recovery after a system crash.
> 
> To my thinking "append only" means just that - only new data can
> be written at the end of the file, and existing data cannot be
> modified.  Allowing hole punch on such a file (e.g. range 0 .. ~0)
> would allow erasing all of the data, entirely bypassing the
> append-only flag.

Not worth arguing over. XFS_IOC_UNRESVSP won't get changed, so the
applications already doing this can just keep using that interface...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com