linux-ext4 - Re: [GIT PULL] dtype handling cleanup for v4.21-rc1

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]

Date:   Wed, 16 Jan 2019 13:07:54 +0100
From:   Jan Kara <jack@...e.cz>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-ext4@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Amir Goldstein <amir73il@...il.com>
Subject: Re: [GIT PULL] dtype handling cleanup for v4.21-rc1

[Added Phillip and Amir to CC (authors)]

On Wed 16-01-19 07:25:01, Linus Torvalds wrote:
> On Wed, Jan 16, 2019 at 6:01 AM Matthew Wilcox <willy@...radead.org> wrote:
> >
> > The ext2/ext4 patches don't show much improvement.  The other patches show
> > more:
> >
> >  fs/nilfs2/dir.c                    | 52 ++++++++++--------------------
> >  include/uapi/linux/nilfs2_ondisk.h |  1 +
> >  2 files changed, 18 insertions(+), 35 deletions(-)
> >
> > (for example).
> >
> > UFS ends up benefiting the most.  You can see the whole diffstat here:
> >
> > https://lore.kernel.org/lkml/20181023201952.GA15676@pathfinder/
> 
> Well, even with _all_ the filesystems converted, you actually have
> more lines added than removed by this "cleanup".
> 
> Sharing code just isn't a win here.
> 
> That said, it's not really the number of lines per se that make me
> question this, I think that's really more of a symptom than the root
> cause. The root cause for the newly adde lines is that this whole
> approach requires that all the numbers are in sync, but then they have
> different *names*.
> 
> Honestly, my gut feel is that I should not pull this in this form.
> 
> I have a suggestion: if people want to do this, and actually share the
> transformation, then the filesystems that use this common code should
> simply *NOT* have their own private names for the enumerations. They
> should actually use those standard names.
> 
> So if the patch for ext2 (for example) were to entirely get rid of the
> whole EXT2_FT_DIR define entirely, and ext2 would just use the actual
> FT_DIR define, than I'd be ok with it. At that point you don't add a
> pointless and expensive abstraction. At that point you say "ext2 uses
> the standard values, so ext2 can just use the standard #defines
> directly".

OK, I'm fine with that. We just have to have a big fat warning at FT_
definitions that these are on-disk values for several filesystems and thus
cannot ever change. As Amir mentioned in another email, the original
motivation for this is that quite a few filesystems copy-pasted ext2 code
and that is slightly buggy. So I still do think there's value in this
cleanup excercise.

> See my argument?
> 
> I think it is completely disgsting to have stuff like this:
> 
> + BUILD_BUG_ON(EXT2_FT_UNKNOWN != FT_UNKNOWN);
> + BUILD_BUG_ON(EXT2_FT_REG_FILE != FT_REG_FILE);
> + BUILD_BUG_ON(EXT2_FT_DIR != FT_DIR);
> + BUILD_BUG_ON(EXT2_FT_CHRDEV != FT_CHRDEV);
> + BUILD_BUG_ON(EXT2_FT_BLKDEV != FT_BLKDEV);
> + BUILD_BUG_ON(EXT2_FT_FIFO != FT_FIFO);
> + BUILD_BUG_ON(EXT2_FT_SOCK != FT_SOCK);
> + BUILD_BUG_ON(EXT2_FT_SYMLINK != FT_SYMLINK);
> + BUILD_BUG_ON(EXT2_FT_MAX != FT_MAX);
> 
> the above is just *garbage*.
> 
> If you fundamentally need the values to be the same, then you simply
> shouldn't have two different set of #defines.
> 
> Get rid of the EXT2_FT_xyz enumeration entirely, and the whole
> craziness goes away.
> 
> > We'd see a lot more improvement in line count if Philip weren't quite
> > so paranoid about checking FOOFS_FT_* == FT_* at build time; eg for btrfs:
> 
> Exact same issue.
> 
> So the more I look at this, the less I like it.
> 
> But if people are actually willing to use *truly* shared code, instead
> of using their own values and then having the crazy "they need to
> match", then it would be a different issue. As it is, I think the
> patch series adds complexity rather than helping anything.
> 
> More complexity and more lines of code? There is absolutely zero upside.

OK, understood. Phillip, could you please rework the patches as Linus
suggests? Thanks!

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR