linux-ext4 - Re: [RFC] [PATCH 1/1] Nanosecond timestamps

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: <20070207203946.GB6565@schatzie.adilger.int>
Date:	Wed, 7 Feb 2007 13:39:46 -0700
From:	Andreas Dilger <adilger@...sterfs.com>
To:	Johann Lombardi <johann.lombardi@...l.net>
Cc:	Kalpak Shah <kalpak@...sterfs.com>,
	linux-ext4 <linux-ext4@...r.kernel.org>, tytso <tytso@....edu>,
	sct <sct@...hat.com>
Subject: Re: [RFC] [PATCH 1/1] Nanosecond timestamps

On Feb 06, 2007  16:12 +0100, Johann Lombardi wrote:
> > +       if (sbi->s_inode_size > EXT3_GOOD_OLD_INODE_SIZE) {
> > +           EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
>
> Maybe EXT3_SB(sb)-> could be replaced by sbi-> here and in the lines below.

Yes, this should definitely be done.  It also increases clarity between
sbi->s_want_extra_isize and es->s_want_extra_isize.

> > +                       if (EXT3_SB(sb)->s_want_extra_isize <
> > +                           le32_to_cpu(es->s_min_extra_isize))
>                                 ^^
> > +                               EXT3_SB(sb)->s_want_extra_isize =
> > +                                       le32_to_cpu(es->s_min_extra_isize);
>                                             ^^
> Since es->s_{min,want}_extra_isize are both __u16 (BTW, shouldn't it be
> __le16?), I think you should use le16_to_cpu() instead of le32_to_cpu().

You are right - this works fine on little endian systems, but fails on
big endian systems where you will get the other half of the word.

This has been a bug in several places already, and I wonder if the
le*_to_cpu() and cpu_to_le*() macros shouldn't do some type checking
instead of just casting the variable to the specified type?

The only problem is if casting constants it would be a bit of a pain
to have to cast them explicitly, though we could have something like:

#define le16_to_cpu(var) (__builtin_constant(var) || !typecheck(__u16, var) ? \
			  __constant_cpu_to_le16(var) : __le16_to_cpu(var))
			   
The only question is whether "typecheck" adds extra variables on the stack
or if the compiler will always optimize them away.

> > +       /* Check if enough inode space is available */
> > +       if (EXT3_GOOD_OLD_INODE_SIZE + EXT3_SB(sb)->s_want_extra_isize >
> > +                                                       sbi->s_inode_size) {
> > +               EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
> > +               printk(KERN_INFO "EXT3-fs: required extra inode space not"
> > +                       "available.\n");
> > +       }
> 
> If the inode size is EXT3_GOOD_OLD_INODE_SIZE, sbi->s_want_extra_isize won't
> be initialized. However, it should not be an issue because the ext3_sb_info
> is set to zero in ext3_fill_super().

So I'm not sure I understand if you have an objection or if this is just a
comment.  sbi->s_want_extra_isize will be zero and it is not possible for
sbi->s_inode_size < EXT3_GOOD_OLD_INODE_SIZE so this case won't be hit.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

-
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