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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 19 Jun 2012 15:48:10 +1000
From:	Tony Breeds <tony@...eyournoodle.com>
To:	Ted Ts'o <tytso@....edu>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: Minimal configuration for e2fsprogs

On Mon, Jun 18, 2012 at 01:12:57PM -0400, Ted Ts'o wrote:
> On Mon, Jun 18, 2012 at 03:58:42PM +1000, Tony Breeds wrote:
> > 
> > I'm not certain we're on the same page.  We're linking statically so
> > that fact we don't call the progress functions doesn't matter.  The
> > code is in libext2fs.a and there must be a call path from (eg)
> > ext2fs_open() to fwrite(stderr, ...).  The fact we don't add the 
> > EXT2_FLAG_PRINT_PROGRESS doesn't come into it does it?
> 
> Oh, I see, the problem is that ext2fs_closefs() is calling the
> progress functions; I had forgotten about it.  Yeah, that's something
> I can fix so that the progress functions are only dragged in if mke2fs
> explicitly requests it, via adding function which enables the progress
> functions, instead of just setting the EXT2_FLAG_PRINT_PROGRESS.

Okay that sounds good to me.  If I get the other bits implemented
before you get to it.  I'm happy to take a run at it.
 
> So a couple of things; it's a really bad to define static functions in
> a header file such as ext2fs.h.  Yes, gcc will normally optimize out
> functions which aren't used, but it will also complain with warnings
> if you enable them.

Thanks for the pointers.  I had meant to make them static inline I just
forgot the inline.
 
> Instead, what I would do is to simply take out the calls to the
> ext2fs_mmp library functions in e2fsck and tune2fs, and remove the MMP
> feature flag from the list of supported features.  That way, if
> someone tries to use the e2fsck binary compiled with --disable-mmp on
> a file system with MMP enabled, e2fsck will refuse to touch the file
> system saying that it doesn't support the feature.  If you just make
> the mmp functions silently succeed (by returning 0), that's really bad
> since file system damage could result.

Okay that all make sense I've had a try but I think it's pretty brutal.
I'm sure I could finesse it a bit but I have a couple of questions:

1) The only way I could see to remove MMP from the supported features
was with something like:

#ifdef CONFIG_MMP
#define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...|EXT4_FEATURE_INCOMPAT_MMP|...)
#else
#define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...)
#endif

But EXT2_LIB_FEATURE_INCOMPAT_SUPP is already #defined twice depending
on the state of ENABLE_COMPRESSION.  So going down the path above would
mean #defining it 4 times which I'd think is starting to indicate there
must be a better way.  Firstly am I in the right ball park, secondly
Would something like

#ifdef ENABLE_COMPRESSION
...
#define _EXT4_FEATURE_INCOMPAT_COMPRESSION EXT4_FEATURE_INCOMPAT_COMPRESSION
#else
#define _EXT4_FEATURE_INCOMPAT_COMPRESSION (0)
#endif

#ifdef ENABLE_MMP
...
#define _EXT4_FEATURE_INCOMPAT_MMP EXT4_FEATURE_INCOMPAT_MMP
#else
#define _EXT4_FEATURE_INCOMPAT_MMP (0)
#endif

#define EXT2_LIB_FEATURE_INCOMPAT_SUPP  (...\
                                         _EXT2_FEATURE_INCOMPAT_COMPRESSION|\
                                         ... \
                                         _EXT4_FEATURE_INCOMPAT_MMP|\
                                         ...)

Be acceptable?

2) in debugfs you have a command table (debug_cmds.ct or similar) there
doesn't seem to be a way to exclude commands based on the state of
CONFIG_MMP.  Is it a problem the expose a debugfs command that will do
nothing when built with --disable-mmp, or should I look at extending
the command table generator to support the conditionals?

I'll add my current patch after my .signature

> I already have things set up so that if you don't actually call the
> functions to read in the bitmaps, the functions to read and write the
> bitmaps don't get dragged into a static library link.  That's what I'm
> referring to.

Ahh thanks for the clarification.
 
> The functions to actually create in-memory bitmaps for various
> housekeeping things, do need to get dragged in, but we don't
> necessarily need to drag in all of the different bitmap back-ends.  If
> we assume that yaboot doesn't need to optimize for low-memory usage
> for really, really big file systems (i.e., you're not going to try to
> allocate files or blocks while opening a multi-terrabyte file system
> using libext2fs on a system with only 512mb), then you wouldn't need
> blkmap64_rb.c.

Right, we're operating in a pretty tight memory environment (typically
128MB or 256MB) but we're also usually opening file-systems <100GB
usually closer to 200MB.
 
> And the bitmap statistics functions are sometihng we can use a
> configure option to disable, which should remove the last bits of
> stdio usage I believe.

Okay.  If I can get the disable-mmp patch into a state you like it I'll
tackle that configure option as well.

> If you'd like to try to clean up the --disable-mmp patch, and perhaps
> try your hand at a --disable-libext2fs-stats patch which disable the
> statistics options, that would be great.  Otherwise, I'll try to get
> to it in the next few weeks.

I really appreciate you help and time, and hope that continually
pointing me in the right direction isn't too taxing on you.

Yours Tony

---
 configure.in           |   21 +++++++++++++++++++++
 debugfs/debugfs.c      |    2 ++
 debugfs/set_fields.c   |    9 +++++++++
 e2fsck/journal.c       |    2 ++
 e2fsck/unix.c          |    4 ++++
 e2fsck/util.c          |    6 ++++++
 lib/config.h.in        |    3 +++
 lib/ext2fs/Makefile.in |    4 ++--
 lib/ext2fs/closefs.c   |    2 ++
 lib/ext2fs/ext2fs.h    |    2 ++
 lib/ext2fs/openfs.c    |    2 ++
 misc/mke2fs.c          |    2 ++
 misc/tune2fs.c         |    6 ++++++
 13 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/configure.in b/configure.in
index 7373e8e..4bacd1a 100644
--- a/configure.in
+++ b/configure.in
@@ -746,6 +746,27 @@ AC_MSG_RESULT([Building uuidd by default])
 )
 AC_SUBST(UUIDD_CMT)
 dnl
+dnl handle --disable-mmp
+dnl
+AH_TEMPLATE([CONFIG_MMP], [Define to 1 to enable mmp support])
+AC_ARG_ENABLE([mmp],
+[  --disable-mmp   	  disable support mmp, Multi Mount Protection],
+if test "$enableval" = "no"
+then
+	AC_MSG_RESULT([Disabling mmp support])
+	MMP_CMT="#"
+else
+	AC_MSG_RESULT([Enabling mmp support])
+	AC_DEFINE(CONFIG_MMP, 1)
+	MMP_CMT=
+fi
+,
+AC_MSG_RESULT([Enabling mmp support by default])
+AC_DEFINE(CONFIG_MMP, 1)
+MMP_CMT=
+)
+AC_SUBST(MMP_CMT)
+dnl
 dnl
 dnl
 MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library
diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index cf80bd0..5df915e 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -2194,6 +2194,7 @@ void do_punch(int argc, char *argv[])
 
 void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
 {
+#ifdef CONFIG_MMP
 	struct ext2_super_block *sb;
 	struct mmp_struct *mmp_s;
 	time_t t;
@@ -2237,6 +2238,7 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[])
 	fprintf(stdout, "node_name: %s\n", mmp_s->mmp_nodename);
 	fprintf(stdout, "device_name: %s\n", mmp_s->mmp_bdevname);
 	fprintf(stdout, "magic: 0x%x\n", mmp_s->mmp_magic);
+#endif
 }
 
 static int source_file(const char *cmd_file, int sci_idx)
