Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Dr. Tilmann Bubeck]

There is a long discussion about booting GRUB2 from a partition 
containing ext2/3/4 (e.g. 
https://bugzilla.redhat.com/show_bug.cgi?id=872826 and many more on the 
net). This is typically a multiboot scenario with a primary boot loader 
in the MBR loading a second bootloader (here "GRUB2") from a partition.

Currently GRUB2 prints an error message when trying to install to a 
ext2/3/4 filesystem with:

"Attempting to install GRUB to a partition instead of the MBR.  This is 
a BAD idea. Embedding is not possible. GRUB can only be installed in 
this setup by using blocklists. However, blocklists are UNRELIABLE and 
their use is discouraged."

The basic problem is, that GRUB needs a safe place to store (currently 
32k) for its boot loader "core.img". That place should be simple to find 
from the primary boot code ("stage1") and the place should be safe for 
user intervention.

QUESTION:

You have reserved a special inode #5 called "EXT4_BOOT_LOADER_INO". Is 
this inode currently used or supported by kernel or user land? What is 
the idea of this inode?

PROPOSAL:

I can think of using that inode to store the file "core.img" of GRUB. 
That file is used by GRUB to boot and the block list of that file is 
stored in GRUB when using "--force" to override the above error.

ext2/3/4 must make sure, that the block list of that file never changes. 
I propose an additional EXT4 ioctl to tell ext4, which file to store in 
EXT4_BOOT_LOADER_INO.

Probably there must be more changes to e2fsck and friends.

Any ideas?

Till

-- 
+-------+-------------------------------------------------------------+
|       | dr. tilmann bubeck               reinform medien- und       |
|       |                                  informationstechnologie AG |
| rein  | fon  : +49 (711) 7 82 76-52      loeffelstr. 40             |
| form  | fax  : +49 (711) 7 82 76-46      70597 stuttgart / germany  |
|    AG | cell.: +49 (172) 8 84 29 72      fon: +49 (711) 75 86 56-10 |
|       | email: t.bubeck@reinform.de      http://www.reinform.de     |
|       +-------------------------------------------------------------+
|       | pflichtangaben nach paragraph 80, AktG:                     |
|       | reinform medien- und informationstechnologie AG, stuttgart  |
|       | handelsregister stuttgart, HRB 23001                        |
|       | vorstand:     dr. tilmann bubeck (vorsitz)                  |
|       | aufsichtsrat: frank stege (vorsitz)                         |
+-------+-------------------------------------------------------------+

Re: Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Theodore Ts'o]

On Fri, Jan 25, 2013 at 09:18:50AM +0100, Dr. Tilmann Bubeck wrote:
> The basic problem is, that GRUB needs a safe place to store
> (currently 32k) for its boot loader "core.img". That place should be
> simple to find from the primary boot code ("stage1") and the place
> should be safe for user intervention.
> 
> QUESTION:
> 
> You have reserved a special inode #5 called "EXT4_BOOT_LOADER_INO".
> Is this inode currently used or supported by kernel or user land?
> What is the idea of this inode?

It was basically for something exactly like this.  :-)

> PROPOSAL:
> 
> I can think of using that inode to store the file "core.img" of
> GRUB. That file is used by GRUB to boot and the block list of that
> file is stored in GRUB when using "--force" to override the above
> error.
> 
> ext2/3/4 must make sure, that the block list of that file never
> changes. I propose an additional EXT4 ioctl to tell ext4, which file
> to store in EXT4_BOOT_LOADER_INO.

What I'm thinking about is a new ioctl that would swap the i_block and
i_blocks array of the BOOT_LOADER_INO and the file descriptor.  That
is, if there were any blocks attached to the boot_loader_ino, they
would become attached to the inode associated with the file
descriptor, and the blocks associated with that inode would be
attached to inode #5.

> Probably there must be more changes to e2fsck and friends.

Actually, no changes to e2fsck would be necessary.  The original plan
was that boot loader inode would be installed while the file system is
unmounted.  But it's already the case that blocks associated with
inode <5> are already accounted for by e2fsck.

      	      	      		       - Ted

Re: Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/26/2013 01:49 PM, Theodore Ts'o wrote:
> What I'm thinking about is a new ioctl that would swap the i_block
> and i_blocks array of the BOOT_LOADER_INO and the file descriptor.
> That is, if there were any blocks attached to the boot_loader_ino,
> they would become attached to the inode associated with the file 
> descriptor, and the blocks associated with that inode would be 
> attached to inode #5.

Isn't that what the new defrag ioctl does already?  So you just need a
way to open the reserved and unnamed inode to call the ioctl on.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with undefined - http://www.enigmail.net/

iQEcBAEBAgAGBQJREbzMAAoJEJrBOlT6nu75KrwH/1tMG3rGEsXpIt61kPQDkH77
QDquiA31N57c8DxPVc8/ibZdNhCAjH84TZFW3Lopzr6zCh03ZbOs08AsEyDeXvWw
pTp3mJzmLvI1Lq7x6snYEA3ndd/taswo9bZwf2fwUwi2hQIBux+G65gnFECiybms
EV4zwUJSVtF8l77HTidrUODQgnI9j+MMwKILWtsgoBsGjMX5eHSO/5fgMCfnIvcv
q5MIU7V0JjaQVMTSAPOVXcy3ZAnvHv2rt+JNX5UyA1bST7BKd/uF9GQAxkakfLnH
oGeYzN9bRayGCszlcIeUMv0xv7J3KXFNB/MpKx42zlPyps92boThv1jLnkWrgOg=
=8PlP
-----END PGP SIGNATURE-----

Re: Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Dr. Tilmann Bubeck]

Ted,

my previous email contains a first implementation of your idea. Did I 
get it right or do you want me something to change?

I can also offer some shell scripts for mass testing the change to a 
fresh ext4 filesystem to ensure, that it does not break anything. I ran 
the script and indeed it does not break anything (as far as I can see).

Kind regards,
  Tilmann

Am 26.01.2013 19:49, schrieb Theodore Ts'o:
> On Fri, Jan 25, 2013 at 09:18:50AM +0100, Dr. Tilmann Bubeck wrote:
>> The basic problem is, that GRUB needs a safe place to store
>> (currently 32k) for its boot loader "core.img". That place should be
>> simple to find from the primary boot code ("stage1") and the place
>> should be safe for user intervention.
>>
>> QUESTION:
>>
>> You have reserved a special inode #5 called "EXT4_BOOT_LOADER_INO".
>> Is this inode currently used or supported by kernel or user land?
>> What is the idea of this inode?
>
> It was basically for something exactly like this.  :-)
>
>> PROPOSAL:
>>
>> I can think of using that inode to store the file "core.img" of
>> GRUB. That file is used by GRUB to boot and the block list of that
>> file is stored in GRUB when using "--force" to override the above
>> error.
>>
>> ext2/3/4 must make sure, that the block list of that file never
>> changes. I propose an additional EXT4 ioctl to tell ext4, which file
>> to store in EXT4_BOOT_LOADER_INO.
>
> What I'm thinking about is a new ioctl that would swap the i_block and
> i_blocks array of the BOOT_LOADER_INO and the file descriptor.  That
> is, if there were any blocks attached to the boot_loader_ino, they
> would become attached to the inode associated with the file
> descriptor, and the blocks associated with that inode would be
> attached to inode #5.
>
>> Probably there must be more changes to e2fsck and friends.
>
> Actually, no changes to e2fsck would be necessary.  The original plan
> was that boot loader inode would be installed while the file system is
> unmounted.  But it's already the case that blocks associated with
> inode <5> are already accounted for by e2fsck.
>
>        	      	      		       - Ted
>


-- 
+-------+-------------------------------------------------------------+
|       | dr. tilmann bubeck               reinform medien- und       |
|       |                                  informationstechnologie AG |
| rein  | fon  : +49 (711) 7 82 76-52      loeffelstr. 40             |
| form  | fax  : +49 (711) 7 82 76-46      70597 stuttgart / germany  |
|    AG | cell.: +49 (172) 8 84 29 72      fon: +49 (711) 75 86 56-10 |
|       | email: t.bubeck@reinform.de      http://www.reinform.de     |
|       +-------------------------------------------------------------+
|       | pflichtangaben nach paragraph 80, AktG:                     |
|       | reinform medien- und informationstechnologie AG, stuttgart  |
|       | handelsregister stuttgart, HRB 23001                        |
|       | vorstand:     dr. tilmann bubeck (vorsitz)                  |
|       | aufsichtsrat: frank stege (vorsitz)                         |
+-------+-------------------------------------------------------------+

Re: Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Dr. Tilmann Bubeck]

Ted,

I sent a patch implementing your idea from below at 2013-02-23 to the  
mailing list. It has received a

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Is there anything more to do to get this accepted? I am willing to do more  
cleanups, if you request them.

Thanks!
   Tilmann


On Sat, 26 Jan 2013 19:49:27 +0100, Theodore Ts'o <tytso@mit.edu> wrote:

> On Fri, Jan 25, 2013 at 09:18:50AM +0100, Dr. Tilmann Bubeck wrote:
>> The basic problem is, that GRUB needs a safe place to store
>> (currently 32k) for its boot loader "core.img". That place should be
>> simple to find from the primary boot code ("stage1") and the place
>> should be safe for user intervention.
>>
>> QUESTION:
>>
>> You have reserved a special inode #5 called "EXT4_BOOT_LOADER_INO".
>> Is this inode currently used or supported by kernel or user land?
>> What is the idea of this inode?
>
> It was basically for something exactly like this.  :-)
>
>> PROPOSAL:
>>
>> I can think of using that inode to store the file "core.img" of
>> GRUB. That file is used by GRUB to boot and the block list of that
>> file is stored in GRUB when using "--force" to override the above
>> error.
>>
>> ext2/3/4 must make sure, that the block list of that file never
>> changes. I propose an additional EXT4 ioctl to tell ext4, which file
>> to store in EXT4_BOOT_LOADER_INO.
>
> What I'm thinking about is a new ioctl that would swap the i_block and
> i_blocks array of the BOOT_LOADER_INO and the file descriptor.  That
> is, if there were any blocks attached to the boot_loader_ino, they
> would become attached to the inode associated with the file
> descriptor, and the blocks associated with that inode would be
> attached to inode #5.
>
>> Probably there must be more changes to e2fsck and friends.
>
> Actually, no changes to e2fsck would be necessary.  The original plan
> was that boot loader inode would be installed while the file system is
> unmounted.  But it's already the case that blocks associated with
> inode <5> are already accounted for by e2fsck.
>
>       	      	      		       - Ted


