linux-ext4 - Re: Question on fallocate/ftruncate sequence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <20090831215612.GG4197@webber.adilger.int>
Date:	Mon, 31 Aug 2009 15:56:12 -0600
From:	Andreas Dilger <adilger@....com>
To:	Jiaying Zhang <jiayingz@...gle.com>
Cc:	Theodore Tso <tytso@....edu>, Frank Mayhar <fmayhar@...gle.com>,
	Eric Sandeen <sandeen@...hat.com>,
	Curt Wohlgemuth <curtw@...gle.com>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: Question on fallocate/ftruncate sequence

On Aug 31, 2009  12:40 -0700, Jiaying Zhang wrote:
> > It's better to define the flag as EXT4_KEEPSIZE_FL, and to use it as
> > EXT4_KEEPSIZE_FL, but make a note of that bitfield position as being
> > reserved in include/linux/fs.h.
> 
> Here is the modified patch based on your suggestions. I stick with the
> KEEPSIZE_FL approach that I think can allow us to handle the special
> truncation accordingly during fsck. Other file systems can also re-use
> this flag when they want to support fallocate with KEEP_SIZE. As you
> suggested, I moved the EXT4_KEEPSIZE_FL checking to ext4_setattr
> that now calls vmtruncate if the KEEPSIZE flag is set in the i_flag.
> Please let me know what you think about this proposed patch.
> 
> --- .pc/fallocate_keepsizse.patch/fs/ext4/extents.c	2009-08-31
> 12:08:10.000000000 -0700
> +++ fs/ext4/extents.c	2009-08-31 12:12:16.000000000 -0700
> @@ -3095,7 +3095,13 @@ static void ext4_falloc_update_inode(str
>  			i_size_write(inode, new_size);
>  		if (new_size > EXT4_I(inode)->i_disksize)
>  			ext4_update_i_disksize(inode, new_size);
> +		inode->i_flags &= ~EXT4_KEEPSIZE_FL;

Note that fallocate can be called multiple times for a file.  The
EXT4_KEEPSIZE_FL should only be cleared if there were writes to
the end of the fallocated space.  In that regard, I think the name
of this flag should be changed to something like "EXT4_EOFBLOCKS_FL"
to indicate that blocks are allocated beyond the end of file (i_size).

>  	} else {
> +		/*
> +		 * Mark that we allocate beyond EOF so the subsequent truncate
> +		 * can proceed even if the new size is the same as i_size.
> +		 */
> +		inode->i_flags |= EXT4_KEEPSIZE_FL;

Similarly, this should only be done in case the fallocate is actually
beyond i_size.  While that is the most common case, it isn't necessarily
ALWAYS going to be true (e.g. if multiple threads are calling fallocate()
on a single file, or if a program always calls fallocate() on a file
without first checking if the file size is large enough).

> +++ include/linux/fs.h	2009-08-31 12:12:16.000000000 -0700
>  #define FS_DIRECTIO_FL			0x00100000 /* Use direct i/o */

> +++ fs/ext4/ext4.h	2009-08-31 12:12:16.000000000 -0700
>  #define EXT4_EXT_MIGRATE		0x00100000 /* Inode is migrating */

Should we redefine EXT4_EXT_MIGRATE not to conflict with FS_DIRECTIO_FL?
I don't think much, if any, use has been made of this flag, and I can
imagine a major headache in the future if this isn't changed now.

Also, EXT4_EXT_MIGRATE doesn't necessarily belong in the i_flags space,
since it is only used in-memory rather than on-disk as all of the others
are.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html