diff --git a/debugfs/set_fields.c b/debugfs/set_fields.c
index 08bfd8d..77fbbc1 100644
--- a/debugfs/set_fields.c
+++ b/debugfs/set_fields.c
@@ -69,8 +69,10 @@ static errcode_t parse_hashalg(struct field_set_info *info, char *field, char *a
 static errcode_t parse_time(struct field_set_info *info, char *field, char *arg);
 static errcode_t parse_bmap(struct field_set_info *info, char *field, char *arg);
 static errcode_t parse_gd_csum(struct field_set_info *info, char *field, char *arg);
+#ifdef CONFIG_MMP
 static errcode_t parse_mmp_clear(struct field_set_info *info, char *field,
 				 char *arg);
+#endif
 
 static struct field_set_info super_fields[] = {
 	{ "inodes_count", &set_sb.s_inodes_count, NULL, 4, parse_uint },
@@ -246,6 +248,7 @@ static struct field_set_info ext4_bg_fields[] = {
 };
 
 static struct field_set_info mmp_fields[] = {
+#ifdef CONFIG_MMP
 	{ "clear", &set_mmp.mmp_magic, NULL, sizeof(set_mmp), parse_mmp_clear },
 	{ "magic", &set_mmp.mmp_magic, NULL, 4, parse_uint },
 	{ "seq", &set_mmp.mmp_seq, NULL, 4, parse_uint },
@@ -255,6 +258,8 @@ static struct field_set_info mmp_fields[] = {
 	{ "bdevname", &set_mmp.mmp_bdevname, NULL, sizeof(set_mmp.mmp_bdevname),
 		parse_string },
 	{ "check_interval", &set_mmp.mmp_check_interval, NULL, 2, parse_uint },
+#endif
+	{ 0, 0, 0, 0 }
 };
 
 static int check_suffix(const char *field)
@@ -748,6 +753,7 @@ void do_set_block_group_descriptor(int argc, char *argv[])
 	}
 }
 
+#ifdef CONFIG
 static errcode_t parse_mmp_clear(struct field_set_info *info,
 				 char *field EXT2FS_ATTR((unused)),
 				 char *arg EXT2FS_ATTR((unused)))
@@ -762,9 +768,11 @@ static errcode_t parse_mmp_clear(struct field_set_info *info,
 
 	return 1; /* we don't need the MMP block written again */
 }
+#endif
 
 void do_set_mmp_value(int argc, char *argv[])
 {
+#ifdef CONFIG_MMP
 	const char *usage = "<field> <value>\n"
 		"\t\"set_mmp_value -l\" will list the names of "
 		"MMP fields\n\twhich can be set.";
@@ -819,5 +827,6 @@ void do_set_mmp_value(int argc, char *argv[])
 				 &set_mmp);
 		*mmp_s = set_mmp;
 	}
+#endif
 }
 
diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 767ea10..e5ee4ed 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -889,7 +889,9 @@ int e2fsck_run_ext3_journal(e2fsck_t ctx)
 	if (stats && stats->bytes_written)
 		kbytes_written = stats->bytes_written >> 10;
 
+#ifdef CONFIG_MMP
 	ext2fs_mmp_stop(ctx->fs);
+#endif
 	ext2fs_free(ctx->fs);
 	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
 			     ctx->superblock, blocksize, io_ptr,
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 94260bd..6cca9e0 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -1042,6 +1042,7 @@ static const char *my_ver_date = E2FSPROGS_DATE;
 
 static int e2fsck_check_mmp(ext2_filsys fs, e2fsck_t ctx)
 {
+#ifdef CONFIG_MMP
 	struct mmp_struct *mmp_s;
 	unsigned int mmp_check_interval;
 	errcode_t retval = 0;
@@ -1120,6 +1121,9 @@ check_error:
 		}
 	}
 	return retval;
+#else
+	return 0;
+#endif
 }
 
 int main (int argc, char *argv[])
diff --git a/e2fsck/util.c b/e2fsck/util.c
index 7c4caab..5b9b154 100644
--- a/e2fsck/util.c
+++ b/e2fsck/util.c
@@ -56,7 +56,9 @@ void fatal_error(e2fsck_t ctx, const char *msg)
 	if (!fs)
 		goto out;
 	if (fs->io) {
+#ifdef CONFIG_MMP
 		ext2fs_mmp_stop(ctx->fs);
+#endif
 		if (ctx->fs->io->magic == EXT2_ET_MAGIC_IO_CHANNEL)
 			io_channel_flush(ctx->fs->io);
 		else
@@ -780,6 +782,7 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
 
 errcode_t e2fsck_mmp_update(ext2_filsys fs)
 {
+#ifdef CONFIF_MMP
 	errcode_t retval;
 
 	retval = ext2fs_mmp_update(fs);
@@ -789,6 +792,9 @@ errcode_t e2fsck_mmp_update(ext2_filsys fs)
 			       "being modified while fsck is running.\n"));
 
 	return retval;