-- 

Mit freundlichen Gruessen,
   Tilmann Bubeck

+-------+-------------------------------------------------------------+
|       | dr. tilmann bubeck               reinform medien- und       |
|       |                                  informationstechnologie AG |
| rein  | fon  : +49 (711) 7 82 76-52      loeffelstr. 40             |
| form  | fax  : +49 (711) 7 82 76-46      70597 stuttgart / germany  |
|    AG | cell.: +49 (172) 8 84 29 72      fon: +49 (711) 75 86 56-10 |
|       | email: t.bubeck@reinform.de      http://www.reinform.de     |
|       +-------------------------------------------------------------+
|       | pflichtangaben nach paragraph 80, AktG:                     |
|       | reinform medien- und informationstechnologie AG, stuttgart  |
|       | handelsregister stuttgart, HRB 23001                        |
|       | vorstand:     dr. tilmann bubeck (vorsitz)                  |
|       | aufsichtsrat: frank stege (vorsitz)                         |
+-------+-------------------------------------------------------------+

Re: Use EXT4_BOOT_LOADER_INO for boot loader GRUB?

[Theodore Ts'o]

On Mon, Mar 18, 2013 at 02:26:25PM +0100, Dr. Tilmann Bubeck wrote:
> Ted,
> 
> I sent a patch implementing your idea from below at 2013-02-23 to
> the mailing list. It has received a
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
> Is there anything more to do to get this accepted? I am willing to
> do more cleanups, if you request them.

Sorry, I'm still working on stablizing and fixing bugs/regressions
from the last merge window, so I haven't started looking at patches
which introduce new features.  I'll hopefully get to it in the next
week or two....

							- Ted

[PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

[Theodore Ts'o]

Hi Tilmann,

I had to make a number of changes to your patches:

#1) If the boot inode is uninitialized, i_flags was set to zero, but
the i_data field was initialized via ext4_ext_tree_init().  As a
result, if you ran e2fsck on a freshly created file system after
running ext4-swap-boot-inode, e2fsck would complain about a corrupted
file system.  (Lesson to be learned: always run e2fsck afterwards
testing the code.  The file system _must_ be consistent afterwards).

#2) Unconditionally calling ext4_ext_tree_init() means the inode will
be initialized to use extents even if the file system does not support
extents (for example, if it is an ext2 or ext3 mapped file system).
This will also cause a file system that will fail an e2fsck for a file
system formatted as ext2 or ext3 but mounted using the ext4 file
system driver.

#3) The original version of the code called truncate_inode_pages()
after releasing i_mutex.  This violates a requirement which is very
clearly documented in the comments of that function.  (Lesson to be
learned: always check the implementation of functions that you're not
familiar with.  This is also why defensive programming such as
including mutex_is_locked() assertions in functions is useful, since
developers don't always read function documentation...)

#4) filemap_flush() needs to be called before you swap the inode data.
That's because the page cache is indexed by inode number and page
number.  So if there is dirty page data, it needs to be flushed to the
inode *before* the messing with the inode data.

#5) I needed to port the patch to deal with the extent status tree.
I'm guessing you probably based your patch off of the 3.7 version of
the kernel, and not the 3.8-rcX development tree.

Here's the modified patch; if someone else on the ext4 development
team could review the patch after my modifications, I'd appreciate it.

						- Ted

>From 3de49c9eeb1303b751b92b2f554c57c686e2997d Mon Sep 17 00:00:00 2001
From: "Dr. Tilmann Bubeck" <t.bubeck@reinform.de>
Date: Sat, 6 Apr 2013 22:44:18 -0400
Subject: [PATCH] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

Add a new ioctl, EXT4_IOC_SWAP_BOOT which swaps i_blocks and
associated attributes (like i_blocks, i_size, i_flags, ...) from
the associated inode with inode EXT4_BOOT_LOADER_INO (#5). This is
typically used to store a boot loader in a secure part of the
filesystem, where it can't be changed by the user on accident.  The
data blocks of the previous boot loader will be associated with the
given inode.

This usercode program is a simple example of the usage:

int main(int argc, char *argv[])
{
  int fd;
  int err;

  if ( argc != 2 ) {
    printf("usage: ext4-swap-boot-inode FILE-TO-SWAP\n");
    exit(1);
  }

  fd = open(argv[1], O_WRONLY);
  if ( fd < 0 ) {
    perror("open");
    exit(1);
  }

  err = ioctl(fd, EXT4_IOC_SWAP_BOOT);
  if ( err < 0 ) {
    perror("ioctl");
    exit(1);
  }

  close(fd);
  exit(0);
}

[ Modified by Theodore Ts'o to fix a number of bugs in the original code.]

Signed-off-by: Dr. Tilmann Bubeck <t.bubeck@reinform.de>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 Documentation/filesystems/ext4.txt |  10 ++
 fs/ext4/ext4.h                     |   8 ++
 fs/ext4/inode.c                    |  11 ++-
 fs/ext4/ioctl.c                    | 197 +++++++++++++++++++++++++++++++++++++
 fs/ext4/move_extent.c              |  48 ++++-----
 5 files changed, 248 insertions(+), 26 deletions(-)

diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
index 34ea4f1..69e83a4 100644
--- a/Documentation/filesystems/ext4.txt
+++ b/Documentation/filesystems/ext4.txt
@@ -587,6 +587,16 @@ Table of Ext4 specific ioctls
 			      bitmaps and inode table, the userspace tool thus
 			      just passes the new number of blocks.
 
+EXT4_IOC_SWAP_BOOT	      Swap i_blocks and associated attributes
+			      (like i_blocks, i_size, i_flags, ...) from
+			      the associated inode with inode
+			      EXT4_BOOT_LOADER_INO (#5). This is typically
+			      used to store a boot loader in a secure part of
+			      the filesystem, where it can't be changed by the
+			      user on accident.
+			      The data blocks of the previous boot loader
+			      will be associated with the given inode.
+
 ..............................................................................
 
 References
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a0637e5..d918715 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -616,6 +616,7 @@ enum {
 #define EXT4_IOC_ALLOC_DA_BLKS		_IO('f', 12)
 #define EXT4_IOC_MOVE_EXT		_IOWR('f', 15, struct move_extent)
 #define EXT4_IOC_RESIZE_FS		_IOW('f', 16, __u64)
+#define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
 
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /*
@@ -1341,6 +1342,7 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
 	return ino == EXT4_ROOT_INO ||
 		ino == EXT4_USR_QUOTA_INO ||
 		ino == EXT4_GRP_QUOTA_INO ||
+		ino == EXT4_BOOT_LOADER_INO ||
 		ino == EXT4_JOURNAL_INO ||
 		ino == EXT4_RESIZE_INO ||
 		(ino >= EXT4_FIRST_INO(sb) &&
@@ -2624,6 +2626,12 @@ extern int ext4_ind_migrate(struct inode *inode);
 
 
 /* move_extent.c */
+extern void ext4_double_down_write_data_sem(struct inode *first,
+					    struct inode *second);
+extern void ext4_double_up_write_data_sem(struct inode *orig_inode,
+					  struct inode *donor_inode);
+void ext4_inode_double_lock(struct inode *inode1, struct inode *inode2);
+void ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2);
 extern int ext4_move_extents(struct file *o_filp, struct file *d_filp,
 			     __u64 start_orig, __u64 start_donor,
 			     __u64 len, __u64 *moved_len);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 769c656..a29bfc2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4191,8 +4191,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	 * NeilBrown 1999oct15
 	 */
 	if (inode->i_nlink == 0) {
-		if (inode->i_mode == 0 ||
-		    !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
+		if ((inode->i_mode == 0 ||
+		     !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) &&
+		    ino != EXT4_BOOT_LOADER_INO) {
 			/* this inode is deleted */
 			ret = -ESTALE;
 			goto bad_inode;
@@ -4200,7 +4201,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 		/* The only unlinked inodes we let through here have
 		 * valid i_mode and are being read by the orphan
 		 * recovery code: that's fine, we're about to complete
-		 * the process of deleting those. */
+		 * the process of deleting those.
+		 * OR it is the EXT4_BOOT_LOADER_INO which is
+		 * not initialized on a new filesystem. */
 	}
 	ei->i_flags = le32_to_cpu(raw_inode->i_flags);
 	inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
@@ -4320,6 +4323,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 		else
 			init_special_inode(inode, inode->i_mode,
 			   new_decode_dev(le32_to_cpu(raw_inode->i_block[1])));
+	} else if (ino == EXT4_BOOT_LOADER_INO) {
+		make_bad_inode(inode);
 	} else {
 		ret = -EIO;
 		EXT4_ERROR_INODE(inode, "bogus i_mode (%o)", inode->i_mode);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a07b7bc..140a8d6 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -17,9 +17,201 @@
 #include <asm/uaccess.h>
 #include "ext4_jbd2.h"
 #include "ext4.h"
+#include "ext4_extents.h"
 
 #define MAX_32_NUM ((((unsigned long long) 1) << 32) - 1)
 
+/**
+ * Swap memory between @a and @b for @len bytes.
+ *
+ * @a:          pointer to first memory area
+ * @b:          pointer to second memory area
+ * @len:        number of bytes to swap
+ *
+ */
+static void memswap(void *a, void *b, size_t len)
+{
+	unsigned char *ap, *bp;
+	unsigned char tmp;
+
+	ap = (unsigned char *)a;
+	bp = (unsigned char *)b;
+	while (len-- > 0) {
+		tmp = *ap;
+		*ap = *bp;
+		*bp = tmp;
+		ap++;
+		bp++;
+	}
+}
+
+/**
+ * Swap i_data and associated attributes between inode1 and inode2.
+ * This function is used for the primary swap between inode1 and inode2
+ * and also to revert this primary swap in case of errors.
+ *
+ * Therefore you have to make sure, that calling this method twice
+ * will revert all changes.
+ *
+ * @inode1:     pointer to first inode
+ * @inode2:     pointer to second inode
+ */
+static void swap_inode_data(struct inode *inode1, struct inode *inode2)
+{
+	loff_t isize;
+	struct ext4_inode_info *ei1;
+	struct ext4_inode_info *ei2;
+
+	ei1 = EXT4_I(inode1);
+	ei2 = EXT4_I(inode2);
+
+	memswap(&inode1->i_flags, &inode2->i_flags, sizeof(inode1->i_flags));
+	memswap(&inode1->i_version, &inode2->i_version,
+		  sizeof(inode1->i_version));
+	memswap(&inode1->i_blocks, &inode2->i_blocks,
+		  sizeof(inode1->i_blocks));
+	memswap(&inode1->i_bytes, &inode2->i_bytes, sizeof(inode1->i_bytes));
+	memswap(&inode1->i_atime, &inode2->i_atime, sizeof(inode1->i_atime));
+	memswap(&inode1->i_mtime, &inode2->i_mtime, sizeof(inode1->i_mtime));
+
+	memswap(ei1->i_data, ei2->i_data, sizeof(ei1->i_data));
+	memswap(&ei1->i_flags, &ei2->i_flags, sizeof(ei1->i_flags));
+	memswap(&ei1->i_disksize, &ei2->i_disksize, sizeof(ei1->i_disksize));
+	memswap(&ei1->i_es_tree, &ei2->i_es_tree, sizeof(ei1->i_es_tree));
+	memswap(&ei1->i_es_lru_nr, &ei2->i_es_lru_nr, sizeof(ei1->i_es_lru_nr));
+
+	isize = i_size_read(inode1);
+	i_size_write(inode1, i_size_read(inode2));
+	i_size_write(inode2, isize);
+}
+
+/**
+ * Swap the information from the given @inode and the inode
+ * EXT4_BOOT_LOADER_INO. It will basically swap i_data and all other
+ * important fields of the inodes.
+ *
+ * @sb:         the super block of the filesystem
+ * @inode:      the inode to swap with EXT4_BOOT_LOADER_INO
+ *
+ */
+static long swap_inode_boot_loader(struct super_block *sb,
+				struct inode *inode)
+{
+	handle_t *handle;
+	int err;
+	struct inode *inode_bl;
+	struct ext4_inode_info *ei;
+	struct ext4_inode_info *ei_bl;
+	struct ext4_sb_info *sbi;
+
+	if (inode->i_nlink != 1 || !S_ISREG(inode->i_mode)) {
+		err = -EINVAL;
+		goto swap_boot_out;
+	}
+
+	if (!inode_owner_or_capable(inode) || !capable(CAP_SYS_ADMIN)) {
+		err = -EPERM;
+		goto swap_boot_out;
+	}
+
+	sbi = EXT4_SB(sb);
+	ei = EXT4_I(inode);
+
+	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO);
+	if (IS_ERR(inode_bl)) {
+		err = PTR_ERR(inode_bl);
+		goto swap_boot_out;
+	}
+	ei_bl = EXT4_I(inode_bl);
+
+	filemap_flush(inode->i_mapping);
+	filemap_flush(inode_bl->i_mapping);
+
+	/* Protect orig inodes against a truncate and make sure,
+	 * that only 1 swap_inode_boot_loader is running. */
+	ext4_inode_double_lock(inode, inode_bl);
+
+	truncate_inode_pages(&inode->i_data, 0);
+	truncate_inode_pages(&inode_bl->i_data, 0);
+
+	/* Wait for all existing dio workers */
+	ext4_inode_block_unlocked_dio(inode);
+	ext4_inode_block_unlocked_dio(inode_bl);
+	inode_dio_wait(inode);
+	inode_dio_wait(inode_bl);
+
+	handle = ext4_journal_start(inode_bl, EXT4_HT_MOVE_EXTENTS, 2);
+	if (IS_ERR(handle)) {
+		err = -EINVAL;
+		goto swap_boot_out;
+	}
+
+	/* Protect extent tree against block allocations via delalloc */
+	ext4_double_down_write_data_sem(inode, inode_bl);
+
+	if (inode_bl->i_nlink == 0) {
+		/* this inode has never been used as a BOOT_LOADER */
+		set_nlink(inode_bl, 1);
+		i_uid_write(inode_bl, 0);
+		i_gid_write(inode_bl, 0);
+		inode_bl->i_flags = 0;
+		ei_bl->i_flags = 0;
+		inode_bl->i_version = 1;
+		i_size_write(inode_bl, 0);
+		inode_bl->i_mode = S_IFREG;
+		if (EXT4_HAS_INCOMPAT_FEATURE(sb,
+					      EXT4_FEATURE_INCOMPAT_EXTENTS)) {
+			ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
+			ext4_ext_tree_init(handle, inode_bl);
+		} else
+			memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));
+	}
+
+	swap_inode_data(inode, inode_bl);
+
+	inode->i_ctime = inode_bl->i_ctime = ext4_current_time(inode);
+
+	spin_lock(&sbi->s_next_gen_lock);
+	inode->i_generation = sbi->s_next_generation++;
+	inode_bl->i_generation = sbi->s_next_generation++;
+	spin_unlock(&sbi->s_next_gen_lock);
+
+	ext4_discard_preallocations(inode);
+
+	err = ext4_mark_inode_dirty(handle, inode);
+	if (err < 0) {
+		ext4_warning(inode->i_sb,
+			"couldn't mark inode #%lu dirty (err %d)",
+			inode->i_ino, err);
+		/* Revert all changes: */
+		swap_inode_data(inode, inode_bl);
+	} else {
+		err = ext4_mark_inode_dirty(handle, inode_bl);
+		if (err < 0) {
+			ext4_warning(inode_bl->i_sb,
+				"couldn't mark inode #%lu dirty (err %d)",
+				inode_bl->i_ino, err);
+			/* Revert all changes: */
+			swap_inode_data(inode, inode_bl);
+			ext4_mark_inode_dirty(handle, inode);
+		}
+	}
+
+	ext4_journal_stop(handle);
+
+	ext4_double_up_write_data_sem(inode, inode_bl);
+
+	ext4_inode_resume_unlocked_dio(inode);
+	ext4_inode_resume_unlocked_dio(inode_bl);
+
+	ext4_inode_double_unlock(inode, inode_bl);
+
+	iput(inode_bl);
+
+swap_boot_out:
+	return err;
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -353,6 +545,11 @@ group_add_out:
 		return err;
 	}
 
+	case EXT4_IOC_SWAP_BOOT:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+		return swap_inode_boot_loader(sb, inode);
+
 	case EXT4_IOC_RESIZE_FS: {
 		ext4_fsblk_t n_blocks_count;
 		struct super_block *sb = inode->i_sb;
diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
index 33e1c08..a2e696e 100644
--- a/fs/ext4/move_extent.c
+++ b/fs/ext4/move_extent.c
@@ -144,12 +144,13 @@ mext_next_extent(struct inode *inode, struct ext4_ext_path *path,
 }
 
 /**
- * double_down_write_data_sem - Acquire two inodes' write lock of i_data_sem
+ * ext4_double_down_write_data_sem - Acquire two inodes' write lock
+ *                                   of i_data_sem
  *
  * Acquire write lock of i_data_sem of the two inodes
  */
-static void
-double_down_write_data_sem(struct inode *first, struct inode *second)
+void
+ext4_double_down_write_data_sem(struct inode *first, struct inode *second)
 {
 	if (first < second) {
 		down_write(&EXT4_I(first)->i_data_sem);
@@ -162,14 +163,15 @@ double_down_write_data_sem(struct inode *first, struct inode *second)
 }
 
 /**
- * double_up_write_data_sem - Release two inodes' write lock of i_data_sem
+ * ext4_double_up_write_data_sem - Release two inodes' write lock of i_data_sem
  *
  * @orig_inode:		original inode structure to be released its lock first
  * @donor_inode:	donor inode structure to be released its lock second
  * Release write lock of i_data_sem of two inodes (orig and donor).
  */
-static void
-double_up_write_data_sem(struct inode *orig_inode, struct inode *donor_inode)
+void
+ext4_double_up_write_data_sem(struct inode *orig_inode,
+			      struct inode *donor_inode)
 {
 	up_write(&EXT4_I(orig_inode)->i_data_sem);
 	up_write(&EXT4_I(donor_inode)->i_data_sem);
@@ -976,7 +978,7 @@ again:
 	 * necessary, just swap data blocks between orig and donor.
 	 */
 	if (uninit) {
-		double_down_write_data_sem(orig_inode, donor_inode);
+		ext4_double_down_write_data_sem(orig_inode, donor_inode);
 		/* If any of extents in range became initialized we have to
 		 * fallback to data copying */
 		uninit = mext_check_coverage(orig_inode, orig_blk_offset,
@@ -990,7 +992,7 @@ again:
 			goto drop_data_sem;
 
 		if (!uninit) {
-			double_up_write_data_sem(orig_inode, donor_inode);
+			ext4_double_up_write_data_sem(orig_inode, donor_inode);
 			goto data_copy;
 		}
 		if ((page_has_private(pagep[0]) &&
@@ -1004,7 +1006,7 @@ again:
 						donor_inode, orig_blk_offset,
 						block_len_in_page, err);
 	drop_data_sem:
-		double_up_write_data_sem(orig_inode, donor_inode);
+		ext4_double_up_write_data_sem(orig_inode, donor_inode);
 		goto unlock_pages;
 	}
 data_copy:
@@ -1065,11 +1067,11 @@ repair_branches:
 	 * Extents are swapped already, but we are not able to copy data.
 	 * Try to swap extents to it's original places
 	 */
-	double_down_write_data_sem(orig_inode, donor_inode);
+	ext4_double_down_write_data_sem(orig_inode, donor_inode);
 	replaced_count = mext_replace_branches(handle, donor_inode, orig_inode,
 					       orig_blk_offset,
 					       block_len_in_page, &err2);
-	double_up_write_data_sem(orig_inode, donor_inode);
+	ext4_double_up_write_data_sem(orig_inode, donor_inode);
 	if (replaced_count != block_len_in_page) {
 		EXT4_ERROR_INODE_BLOCK(orig_inode, (sector_t)(orig_blk_offset),
 				       "Unable to copy data block,"
@@ -1209,15 +1211,15 @@ mext_check_arguments(struct inode *orig_inode,
 }
 
 /**
- * mext_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
+ * ext4_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
  *
  * @inode1:	the inode structure
  * @inode2:	the inode structure
  *
  * Lock two inodes' i_mutex
  */
-static void
-mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
+void
+ext4_inode_double_lock(struct inode *inode1, struct inode *inode2)
 {
 	BUG_ON(inode1 == inode2);
 	if (inode1 < inode2) {
@@ -1230,15 +1232,15 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
 }
 
 /**
- * mext_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
+ * ext4_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
  *
  * @inode1:     the inode that is released first
  * @inode2:     the inode that is released second
  *
  */
 
-static void
-mext_inode_double_unlock(struct inode *inode1, struct inode *inode2)
+void
+ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2)
 {
 	mutex_unlock(&inode1->i_mutex);
 	mutex_unlock(&inode2->i_mutex);
@@ -1333,7 +1335,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 		return -EINVAL;
 	}
 	/* Protect orig and donor inodes against a truncate */
-	mext_inode_double_lock(orig_inode, donor_inode);
+	ext4_inode_double_lock(orig_inode, donor_inode);
 
 	/* Wait for all existing dio workers */
 	ext4_inode_block_unlocked_dio(orig_inode);
@@ -1342,7 +1344,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 	inode_dio_wait(donor_inode);
 
 	/* Protect extent tree against block allocations via delalloc */
-	double_down_write_data_sem(orig_inode, donor_inode);
+	ext4_double_down_write_data_sem(orig_inode, donor_inode);
 	/* Check the filesystem environment whether move_extent can be done */
 	ret = mext_check_arguments(orig_inode, donor_inode, orig_start,
 				    donor_start, &len);
@@ -1466,7 +1468,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 		 * b. racing with ->readpage, ->write_begin, and ext4_get_block
 		 *    in move_extent_per_page
 		 */
-		double_up_write_data_sem(orig_inode, donor_inode);
+		ext4_double_up_write_data_sem(orig_inode, donor_inode);
 
 		while (orig_page_offset <= seq_end_page) {
 
@@ -1500,7 +1502,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 				block_len_in_page = rest_blocks;
 		}
 
-		double_down_write_data_sem(orig_inode, donor_inode);
+		ext4_double_down_write_data_sem(orig_inode, donor_inode);
 		if (ret < 0)
 			break;
 
@@ -1538,10 +1540,10 @@ out:
 		ext4_ext_drop_refs(holecheck_path);
 		kfree(holecheck_path);
 	}
-	double_up_write_data_sem(orig_inode, donor_inode);
+	ext4_double_up_write_data_sem(orig_inode, donor_inode);
 	ext4_inode_resume_unlocked_dio(orig_inode);
 	ext4_inode_resume_unlocked_dio(donor_inode);
-	mext_inode_double_unlock(orig_inode, donor_inode);
+	ext4_inode_double_unlock(orig_inode, donor_inode);
 
 	return ret;
 }
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

[Andreas Dilger]

On 2013-04-06, at 20:47, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> I had to make a number of changes to your patches:
> 
> #1) If the boot inode is uninitialized, i_flags was set to zero, but
> the i_data field was initialized via ext4_ext_tree_init().  As a
> result, if you ran e2fsck on a freshly created file system after
> running ext4-swap-boot-inode, e2fsck would complain about a corrupted
> file system.  (Lesson to be learned: always run e2fsck afterwards
> testing the code.  The file system _must_ be consistent afterwards).
> 
> #2) Unconditionally calling ext4_ext_tree_init() means the inode will
> be initialized to use extents even if the file system does not support
> extents (for example, if it is an ext2 or ext3 mapped file system).
> This will also cause a file system that will fail an e2fsck for a file
> system formatted as ext2 or ext3 but mounted using the ext4 file
> system driver.
> 
> #3) The original version of the code called truncate_inode_pages()
> after releasing i_mutex.  This violates a requirement which is very
> clearly documented in the comments of that function.  (Lesson to be
> learned: always check the implementation of functions that you're not
> familiar with.  This is also why defensive programming such as
> including mutex_is_locked() assertions in functions is useful, since
> developers don't always read function documentation...)
> 
> #4) filemap_flush() needs to be called before you swap the inode data.
> That's because the page cache is indexed by inode number and page
> number.  So if there is dirty page data, it needs to be flushed to the
> inode *before* the messing with the inode data.
> 
> #5) I needed to port the patch to deal with the extent status tree.
> I'm guessing you probably based your patch off of the 3.7 version of
> the kernel, and not the 3.8-rcX development tree.
> 
> Here's the modified patch; if someone else on the ext4 development
> team could review the patch after my modifications, I'd appreciate it.
> 
>                        - Ted
> 
> From 3de49c9eeb1303b751b92b2f554c57c686e2997d Mon Sep 17 00:00:00 2001
> From: "Dr. Tilmann Bubeck" <t.bubeck@reinform.de>
> Date: Sat, 6 Apr 2013 22:44:18 -0400
> Subject: [PATCH] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT
> 
> Add a new ioctl, EXT4_IOC_SWAP_BOOT which swaps i_blocks and
> associated attributes (like i_blocks, i_size, i_flags, ...) from
> the associated inode with inode EXT4_BOOT_LOADER_INO (#5). This is
> typically used to store a boot loader in a secure part of the
> filesystem, where it can't be changed by the user on accident.  The

s/on accident/by accident/

> data blocks of the previous boot loader will be associated with the
> given inode.
> 
> This usercode program is a simple example of the usage:
> 
> int main(int argc, char *argv[])
> {
>  int fd;
>  int err;
> 
>  if ( argc != 2 ) {
>    printf("usage: ext4-swap-boot-inode FILE-TO-SWAP\n");
>    exit(1);
>  }
> 
>  fd = open(argv[1], O_WRONLY);
>  if ( fd < 0 ) {
>    perror("open");
>    exit(1);
>  }
> 
>  err = ioctl(fd, EXT4_IOC_SWAP_BOOT);
>  if ( err < 0 ) {
>    perror("ioctl");
>    exit(1);
>  }
> 
>  close(fd);
>  exit(0);
> }
> 
> [ Modified by Theodore Ts'o to fix a number of bugs in the original code.]
> 
> Signed-off-by: Dr. Tilmann Bubeck <t.bubeck@reinform.de>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
> Documentation/filesystems/ext4.txt |  10 ++
> fs/ext4/ext4.h                     |   8 ++
> fs/ext4/inode.c                    |  11 ++-
> fs/ext4/ioctl.c                    | 197 +++++++++++++++++++++++++++++++++++++
> fs/ext4/move_extent.c              |  48 ++++-----
> 5 files changed, 248 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
> index 34ea4f1..69e83a4 100644
> --- a/Documentation/filesystems/ext4.txt
> +++ b/Documentation/filesystems/ext4.txt
> @@ -587,6 +587,16 @@ Table of Ext4 specific ioctls
>                  bitmaps and inode table, the userspace tool thus
>                  just passes the new number of blocks.
> 
> +EXT4_IOC_SWAP_BOOT          Swap i_blocks and associated attributes
> +                  (like i_blocks, i_size, i_flags, ...) from
> +                  the associated inode with inode

s/associated inode/specified inode/ or similar 

> +                  EXT4_BOOT_LOADER_INO (#5). This is typically
> +                  used to store a boot loader in a secure part of
> +                  the filesystem, where it can't be changed by the

s/the user/a normal user/

> +                  user on accident.

s/on/by/

> +                  The data blocks of the previous boot loader
> +                  will be associated with the given inode.
> +
> ..............................................................................
> 
> References
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a0637e5..d918715 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -616,6 +616,7 @@ enum {
> #define EXT4_IOC_ALLOC_DA_BLKS        _IO('f', 12)
> #define EXT4_IOC_MOVE_EXT        _IOWR('f', 15, struct move_extent)
> #define EXT4_IOC_RESIZE_FS        _IOW('f', 16, __u64)
> +#define EXT4_IOC_SWAP_BOOT        _IO('f', 17)
> 
> #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
> /*
> @@ -1341,6 +1342,7 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
>    return ino == EXT4_ROOT_INO ||
>        ino == EXT4_USR_QUOTA_INO ||
>        ino == EXT4_GRP_QUOTA_INO ||
> +        ino == EXT4_BOOT_LOADER_INO ||
>        ino == EXT4_JOURNAL_INO ||
>        ino == EXT4_RESIZE_INO ||
>        (ino >= EXT4_FIRST_INO(sb) &&
> @@ -2624,6 +2626,12 @@ extern int ext4_ind_migrate(struct inode *inode);
> 
> 
> /* move_extent.c */
> +extern void ext4_double_down_write_data_sem(struct inode *first,
> +                        struct inode *second);
> +extern void ext4_double_up_write_data_sem(struct inode *orig_inode,
> +                      struct inode *donor_inode);
> +void ext4_inode_double_lock(struct inode *inode1, struct inode *inode2);
> +void ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2);
> extern int ext4_move_extents(struct file *o_filp, struct file *d_filp,
>                 __u64 start_orig, __u64 start_donor,
>                 __u64 len, __u64 *moved_len);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 769c656..a29bfc2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4191,8 +4191,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>     * NeilBrown 1999oct15
>     */
>    if (inode->i_nlink == 0) {
> -        if (inode->i_mode == 0 ||
> -            !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
> +        if ((inode->i_mode == 0 ||
> +             !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) &&
> +            ino != EXT4_BOOT_LOADER_INO) {
>            /* this inode is deleted */
>            ret = -ESTALE;
>            goto bad_inode;
> @@ -4200,7 +4201,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>        /* The only unlinked inodes we let through here have
>         * valid i_mode and are being read by the orphan
>         * recovery code: that's fine, we're about to complete
> -         * the process of deleting those. */
> +         * the process of deleting those.
> +         * OR it is the EXT4_BOOT_LOADER_INO which is
> +         * not initialized on a new filesystem. */
>    }
>    ei->i_flags = le32_to_cpu(raw_inode->i_flags);
>    inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
> @@ -4320,6 +4323,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>        else
>            init_special_inode(inode, inode->i_mode,
>               new_decode_dev(le32_to_cpu(raw_inode->i_block[1])));
> +    } else if (ino == EXT4_BOOT_LOADER_INO) {
> +        make_bad_inode(inode);
>    } else {
>        ret = -EIO;
>        EXT4_ERROR_INODE(inode, "bogus i_mode (%o)", inode->i_mode);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a07b7bc..140a8d6 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -17,9 +17,201 @@
> #include <asm/uaccess.h>
> #include "ext4_jbd2.h"
> #include "ext4.h"
> +#include "ext4_extents.h"
> 
> #define MAX_32_NUM ((((unsigned long long) 1) << 32) - 1)
> 
> +/**
> + * Swap memory between @a and @b for @len bytes.
> + *
> + * @a:          pointer to first memory area
> + * @b:          pointer to second memory area
> + * @len:        number of bytes to swap
> + *
> + */
> +static void memswap(void *a, void *b, size_t len)
> +{
> +    unsigned char *ap, *bp;
> +    unsigned char tmp;
> +
> +    ap = (unsigned char *)a;
> +    bp = (unsigned char *)b;
> +    while (len-- > 0) {
> +        tmp = *ap;
> +        *ap = *bp;
> +        *bp = tmp;
> +        ap++;
> +        bp++;
> +    }
> +}
> +
> +/**
> + * Swap i_data and associated attributes between inode1 and inode2.

Should this be @inode1 and @inode2?

> + * This function is used for the primary swap between inode1 and inode2
> + * and also to revert this primary swap in case of errors.
> + *
> + * Therefore you have to make sure, that calling this method twice
> + * will revert all changes.
> + *
> + * @inode1:     pointer to first inode
> + * @inode2:     pointer to second inode
> + */
> +static void swap_inode_data(struct inode *inode1, struct inode *inode2)
> +{
> +    loff_t isize;
> +    struct ext4_inode_info *ei1;
> +    struct ext4_inode_info *ei2;
> +
> +    ei1 = EXT4_I(inode1);
> +    ei2 = EXT4_I(inode2);
> +
> +    memswap(&inode1->i_flags, &inode2->i_flags, sizeof(inode1->i_flags));
> +    memswap(&inode1->i_version, &inode2->i_version,
> +          sizeof(inode1->i_version));
> +    memswap(&inode1->i_blocks, &inode2->i_blocks,
> +          sizeof(inode1->i_blocks));
> +    memswap(&inode1->i_bytes, &inode2->i_bytes, sizeof(inode1->i_bytes));
> +    memswap(&inode1->i_atime, &inode2->i_atime, sizeof(inode1->i_atime));
> +    memswap(&inode1->i_mtime, &inode2->i_mtime, sizeof(inode1->i_mtime));
> +
> +    memswap(ei1->i_data, ei2->i_data, sizeof(ei1->i_data));
> +    memswap(&ei1->i_flags, &ei2->i_flags, sizeof(ei1->i_flags));
> +    memswap(&ei1->i_disksize, &ei2->i_disksize, sizeof(ei1->i_disksize));
> +    memswap(&ei1->i_es_tree, &ei2->i_es_tree, sizeof(ei1->i_es_tree));
> +    memswap(&ei1->i_es_lru_nr, &ei2->i_es_lru_nr, sizeof(ei1->i_es_lru_nr));
> +
> +    isize = i_size_read(inode1);
> +    i_size_write(inode1, i_size_read(inode2));
> +    i_size_write(inode2, isize);
> +}
> +
> +/**
> + * Swap the information from the given @inode and the inode
> + * EXT4_BOOT_LOADER_INO. It will basically swap i_data and all other
> + * important fields of the inodes.
> + *
> + * @sb:         the super block of the filesystem
> + * @inode:      the inode to swap with EXT4_BOOT_LOADER_INO
> + *
> + */
> +static long swap_inode_boot_loader(struct super_block *sb,
> +                struct inode *inode)
> +{
> +    handle_t *handle;
> +    int err;
> +    struct inode *inode_bl;
> +    struct ext4_inode_info *ei;
> +    struct ext4_inode_info *ei_bl;
> +    struct ext4_sb_info *sbi;
> +
> +    if (inode->i_nlink != 1 || !S_ISREG(inode->i_mode)) {
> +        err = -EINVAL;
> +        goto swap_boot_out;
> +    }
> +
> +    if (!inode_owner_or_capable(inode) || !capable(CAP_SYS_ADMIN)) {
> +        err = -EPERM;
> +        goto swap_boot_out;
> +    }
> +
> +    sbi = EXT4_SB(sb);
> +    ei = EXT4_I(inode);
> +
> +    inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO);
> +    if (IS_ERR(inode_bl)) {
> +        err = PTR_ERR(inode_bl);
> +        goto swap_boot_out;
> +    }
> +    ei_bl = EXT4_I(inode_bl);
> +
> +    filemap_flush(inode->i_mapping);
> +    filemap_flush(inode_bl->i_mapping);

Technically, it shouldn't be possible to have dirty pages on the bootloader inode, since any inode swapped there will itself have been flushed, and should not be directly accessible after that point. That said, I guess this ioctl() won't be called too often so the empty flush is mostly harmless. 

> +    /* Protect orig inodes against a truncate and make sure,
> +     * that only 1 swap_inode_boot_loader is running. */
> +    ext4_inode_double_lock(inode, inode_bl);
> +
> +    truncate_inode_pages(&inode->i_data, 0);
> +    truncate_inode_pages(&inode_bl->i_data, 0);

Ditto, hmm unless there are read pages from boot time?  No, I guess Grub and friends will not have cached data in the kernel. 

> +    /* Wait for all existing dio workers */
> +    ext4_inode_block_unlocked_dio(inode);
> +    ext4_inode_block_unlocked_dio(inode_bl);

Ditto. 

> +    inode_dio_wait(inode);
> +    inode_dio_wait(inode_bl);

Ditto. 

> +    handle = ext4_journal_start(inode_bl, EXT4_HT_MOVE_EXTENTS, 2);
> +    if (IS_ERR(handle)) {
> +        err = -EINVAL;
> +        goto swap_boot_out;
> +    }
> +
> +    /* Protect extent tree against block allocations via delalloc */
> +    ext4_double_down_write_data_sem(inode, inode_bl);
> +
> +    if (inode_bl->i_nlink == 0) {
> +        /* this inode has never been used as a BOOT_LOADER */
> +        set_nlink(inode_bl, 1);
> +        i_uid_write(inode_bl, 0);
> +        i_gid_write(inode_bl, 0);
> +        inode_bl->i_flags = 0;
> +        ei_bl->i_flags = 0;
> +        inode_bl->i_version = 1;
> +        i_size_write(inode_bl, 0);
> +        inode_bl->i_mode = S_IFREG;
> +        if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> +                          EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> +            ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
> +            ext4_ext_tree_init(handle, inode_bl);
> +        } else
> +            memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));

I don't understand this. Wouldn't this clobber the block pointers if an existing boot inode and cause them to leak?  This seems broken to me. 

Also, style wise it is better to wrap the "else" clause in braces when the "if" clause uses them. 

> +    }
> +
> +    swap_inode_data(inode, inode_bl);
> +
> +    inode->i_ctime = inode_bl->i_ctime = ext4_current_time(inode);
> +
> +    spin_lock(&sbi->s_next_gen_lock);
> +    inode->i_generation = sbi->s_next_generation++;
> +    inode_bl->i_generation = sbi->s_next_generation++;
> +    spin_unlock(&sbi->s_next_gen_lock);
> +
> +    ext4_discard_preallocations(inode);
> +
> +    err = ext4_mark_inode_dirty(handle, inode);
> +    if (err < 0) {
> +        ext4_warning(inode->i_sb,
> +            "couldn't mark inode #%lu dirty (err %d)",
> +            inode->i_ino, err);
> +        /* Revert all changes: */
> +        swap_inode_data(inode, inode_bl);
> +    } else {
> +        err = ext4_mark_inode_dirty(handle, inode_bl);
> +        if (err < 0) {
> +            ext4_warning(inode_bl->i_sb,
> +                "couldn't mark inode #%lu dirty (err %d)",
> +                inode_bl->i_ino, err);
> +            /* Revert all changes: */
> +            swap_inode_data(inode, inode_bl);
> +            ext4_mark_inode_dirty(handle, inode);
> +        }
> +    }
> +
> +    ext4_journal_stop(handle);
> +
> +    ext4_double_up_write_data_sem(inode, inode_bl);
> +
> +    ext4_inode_resume_unlocked_dio(inode);
> +    ext4_inode_resume_unlocked_dio(inode_bl);

Again, we shouldn't ever have IO inflight against the boot inode, AFAICS. 

Cheers, Andreas

> +
> +    ext4_inode_double_unlock(inode, inode_bl);
> +
> +    iput(inode_bl);
> +
> +swap_boot_out:
> +    return err;
> +}
> +
> long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
>    struct inode *inode = file_inode(filp);
> @@ -353,6 +545,11 @@ group_add_out:
>        return err;
>    }
> 
> +    case EXT4_IOC_SWAP_BOOT:
> +        if (!(filp->f_mode & FMODE_WRITE))
> +            return -EBADF;
> +        return swap_inode_boot_loader(sb, inode);
> +
>    case EXT4_IOC_RESIZE_FS: {
>        ext4_fsblk_t n_blocks_count;
>        struct super_block *sb = inode->i_sb;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 33e1c08..a2e696e 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -144,12 +144,13 @@ mext_next_extent(struct inode *inode, struct ext4_ext_path *path,
> }
> 
> /**
> - * double_down_write_data_sem - Acquire two inodes' write lock of i_data_sem
> + * ext4_double_down_write_data_sem - Acquire two inodes' write lock
> + *                                   of i_data_sem
>  *
>  * Acquire write lock of i_data_sem of the two inodes
>  */
> -static void
> -double_down_write_data_sem(struct inode *first, struct inode *second)
> +void
> +ext4_double_down_write_data_sem(struct inode *first, struct inode *second)
> {
>    if (first < second) {
>        down_write(&EXT4_I(first)->i_data_sem);
> @@ -162,14 +163,15 @@ double_down_write_data_sem(struct inode *first, struct inode *second)
> }
> 
> /**
> - * double_up_write_data_sem - Release two inodes' write lock of i_data_sem
> + * ext4_double_up_write_data_sem - Release two inodes' write lock of i_data_sem
>  *
>  * @orig_inode:        original inode structure to be released its lock first
>  * @donor_inode:    donor inode structure to be released its lock second
>  * Release write lock of i_data_sem of two inodes (orig and donor).
>  */
> -static void
> -double_up_write_data_sem(struct inode *orig_inode, struct inode *donor_inode)
> +void
> +ext4_double_up_write_data_sem(struct inode *orig_inode,
> +                  struct inode *donor_inode)
> {
>    up_write(&EXT4_I(orig_inode)->i_data_sem);
>    up_write(&EXT4_I(donor_inode)->i_data_sem);
> @@ -976,7 +978,7 @@ again:
>     * necessary, just swap data blocks between orig and donor.
>     */
>    if (uninit) {
> -        double_down_write_data_sem(orig_inode, donor_inode);
> +        ext4_double_down_write_data_sem(orig_inode, donor_inode);
>        /* If any of extents in range became initialized we have to
>         * fallback to data copying */
>        uninit = mext_check_coverage(orig_inode, orig_blk_offset,
> @@ -990,7 +992,7 @@ again:
>            goto drop_data_sem;
> 
>        if (!uninit) {
> -            double_up_write_data_sem(orig_inode, donor_inode);
> +            ext4_double_up_write_data_sem(orig_inode, donor_inode);
>            goto data_copy;
>        }
>        if ((page_has_private(pagep[0]) &&
> @@ -1004,7 +1006,7 @@ again:
>                        donor_inode, orig_blk_offset,
>                        block_len_in_page, err);
>    drop_data_sem:
> -        double_up_write_data_sem(orig_inode, donor_inode);
> +        ext4_double_up_write_data_sem(orig_inode, donor_inode);
>        goto unlock_pages;
>    }
> data_copy:
> @@ -1065,11 +1067,11 @@ repair_branches:
>     * Extents are swapped already, but we are not able to copy data.
>     * Try to swap extents to it's original places
>     */
> -    double_down_write_data_sem(orig_inode, donor_inode);
> +    ext4_double_down_write_data_sem(orig_inode, donor_inode);
>    replaced_count = mext_replace_branches(handle, donor_inode, orig_inode,
>                           orig_blk_offset,
>                           block_len_in_page, &err2);
> -    double_up_write_data_sem(orig_inode, donor_inode);
> +    ext4_double_up_write_data_sem(orig_inode, donor_inode);
>    if (replaced_count != block_len_in_page) {
>        EXT4_ERROR_INODE_BLOCK(orig_inode, (sector_t)(orig_blk_offset),
>                       "Unable to copy data block,"
> @@ -1209,15 +1211,15 @@ mext_check_arguments(struct inode *orig_inode,
> }
> 
> /**
> - * mext_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
> + * ext4_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
>  *
>  * @inode1:    the inode structure
>  * @inode2:    the inode structure
>  *
>  * Lock two inodes' i_mutex
>  */
> -static void
> -mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
> +void
> +ext4_inode_double_lock(struct inode *inode1, struct inode *inode2)
> {
>    BUG_ON(inode1 == inode2);
>    if (inode1 < inode2) {
> @@ -1230,15 +1232,15 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
> }
> 
> /**
> - * mext_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
> + * ext4_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
>  *
>  * @inode1:     the inode that is released first
>  * @inode2:     the inode that is released second
>  *
>  */
> 
> -static void
> -mext_inode_double_unlock(struct inode *inode1, struct inode *inode2)
> +void
> +ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2)
> {
>    mutex_unlock(&inode1->i_mutex);
>    mutex_unlock(&inode2->i_mutex);
> @@ -1333,7 +1335,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>        return -EINVAL;
>    }
>    /* Protect orig and donor inodes against a truncate */
> -    mext_inode_double_lock(orig_inode, donor_inode);
> +    ext4_inode_double_lock(orig_inode, donor_inode);
> 
>    /* Wait for all existing dio workers */
>    ext4_inode_block_unlocked_dio(orig_inode);
> @@ -1342,7 +1344,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>    inode_dio_wait(donor_inode);
> 
>    /* Protect extent tree against block allocations via delalloc */
> -    double_down_write_data_sem(orig_inode, donor_inode);
> +    ext4_double_down_write_data_sem(orig_inode, donor_inode);
>    /* Check the filesystem environment whether move_extent can be done */
>    ret = mext_check_arguments(orig_inode, donor_inode, orig_start,
>                    donor_start, &len);
> @@ -1466,7 +1468,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>         * b. racing with ->readpage, ->write_begin, and ext4_get_block
>         *    in move_extent_per_page
>         */
> -        double_up_write_data_sem(orig_inode, donor_inode);
> +        ext4_double_up_write_data_sem(orig_inode, donor_inode);
> 
>        while (orig_page_offset <= seq_end_page) {
> 
> @@ -1500,7 +1502,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>                block_len_in_page = rest_blocks;
>        }
> 
> -        double_down_write_data_sem(orig_inode, donor_inode);
> +        ext4_double_down_write_data_sem(orig_inode, donor_inode);
>        if (ret < 0)
>            break;
> 
> @@ -1538,10 +1540,10 @@ out:
>        ext4_ext_drop_refs(holecheck_path);
>        kfree(holecheck_path);
>    }
> -    double_up_write_data_sem(orig_inode, donor_inode);
> +    ext4_double_up_write_data_sem(orig_inode, donor_inode);
>    ext4_inode_resume_unlocked_dio(orig_inode);
>    ext4_inode_resume_unlocked_dio(donor_inode);
> -    mext_inode_double_unlock(orig_inode, donor_inode);
> +    ext4_inode_double_unlock(orig_inode, donor_inode);
> 
>    return ret;
> }
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

[Dr. Tilmann Bubeck]

Hi Ted,

thanks for improving my patch and thanks a lot for the lessons! I 
appreciate this really.

Feel free to improve the patch, it is getting better with every release.

Thanks!
  Tilmann

Am 07.04.2013 04:47, schrieb Theodore Ts'o:
> Hi Tilmann,
>
> I had to make a number of changes to your patches:
>
> #1) If the boot inode is uninitialized, i_flags was set to zero, but
> the i_data field was initialized via ext4_ext_tree_init().  As a
> result, if you ran e2fsck on a freshly created file system after
> running ext4-swap-boot-inode, e2fsck would complain about a corrupted
> file system.  (Lesson to be learned: always run e2fsck afterwards
> testing the code.  The file system _must_ be consistent afterwards).
>
> #2) Unconditionally calling ext4_ext_tree_init() means the inode will
> be initialized to use extents even if the file system does not support
> extents (for example, if it is an ext2 or ext3 mapped file system).
> This will also cause a file system that will fail an e2fsck for a file
> system formatted as ext2 or ext3 but mounted using the ext4 file
> system driver.
>
> #3) The original version of the code called truncate_inode_pages()
> after releasing i_mutex.  This violates a requirement which is very
> clearly documented in the comments of that function.  (Lesson to be
> learned: always check the implementation of functions that you're not
> familiar with.  This is also why defensive programming such as
> including mutex_is_locked() assertions in functions is useful, since
> developers don't always read function documentation...)
>
> #4) filemap_flush() needs to be called before you swap the inode data.
> That's because the page cache is indexed by inode number and page
> number.  So if there is dirty page data, it needs to be flushed to the
> inode *before* the messing with the inode data.
>
> #5) I needed to port the patch to deal with the extent status tree.
> I'm guessing you probably based your patch off of the 3.7 version of
> the kernel, and not the 3.8-rcX development tree.
>
> Here's the modified patch; if someone else on the ext4 development
> team could review the patch after my modifications, I'd appreciate it.
>
> 						- Ted
>
>  From 3de49c9eeb1303b751b92b2f554c57c686e2997d Mon Sep 17 00:00:00 2001
> From: "Dr. Tilmann Bubeck" <t.bubeck@reinform.de>
> Date: Sat, 6 Apr 2013 22:44:18 -0400
> Subject: [PATCH] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT
>
> Add a new ioctl, EXT4_IOC_SWAP_BOOT which swaps i_blocks and
> associated attributes (like i_blocks, i_size, i_flags, ...) from
> the associated inode with inode EXT4_BOOT_LOADER_INO (#5). This is
> typically used to store a boot loader in a secure part of the
> filesystem, where it can't be changed by the user on accident.  The
> data blocks of the previous boot loader will be associated with the
> given inode.
>
> This usercode program is a simple example of the usage:
>
> int main(int argc, char *argv[])
> {
>    int fd;
>    int err;
>
>    if ( argc != 2 ) {
>      printf("usage: ext4-swap-boot-inode FILE-TO-SWAP\n");
>      exit(1);
>    }
>
>    fd = open(argv[1], O_WRONLY);
>    if ( fd < 0 ) {
>      perror("open");
>      exit(1);
>    }
>
>    err = ioctl(fd, EXT4_IOC_SWAP_BOOT);
>    if ( err < 0 ) {
>      perror("ioctl");
>      exit(1);
>    }
>
>    close(fd);
>    exit(0);
> }
>
> [ Modified by Theodore Ts'o to fix a number of bugs in the original code.]
>
> Signed-off-by: Dr. Tilmann Bubeck <t.bubeck@reinform.de>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>   Documentation/filesystems/ext4.txt |  10 ++
>   fs/ext4/ext4.h                     |   8 ++
>   fs/ext4/inode.c                    |  11 ++-
>   fs/ext4/ioctl.c                    | 197 +++++++++++++++++++++++++++++++++++++
>   fs/ext4/move_extent.c              |  48 ++++-----
>   5 files changed, 248 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
> index 34ea4f1..69e83a4 100644
> --- a/Documentation/filesystems/ext4.txt
> +++ b/Documentation/filesystems/ext4.txt
> @@ -587,6 +587,16 @@ Table of Ext4 specific ioctls
>   			      bitmaps and inode table, the userspace tool thus
>   			      just passes the new number of blocks.
>
> +EXT4_IOC_SWAP_BOOT	      Swap i_blocks and associated attributes
> +			      (like i_blocks, i_size, i_flags, ...) from
> +			      the associated inode with inode
> +			      EXT4_BOOT_LOADER_INO (#5). This is typically
> +			      used to store a boot loader in a secure part of
> +			      the filesystem, where it can't be changed by the
> +			      user on accident.
> +			      The data blocks of the previous boot loader
> +			      will be associated with the given inode.
> +
>   ..............................................................................
>
>   References
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a0637e5..d918715 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -616,6 +616,7 @@ enum {
>   #define EXT4_IOC_ALLOC_DA_BLKS		_IO('f', 12)
>   #define EXT4_IOC_MOVE_EXT		_IOWR('f', 15, struct move_extent)
>   #define EXT4_IOC_RESIZE_FS		_IOW('f', 16, __u64)
> +#define EXT4_IOC_SWAP_BOOT		_IO('f', 17)
>
>   #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
>   /*
> @@ -1341,6 +1342,7 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
>   	return ino == EXT4_ROOT_INO ||
>   		ino == EXT4_USR_QUOTA_INO ||
>   		ino == EXT4_GRP_QUOTA_INO ||
> +		ino == EXT4_BOOT_LOADER_INO ||
>   		ino == EXT4_JOURNAL_INO ||
>   		ino == EXT4_RESIZE_INO ||
>   		(ino >= EXT4_FIRST_INO(sb) &&
> @@ -2624,6 +2626,12 @@ extern int ext4_ind_migrate(struct inode *inode);
>
>
>   /* move_extent.c */
> +extern void ext4_double_down_write_data_sem(struct inode *first,
> +					    struct inode *second);
> +extern void ext4_double_up_write_data_sem(struct inode *orig_inode,
> +					  struct inode *donor_inode);
> +void ext4_inode_double_lock(struct inode *inode1, struct inode *inode2);
> +void ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2);
>   extern int ext4_move_extents(struct file *o_filp, struct file *d_filp,
>   			     __u64 start_orig, __u64 start_donor,
>   			     __u64 len, __u64 *moved_len);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 769c656..a29bfc2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4191,8 +4191,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>   	 * NeilBrown 1999oct15
>   	 */
>   	if (inode->i_nlink == 0) {
> -		if (inode->i_mode == 0 ||
> -		    !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
> +		if ((inode->i_mode == 0 ||
> +		     !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) &&
> +		    ino != EXT4_BOOT_LOADER_INO) {
>   			/* this inode is deleted */
>   			ret = -ESTALE;
>   			goto bad_inode;
> @@ -4200,7 +4201,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>   		/* The only unlinked inodes we let through here have
>   		 * valid i_mode and are being read by the orphan
>   		 * recovery code: that's fine, we're about to complete
> -		 * the process of deleting those. */
> +		 * the process of deleting those.
> +		 * OR it is the EXT4_BOOT_LOADER_INO which is
> +		 * not initialized on a new filesystem. */
>   	}
>   	ei->i_flags = le32_to_cpu(raw_inode->i_flags);
>   	inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
> @@ -4320,6 +4323,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>   		else
>   			init_special_inode(inode, inode->i_mode,
>   			   new_decode_dev(le32_to_cpu(raw_inode->i_block[1])));
> +	} else if (ino == EXT4_BOOT_LOADER_INO) {
> +		make_bad_inode(inode);
>   	} else {
>   		ret = -EIO;
>   		EXT4_ERROR_INODE(inode, "bogus i_mode (%o)", inode->i_mode);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a07b7bc..140a8d6 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -17,9 +17,201 @@
>   #include <asm/uaccess.h>
>   #include "ext4_jbd2.h"
>   #include "ext4.h"
> +#include "ext4_extents.h"
>
>   #define MAX_32_NUM ((((unsigned long long) 1) << 32) - 1)
>
> +/**
> + * Swap memory between @a and @b for @len bytes.
> + *
> + * @a:          pointer to first memory area
> + * @b:          pointer to second memory area
> + * @len:        number of bytes to swap
> + *
> + */
> +static void memswap(void *a, void *b, size_t len)
> +{
> +	unsigned char *ap, *bp;
> +	unsigned char tmp;
> +
> +	ap = (unsigned char *)a;
> +	bp = (unsigned char *)b;
> +	while (len-- > 0) {
> +		tmp = *ap;
> +		*ap = *bp;
> +		*bp = tmp;
> +		ap++;
> +		bp++;
> +	}
> +}
> +
> +/**
> + * Swap i_data and associated attributes between inode1 and inode2.
> + * This function is used for the primary swap between inode1 and inode2
> + * and also to revert this primary swap in case of errors.
> + *
> + * Therefore you have to make sure, that calling this method twice
> + * will revert all changes.
> + *
> + * @inode1:     pointer to first inode
> + * @inode2:     pointer to second inode
> + */
> +static void swap_inode_data(struct inode *inode1, struct inode *inode2)
> +{
> +	loff_t isize;
> +	struct ext4_inode_info *ei1;
> +	struct ext4_inode_info *ei2;
> +
> +	ei1 = EXT4_I(inode1);
> +	ei2 = EXT4_I(inode2);
> +
> +	memswap(&inode1->i_flags, &inode2->i_flags, sizeof(inode1->i_flags));
> +	memswap(&inode1->i_version, &inode2->i_version,
> +		  sizeof(inode1->i_version));
> +	memswap(&inode1->i_blocks, &inode2->i_blocks,
> +		  sizeof(inode1->i_blocks));
> +	memswap(&inode1->i_bytes, &inode2->i_bytes, sizeof(inode1->i_bytes));
> +	memswap(&inode1->i_atime, &inode2->i_atime, sizeof(inode1->i_atime));
> +	memswap(&inode1->i_mtime, &inode2->i_mtime, sizeof(inode1->i_mtime));
> +
> +	memswap(ei1->i_data, ei2->i_data, sizeof(ei1->i_data));
> +	memswap(&ei1->i_flags, &ei2->i_flags, sizeof(ei1->i_flags));
> +	memswap(&ei1->i_disksize, &ei2->i_disksize, sizeof(ei1->i_disksize));
> +	memswap(&ei1->i_es_tree, &ei2->i_es_tree, sizeof(ei1->i_es_tree));
> +	memswap(&ei1->i_es_lru_nr, &ei2->i_es_lru_nr, sizeof(ei1->i_es_lru_nr));
> +
> +	isize = i_size_read(inode1);
> +	i_size_write(inode1, i_size_read(inode2));
> +	i_size_write(inode2, isize);
> +}
> +
> +/**
> + * Swap the information from the given @inode and the inode
> + * EXT4_BOOT_LOADER_INO. It will basically swap i_data and all other
> + * important fields of the inodes.
> + *
> + * @sb:         the super block of the filesystem
> + * @inode:      the inode to swap with EXT4_BOOT_LOADER_INO
> + *
> + */
> +static long swap_inode_boot_loader(struct super_block *sb,
> +				struct inode *inode)
> +{
> +	handle_t *handle;
> +	int err;
> +	struct inode *inode_bl;
> +	struct ext4_inode_info *ei;
> +	struct ext4_inode_info *ei_bl;
> +	struct ext4_sb_info *sbi;
> +
> +	if (inode->i_nlink != 1 || !S_ISREG(inode->i_mode)) {
> +		err = -EINVAL;
> +		goto swap_boot_out;
> +	}
> +
> +	if (!inode_owner_or_capable(inode) || !capable(CAP_SYS_ADMIN)) {
> +		err = -EPERM;
> +		goto swap_boot_out;
> +	}
> +
> +	sbi = EXT4_SB(sb);
> +	ei = EXT4_I(inode);
> +
> +	inode_bl = ext4_iget(sb, EXT4_BOOT_LOADER_INO);
> +	if (IS_ERR(inode_bl)) {
> +		err = PTR_ERR(inode_bl);
> +		goto swap_boot_out;
> +	}
> +	ei_bl = EXT4_I(inode_bl);
> +
> +	filemap_flush(inode->i_mapping);
> +	filemap_flush(inode_bl->i_mapping);
> +
> +	/* Protect orig inodes against a truncate and make sure,
> +	 * that only 1 swap_inode_boot_loader is running. */
> +	ext4_inode_double_lock(inode, inode_bl);
> +
> +	truncate_inode_pages(&inode->i_data, 0);
> +	truncate_inode_pages(&inode_bl->i_data, 0);
> +
> +	/* Wait for all existing dio workers */
> +	ext4_inode_block_unlocked_dio(inode);
> +	ext4_inode_block_unlocked_dio(inode_bl);
> +	inode_dio_wait(inode);
> +	inode_dio_wait(inode_bl);
> +
> +	handle = ext4_journal_start(inode_bl, EXT4_HT_MOVE_EXTENTS, 2);
> +	if (IS_ERR(handle)) {
> +		err = -EINVAL;
> +		goto swap_boot_out;
> +	}
> +
> +	/* Protect extent tree against block allocations via delalloc */
> +	ext4_double_down_write_data_sem(inode, inode_bl);
> +
> +	if (inode_bl->i_nlink == 0) {
> +		/* this inode has never been used as a BOOT_LOADER */
> +		set_nlink(inode_bl, 1);
> +		i_uid_write(inode_bl, 0);
> +		i_gid_write(inode_bl, 0);
> +		inode_bl->i_flags = 0;
> +		ei_bl->i_flags = 0;
> +		inode_bl->i_version = 1;
> +		i_size_write(inode_bl, 0);
> +		inode_bl->i_mode = S_IFREG;
> +		if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> +					      EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> +			ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
> +			ext4_ext_tree_init(handle, inode_bl);
> +		} else
> +			memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));
> +	}
> +
> +	swap_inode_data(inode, inode_bl);
> +
> +	inode->i_ctime = inode_bl->i_ctime = ext4_current_time(inode);
> +
> +	spin_lock(&sbi->s_next_gen_lock);
> +	inode->i_generation = sbi->s_next_generation++;
> +	inode_bl->i_generation = sbi->s_next_generation++;
> +	spin_unlock(&sbi->s_next_gen_lock);
> +
> +	ext4_discard_preallocations(inode);
> +
> +	err = ext4_mark_inode_dirty(handle, inode);
> +	if (err < 0) {
> +		ext4_warning(inode->i_sb,
> +			"couldn't mark inode #%lu dirty (err %d)",
> +			inode->i_ino, err);
> +		/* Revert all changes: */
> +		swap_inode_data(inode, inode_bl);
> +	} else {
> +		err = ext4_mark_inode_dirty(handle, inode_bl);
> +		if (err < 0) {
> +			ext4_warning(inode_bl->i_sb,
> +				"couldn't mark inode #%lu dirty (err %d)",
> +				inode_bl->i_ino, err);
> +			/* Revert all changes: */
> +			swap_inode_data(inode, inode_bl);
> +			ext4_mark_inode_dirty(handle, inode);
> +		}
> +	}
> +
> +	ext4_journal_stop(handle);
> +
> +	ext4_double_up_write_data_sem(inode, inode_bl);
> +
> +	ext4_inode_resume_unlocked_dio(inode);
> +	ext4_inode_resume_unlocked_dio(inode_bl);
> +
> +	ext4_inode_double_unlock(inode, inode_bl);
> +
> +	iput(inode_bl);
> +
> +swap_boot_out:
> +	return err;
> +}
> +
>   long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   {
>   	struct inode *inode = file_inode(filp);
> @@ -353,6 +545,11 @@ group_add_out:
>   		return err;
>   	}
>
> +	case EXT4_IOC_SWAP_BOOT:
> +		if (!(filp->f_mode & FMODE_WRITE))
> +			return -EBADF;
> +		return swap_inode_boot_loader(sb, inode);
> +
>   	case EXT4_IOC_RESIZE_FS: {
>   		ext4_fsblk_t n_blocks_count;
>   		struct super_block *sb = inode->i_sb;
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 33e1c08..a2e696e 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -144,12 +144,13 @@ mext_next_extent(struct inode *inode, struct ext4_ext_path *path,
>   }
>
>   /**
> - * double_down_write_data_sem - Acquire two inodes' write lock of i_data_sem
> + * ext4_double_down_write_data_sem - Acquire two inodes' write lock
> + *                                   of i_data_sem
>    *
>    * Acquire write lock of i_data_sem of the two inodes
>    */
> -static void
> -double_down_write_data_sem(struct inode *first, struct inode *second)
> +void
> +ext4_double_down_write_data_sem(struct inode *first, struct inode *second)
>   {
>   	if (first < second) {
>   		down_write(&EXT4_I(first)->i_data_sem);
> @@ -162,14 +163,15 @@ double_down_write_data_sem(struct inode *first, struct inode *second)
>   }
>
>   /**
> - * double_up_write_data_sem - Release two inodes' write lock of i_data_sem
> + * ext4_double_up_write_data_sem - Release two inodes' write lock of i_data_sem
>    *
>    * @orig_inode:		original inode structure to be released its lock first
>    * @donor_inode:	donor inode structure to be released its lock second
>    * Release write lock of i_data_sem of two inodes (orig and donor).
>    */
> -static void
> -double_up_write_data_sem(struct inode *orig_inode, struct inode *donor_inode)
> +void
> +ext4_double_up_write_data_sem(struct inode *orig_inode,
> +			      struct inode *donor_inode)
>   {
>   	up_write(&EXT4_I(orig_inode)->i_data_sem);
>   	up_write(&EXT4_I(donor_inode)->i_data_sem);
> @@ -976,7 +978,7 @@ again:
>   	 * necessary, just swap data blocks between orig and donor.
>   	 */
>   	if (uninit) {
> -		double_down_write_data_sem(orig_inode, donor_inode);
> +		ext4_double_down_write_data_sem(orig_inode, donor_inode);
>   		/* If any of extents in range became initialized we have to
>   		 * fallback to data copying */
>   		uninit = mext_check_coverage(orig_inode, orig_blk_offset,
> @@ -990,7 +992,7 @@ again:
>   			goto drop_data_sem;
>
>   		if (!uninit) {
> -			double_up_write_data_sem(orig_inode, donor_inode);
> +			ext4_double_up_write_data_sem(orig_inode, donor_inode);
>   			goto data_copy;
>   		}
>   		if ((page_has_private(pagep[0]) &&
> @@ -1004,7 +1006,7 @@ again:
>   						donor_inode, orig_blk_offset,
>   						block_len_in_page, err);
>   	drop_data_sem:
> -		double_up_write_data_sem(orig_inode, donor_inode);
> +		ext4_double_up_write_data_sem(orig_inode, donor_inode);
>   		goto unlock_pages;
>   	}
>   data_copy:
> @@ -1065,11 +1067,11 @@ repair_branches:
>   	 * Extents are swapped already, but we are not able to copy data.
>   	 * Try to swap extents to it's original places
>   	 */
> -	double_down_write_data_sem(orig_inode, donor_inode);
> +	ext4_double_down_write_data_sem(orig_inode, donor_inode);
>   	replaced_count = mext_replace_branches(handle, donor_inode, orig_inode,
>   					       orig_blk_offset,
>   					       block_len_in_page, &err2);
> -	double_up_write_data_sem(orig_inode, donor_inode);
> +	ext4_double_up_write_data_sem(orig_inode, donor_inode);
>   	if (replaced_count != block_len_in_page) {
>   		EXT4_ERROR_INODE_BLOCK(orig_inode, (sector_t)(orig_blk_offset),
>   				       "Unable to copy data block,"
> @@ -1209,15 +1211,15 @@ mext_check_arguments(struct inode *orig_inode,
>   }
>
>   /**
> - * mext_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
> + * ext4_inode_double_lock - Lock i_mutex on both @inode1 and @inode2
>    *
>    * @inode1:	the inode structure
>    * @inode2:	the inode structure
>    *
>    * Lock two inodes' i_mutex
>    */
> -static void
> -mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
> +void
> +ext4_inode_double_lock(struct inode *inode1, struct inode *inode2)
>   {
>   	BUG_ON(inode1 == inode2);
>   	if (inode1 < inode2) {
> @@ -1230,15 +1232,15 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
>   }
>
>   /**
> - * mext_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
> + * ext4_inode_double_unlock - Release i_mutex on both @inode1 and @inode2
>    *
>    * @inode1:     the inode that is released first
>    * @inode2:     the inode that is released second
>    *
>    */
>
> -static void
> -mext_inode_double_unlock(struct inode *inode1, struct inode *inode2)
> +void
> +ext4_inode_double_unlock(struct inode *inode1, struct inode *inode2)
>   {
>   	mutex_unlock(&inode1->i_mutex);
>   	mutex_unlock(&inode2->i_mutex);
> @@ -1333,7 +1335,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>   		return -EINVAL;
>   	}
>   	/* Protect orig and donor inodes against a truncate */
> -	mext_inode_double_lock(orig_inode, donor_inode);
> +	ext4_inode_double_lock(orig_inode, donor_inode);
>
>   	/* Wait for all existing dio workers */
>   	ext4_inode_block_unlocked_dio(orig_inode);
> @@ -1342,7 +1344,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>   	inode_dio_wait(donor_inode);
>
>   	/* Protect extent tree against block allocations via delalloc */
> -	double_down_write_data_sem(orig_inode, donor_inode);
> +	ext4_double_down_write_data_sem(orig_inode, donor_inode);
>   	/* Check the filesystem environment whether move_extent can be done */
>   	ret = mext_check_arguments(orig_inode, donor_inode, orig_start,
>   				    donor_start, &len);
> @@ -1466,7 +1468,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>   		 * b. racing with ->readpage, ->write_begin, and ext4_get_block
>   		 *    in move_extent_per_page
>   		 */
> -		double_up_write_data_sem(orig_inode, donor_inode);
> +		ext4_double_up_write_data_sem(orig_inode, donor_inode);
>
>   		while (orig_page_offset <= seq_end_page) {
>
> @@ -1500,7 +1502,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
>   				block_len_in_page = rest_blocks;
>   		}
>
> -		double_down_write_data_sem(orig_inode, donor_inode);
> +		ext4_double_down_write_data_sem(orig_inode, donor_inode);
>   		if (ret < 0)
>   			break;
>
> @@ -1538,10 +1540,10 @@ out:
>   		ext4_ext_drop_refs(holecheck_path);
>   		kfree(holecheck_path);
>   	}
> -	double_up_write_data_sem(orig_inode, donor_inode);
> +	ext4_double_up_write_data_sem(orig_inode, donor_inode);
>   	ext4_inode_resume_unlocked_dio(orig_inode);
>   	ext4_inode_resume_unlocked_dio(donor_inode);
> -	mext_inode_double_unlock(orig_inode, donor_inode);
> +	ext4_inode_double_unlock(orig_inode, donor_inode);
>
>   	return ret;
>   }
>


-- 
+-------+-------------------------------------------------------------+
|       | dr. tilmann bubeck               reinform medien- und       |
|       |                                  informationstechnologie AG |
| rein  | fon  : +49 (711) 7 82 76-52      loeffelstr. 40             |
| form  | fax  : +49 (711) 7 82 76-46      70597 stuttgart / germany  |
|    AG | cell.: +49 (172) 8 84 29 72      fon: +49 (711) 75 86 56-10 |
|       | email: t.bubeck@reinform.de      http://www.reinform.de     |
|       +-------------------------------------------------------------+
|       | pflichtangaben nach paragraph 80, AktG:                     |
|       | reinform medien- und informationstechnologie AG, stuttgart  |
|       | handelsregister stuttgart, HRB 23001                        |
|       | vorstand:     dr. tilmann bubeck (vorsitz)                  |
|       | aufsichtsrat: frank stege (vorsitz)                         |
+-------+-------------------------------------------------------------+

Re: [PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

[Theodore Ts'o]

On Sun, Apr 07, 2013 at 11:09:58AM -0600, Andreas Dilger wrote:
> > +    filemap_flush(inode_bl->i_mapping);
> 
> Technically, it shouldn't be possible to have dirty pages on the
> bootloader inode, since any inode swapped there will itself have
> been flushed, and should not be directly accessible after that
> point. That said, I guess this ioctl() won't be called too often so
> the empty flush is mostly harmless.

Yes, technically there should be no mappings for the inode boot
loader.  (Well, unless someone had one of the unofficial open-by-inode
number patches).

> > +    if (inode_bl->i_nlink == 0) {
> > +        /* this inode has never been used as a BOOT_LOADER */
> > +        set_nlink(inode_bl, 1);
> > +        i_uid_write(inode_bl, 0);
> > +        i_gid_write(inode_bl, 0);
> > +        inode_bl->i_flags = 0;
> > +        ei_bl->i_flags = 0;
> > +        inode_bl->i_version = 1;
> > +        i_size_write(inode_bl, 0);
> > +        inode_bl->i_mode = S_IFREG;
> > +        if (EXT4_HAS_INCOMPAT_FEATURE(sb,
> > +                          EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> > +            ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
> > +            ext4_ext_tree_init(handle, inode_bl);
> > +        } else
> > +            memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));
> 
> I don't understand this. Wouldn't this clobber the block pointers if 
> an existing boot inode and cause them to leak?  This seems broken to me. 

If i_nlink is zero, then there is no existing boot loader inode.  In
theory the rest of the inode is zero, but this is to make sure the
inode is initialized to something sane before we swap it into the
user-visible inode.

I'll fix up the rest of the minor errors and typos which you pointed
out, thanks!

						- Ted

Re: [PATCH -v3] ext4: implementation of a new ioctl called EXT4_IOC_SWAP_BOOT

[Andreas Dilger]

On 2013-04-07, at 13:48, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sun, Apr 07, 2013 at 11:09:58AM -0600, Andreas Dilger wrote:
>>> +    if (inode_bl->i_nlink == 0) {
>>> +        /* this inode has never been used as a BOOT_LOADER */
>>> +        set_nlink(inode_bl, 1);
>>> +        i_uid_write(inode_bl, 0);
>>> +        i_gid_write(inode_bl, 0);
>>> +        inode_bl->i_flags = 0;
>>> +        ei_bl->i_flags = 0;
>>> +        inode_bl->i_version = 1;
>>> +        i_size_write(inode_bl, 0);
>>> +        inode_bl->i_mode = S_IFREG;
>>> +        if (EXT4_HAS_INCOMPAT_FEATURE(sb,
>>> +                          EXT4_FEATURE_INCOMPAT_EXTENTS)) {
>>> +            ext4_set_inode_flag(inode_bl, EXT4_INODE_EXTENTS);
>>> +            ext4_ext_tree_init(handle, inode_bl);
>>> +        } else
>>> +            memset(ei_bl->i_data, 0, sizeof(ei_bl->i_data));
>> 
>> I don't understand this. Wouldn't this clobber the block pointers if 
>> an existing boot inode and cause them to leak?  This seems broken to me. 
> 
> If i_nlink is zero, then there is no existing boot loader inode.  In
> theory the rest of the inode is zero, but this is to make sure the
> inode is initialized to something sane before we swap it into the
> user-visible inode.

Oh, my bad. I thought the else was for the nlink != 0 case, not the !extent case (the danger of reading email on my phone, I guess). Looks fine. 

> 

Cheers, Andreas