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] [day] [month] [year] [list]
Date:	Wed, 14 Apr 2010 10:19:52 +0200
From:	Jan Blunck <jblunck@...e.de>
To:	Jan Kara <jack@...e.cz>
Cc:	tytso@....edu,
	Linux-Kernel Mailinglist <linux-kernel@...r.kernel.org>,
	linux-ext4@...r.kernel.org,
	Frederic Weisbecker <fweisbec@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	Andi Kleen <andi@...stfloor.org>,
	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Subject: Re: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

On Tue, Apr 13, Jan Kara wrote:

> On Mon 12-04-10 22:41:45, Jan Blunck wrote:
> > Add a spinlock that protects against concurrent modifications of
> > s_mount_state, s_blocks_last, s_overhead_last and the content of the
> > superblock's buffer pointed to by sbi->s_es. This is a preparation patch
> > for removing the BKL from ext2 in the next patch.
> > 
> > Signed-off-by: Jan Blunck <jblunck@...e.de>
> > Cc: Andi Kleen <andi@...stfloor.org>
> > Cc: Jan Kara <jack@...e.cz>
> > Cc: OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
> > ---
> >  fs/ext2/inode.c            |    2 ++
> >  fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
> >  include/linux/ext2_fs_sb.h |    6 ++++++
> >  3 files changed, 37 insertions(+), 2 deletions(-)
> > 
> >diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> >index fc13cc1..5d15442 100644
> >--- a/fs/ext2/inode.c
> >+++ b/fs/ext2/inode.c
> >@@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
> >                               * created, add a flag to the superblock.
> >                               */
> >                               lock_kernel();
> >+                              spin_lock(&EXT2_SB(sb)->s_lock);
> >                               ext2_update_dynamic_rev(sb);
> >                               EXT2_SET_RO_COMPAT_FEATURE(sb,
> >                                       EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> >+                              spin_unlock(&EXT2_SB(sb)->s_lock);
> >                               unlock_kernel();
> >                               ext2_write_super(sb);
> >                       }
>   Looking at this - probably we should protect by this lock also setting of
> a feature in ext2_xattr_update_super_block(). It's an unrelated bugfix but
> when we are already doing the bugfixing & cleanups in this area...
> 
> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > index b01c491..34d7a62 100644
> > --- a/fs/ext2/super.c
> > +++ b/fs/ext2/super.c
> > @@ -209,6 +216,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
> >  	struct ext2_super_block *es = sbi->s_es;
> >  	unsigned long def_mount_opts;
> >  
> > +	spin_lock(&sbi->s_lock);
> >  	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
> >  
> >  	if (sbi->s_sb_block != 1)
> > @@ -281,6 +289,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
> >  	if (!test_opt(sb, RESERVATION))
> >  		seq_puts(seq, ",noreservation");
> >  
> > +	spin_unlock(&sbi->s_lock);
> >  	return 0;
> >  }
>   Why exactly do you have in the above? Probably because of consistent
> view of mount options? You should comment about that in the changelo and
> especially at the lock declaration in ext2_fs.h.
> 
> > @@ -1158,19 +1173,22 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
> >   * may have been checked while mounted and e2fsck may have
> >   * set s_state to EXT2_VALID_FS after some corrections.
> >   */
> > -
> >  static int ext2_sync_fs(struct super_block *sb, int wait)
> >  {
> > +	struct ext2_sb_info *sbi = EXT2_SB(sb);
> >  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
> >  	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
> >  
> >  	lock_kernel();
> > +	spin_lock(&sbi->s_lock);
> >  	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
> >  		ext2_debug("setting valid to 0\n");
> >  		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
> > +		spin_unlock(&sbi->s_lock);
> >  		ext2_sync_super(sb, es);
> >  	} else {
> >  		ext2_commit_super(sb, es);
> > +		spin_unlock(&sbi->s_lock);
>   Could you please fold in ext2_commit_super? It's used only here and it's
> name looks a bit scary to be called under the spinlock...

Right. Is there actually a reason why we don't update the s_free_blocks_count
and s_free_inodes_count when we found the filesystem did not have the
EXT2_VALID_FS set? Otherwise I could use ext2_sync_super() in both and make it
honor the wait flag by just calling sync_dirty_buffer() when it is necessary.

What do you think?

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 40fc8d5..8187061 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -39,7 +39,7 @@
 #include "xip.h"
 
 static void ext2_sync_super(struct super_block *sb,
-			    struct ext2_super_block *es);
+			    struct ext2_super_block *es, int wait);
 static int ext2_remount (struct super_block * sb, int * flags, char * data);
 static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf);
 static int ext2_sync_fs(struct super_block *sb, int wait);
@@ -54,7 +54,7 @@ void ext2_error (struct super_block * sb, const char * function,
 	if (!(sb->s_flags & MS_RDONLY)) {
 		sbi->s_mount_state |= EXT2_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT2_ERROR_FS);
-		ext2_sync_super(sb, es);
+		ext2_sync_super(sb, es, 1);
 	}
 
 	va_start(args, fmt);
@@ -125,7 +125,7 @@ static void ext2_put_super (struct super_block * sb)
 		struct ext2_super_block *es = sbi->s_es;
 
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
-		ext2_sync_super(sb, es);
+		ext2_sync_super(sb, es, 1);
 	}
 	db_count = sbi->s_gdb_count;
 	for (i = 0; i < db_count; i++)
@@ -1127,23 +1127,16 @@ static void ext2_clear_super_error(struct super_block *sb)
 	}
 }
 
-static void ext2_commit_super (struct super_block * sb,
-			       struct ext2_super_block * es)
-{
-	ext2_clear_super_error(sb);
-	es->s_wtime = cpu_to_le32(get_seconds());
-	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
-	sb->s_dirt = 0;
-}
-
-static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
+static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es,
+			    int wait)
 {
 	ext2_clear_super_error(sb);
 	es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb));
 	es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb));
 	es->s_wtime = cpu_to_le32(get_seconds());
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
-	sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
+	if (wait)
+		sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
 	sb->s_dirt = 0;
 }
 
@@ -1167,11 +1160,8 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
 		ext2_debug("setting valid to 0\n");
 		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
-		ext2_sync_super(sb, es);
-	} else {
-		ext2_commit_super(sb, es);
 	}
-	sb->s_dirt = 0;
+	ext2_sync_super(sb, es, wait);
 	unlock_kernel();
 
 	return 0;
@@ -1269,7 +1259,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		if (!ext2_setup_super (sb, es, 0))
 			sb->s_flags &= ~MS_RDONLY;
 	}
-	ext2_sync_super(sb, es);
+	ext2_sync_super(sb, es, 1);
 	unlock_kernel();
 	return 0;
 restore_opts:
--
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

Powered by blists - more mailing lists