linux-ext4 - Re: Minimal configuration for e2fsprogs

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

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

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

Message-ID: <20120619135610.GA10637@thunk.org>
Date:	Tue, 19 Jun 2012 09:56:10 -0400
From:	Ted Ts'o <tytso@....edu>
To:	Andreas Dilger <adilger@...ger.ca>
Cc:	Tony Breeds <tony@...eyournoodle.com>, linux-ext4@...r.kernel.org
Subject: Re: Minimal configuration for e2fsprogs

On Tue, Jun 19, 2012 at 12:01:54AM -0600, Andreas Dilger wrote:
> 
> That is what I would do, though perhaps with macro names that are not
> so confusingly similar to the actual macro names, which might otherwise
> cause confusion if someone doesn't read the code very closely.  Like:
> 
> #ifdef ENABLE_MMP
> ...
> #define EXT2_LIB_INCOMPAT_SUPP_MMP EXT4_FEATURE_INCOMPAT_MMP
> #else
> #define EXT2_LIB_INCOMPAT_SUPP_MMP (0)
> #endif

either name is fine with me; I'm not particularly picky here, since
the only place the #define would get used is the header file, the
opportunities for confusion are pretty limited.

> > 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'd have it print an error of some kind, i.e., "MMP not supported"


> > --- 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 */
> 
> These should all conditionally become static inline no-op functions
> here (returning zero as needed) so that the calling code does not need
> messy #ifdefs everywhere.

If we've removed MMP from the list of supported features, a much
simpler thing we can do is to just add at the beginning of each of the
functions in mmp.c:

#ifndef CONFIG_MMP
	return EXT2_ET_OP_NOT_SUPPORTED;
#endif

and then letting the compiler optimize out the rest of the code in the
function; I wouldn't worry about using static inlines, since it's not
something we really need to optimize in this case.  The mmp functions
don't get called all that often anyway.

I also wouldn't bother commenting out the function signatures in
ext2fs.h.  For programs which are including ext2fs.h, they're not
going to be including the config.h which has CONFIG_* or ENABLE_*
autoconf #defines anyway.

Arguably all of the EXT2_LIB_* defines shouldn't be in ext2fs.h for
that reason; they should be in ext2fsP.h, and the only reason why they
aren't is due to history (since we didn't have ext2fsP.h when they
were first introduced).  At some point I'll probably move that logic
into ext2fsP.h, just to keep ext2fs.h smaller/cleaner for application
programs.

Regards,

						- Ted

P.S.  I'm debating whether or not it makes sense to create a special
static library just for bootloaders which is stripped down and
read-only.  I realized while looking at things that there is a pretty
large amount of coding getting dragged in due to namei.c pulling in
dir_iterate.c pulling in block_iterate.c pulling in extent.c, which
then pulls in a lot of code since we might need to allocate or
deallocate blocks.  If we made a read-only variant of the library, it
could significantly reduce the size of the statically linked
bootloader.  I'm not entirely sure it's worth it, since you don't seem
to be worrying about the compiled binary size of yaboot, but it's
something we could do if it was interesting.  What do you think?
--
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