+#else
+	return 0;
+#endif
 }
 
 void e2fsck_set_bitmap_type(ext2_filsys fs, unsigned int default_type,
diff --git a/lib/config.h.in b/lib/config.h.in
index 90e9743..52c3897 100644
--- a/lib/config.h.in
+++ b/lib/config.h.in
@@ -12,6 +12,9 @@
 /* Define to 1 if debugging ext3/4 journal code */
 #undef CONFIG_JBD_DEBUG
 
+/* Define to 1 to enable mmp support */
+#undef CONFIG_MMP
+
 /* Define to 1 to enable quota support */
 #undef CONFIG_QUOTA
 
diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index 0d9ac21..5b73958 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -14,9 +14,10 @@ MK_CMDS=	_SS_DIR_OVERRIDE=../ss ../ss/mk_cmds
 @RESIZER_CMT@...IZE_LIB_OBJS = dupfs.o
 @TEST_IO_CMT@...T_IO_LIB_OBJS = test_io.o
 @IMAGER_CMT@...MAGE_LIB_OBJS = imager.o
+@..._CMT@..._OBJS = mmp.o
 
 OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
-	$(TEST_IO_LIB_OBJS) \
+	$(TEST_IO_LIB_OBJS) $(MMP_OBJS) \
 	ext2_err.o \
 	alloc.o \
 	alloc_sb.o \
@@ -66,7 +67,6 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
 	lookup.o \
 	mkdir.o \
 	mkjournal.o \
-	mmp.o \
 	namei.o \
 	native.o \
 	newdir.o \
diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
index 973c2a2..abf77cc 100644
--- a/lib/ext2fs/closefs.c
+++ b/lib/ext2fs/closefs.c
@@ -464,9 +464,11 @@ errcode_t ext2fs_close2(ext2_filsys fs, int flags)
 			return retval;
 	}
 
+#ifdef CONFIG_MMP
 	retval = ext2fs_mmp_stop(fs);
 	if (retval)
 		return retval;
+#endif
 
 	ext2fs_free(fs);
 	return 0;
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index ff088bb..fc83973 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1366,6 +1366,7 @@ errcode_t ext2fs_unlink(ext2_filsys fs, ext2_ino_t dir, const char *name,
 			ext2_ino_t ino, int flags);
 
 /* mmp.c */
+#ifdef CONFIG_MMP
 errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf);
 errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf);
 errcode_t ext2fs_mmp_clear(ext2_filsys fs);
@@ -1374,6 +1375,7 @@ errcode_t ext2fs_mmp_start(ext2_filsys fs);
 errcode_t ext2fs_mmp_update(ext2_filsys fs);
 errcode_t ext2fs_mmp_stop(ext2_filsys fs);
 unsigned ext2fs_mmp_new_seq(void);
+#endif /* CONFIG_MMP */
 
 /* read_bb.c */
 extern errcode_t ext2fs_read_bb_inode(ext2_filsys fs,
diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index 482e4ab..88fdd4d 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -391,6 +391,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 	fs->flags &= ~EXT2_FLAG_NOFREE_ON_ERROR;
 	*ret_fs = fs;
 
+#ifdef CONFIG_MMP
 	if ((fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) &&
 	    !(flags & EXT2_FLAG_SKIP_MMP) &&
 	    (flags & (EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE))) {
@@ -401,6 +402,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
 			goto cleanup;
 		}
 	}
+#endif
 
 	return 0;
 cleanup:
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 7ec8cc2..e9b4c1f 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -2557,6 +2557,7 @@ int main (int argc, char *argv[])
 			printf(_("done\n"));
 	}
 no_journal:
+#ifdef CONFIG_MMP
 	if (!super_only &&
 	    fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) {
 		retval = ext2fs_mmp_init(fs);
@@ -2570,6 +2571,7 @@ no_journal:
 				 "with update interval %d seconds.\n"),
 			       fs->super->s_mmp_update_interval);
 	}
+#endif
 
 	if (EXT2_HAS_RO_COMPAT_FEATURE(&fs_param,
 				       EXT4_FEATURE_RO_COMPAT_BIGALLOC))
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index f1f0bcf..2c4b20b 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -430,6 +430,7 @@ static int update_feature_set(ext2_filsys fs, char *features)
 				return 1;
 		}
 	}
+#ifdef CONFIG_MMP
 	if (FEATURE_ON(E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_MMP)) {
 		int error;
 
@@ -495,6 +496,7 @@ mmp_error:
 		sb->s_mmp_block = 0;
 		sb->s_mmp_update_interval = 0;
 	}
+#endif
 
 	if (FEATURE_ON(E2P_FEATURE_COMPAT, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) {
 		/*
@@ -2105,10 +2107,12 @@ retry_open:
 			goto closefs;
 		}
 	}
+#ifdef CONFIG_MMP
 	if (clear_mmp) {
 		rc = ext2fs_mmp_clear(fs);
 		goto closefs;
 	}
+#endif
 	if (journal_size || journal_device) {
 		rc = add_journal(fs);
 		if (rc)
@@ -2219,7 +2223,9 @@ retry_open:
 
 closefs:
 	if (rc) {
+#ifdef CONFIG_MMP
 		ext2fs_mmp_stop(fs);
+#endif
 		exit(1);
 	}
 
-- 
1.7.6.4


Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists