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:   Mon, 4 May 2020 17:00:03 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Jonny Grant <jg@...k.org>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: /fs/ext4/ext4.h add a comment to ext4_dir_entry_2


> On May 4, 2020, at 4:39 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> 
> On Mon, May 04, 2020 at 04:26:35PM -0600, Andreas Dilger wrote:
>> 
>>> On May 3, 2020, at 6:52 AM, Jonny Grant <jg@...k.org> wrote:
>>> 
>>> Hello
>>> 
>>> Could a comment be added to clarify 'file_type' ?
>>> 
>>> struct ext4_dir_entry_2 {
>>>   __le32    inode;            /* Inode number */
>>>   __le16    rec_len;        /* Directory entry length */
>>>   __u8    name_len;        /* Name length */
>>>   __u8    file_type;
>>>   char    name[EXT4_NAME_LEN];    /* File name */
>>> };
>>> 
>>> 
>>> 
>>> This what I am proposing to add:
>>> 
>>>   __u8    file_type;        /* See directory file type macros below */
>> 
>> For this kind of structure field, it makes sense to reference the macro
>> names directly, like:
>> 
>> 	__u8	file_type;	/* See EXT4_FT_* type macros below */
>> 
>> since "macros below" may be ambiguous as the header changes over time.
>> 
>> 
>> Even better (IMHO) is to use a named enum for this, like:
>> 
>>        enum ext4_file_type file_type:8; /* See EXT4_FT_ types below */
>> 
>> /*
>> * Ext4 directory file types.  Only the low 3 bits are used.  The
>> * other bits are reserved for now.
>> */
>> enum ext4_file_type {
>> 	EXT4_FT_UNKNOWN		= 0,
>> 	EXT4_FT_REG_FILE	= 1,
>> 	EXT4_FT_DIR		= 2,
>> 	EXT4_FT_CHRDEV		= 3,
>> 	EXT4_FT_BLKDEV		= 4,
>> 	EXT4_FT_FIFO		= 5,
>> 	EXT4_FT_SOCK		= 6,
>> 	EXT4_FT_SYMLINK		= 7,
>> 	EXT4_FT_MAX,
>> 	EXT4_FT_DIR_CSUM	= 0xDE
>> };
>> 
>> so that the allowed values for this field are clear from the definition.
>> However, the use of a fixed-with bitfield (enum :8) is a GCC-ism and Ted
>> may be against that for portability reasons, since the kernel and
>> userspace headers should be as similar as possible.
> 
> This is an on-disk structure.  Do /not/ make this an enum because that
> would replace a __u8 with an int, which will break directories.

No, that is what the fixed bitfield declaration "enum ... :8" would do -
declare this enum to be an 8-bit integer.  I've verified that this works
as expected with GCC, to allow an enum with a specific size, like :8 or
:32 or :64.  Obviously, if you specify a bitfield size that doesn't align
with the start of the next structure field, there would be padding added
so that the next field is properly aligned, but that isn't the case here.

Since e2fsprogs needs to be portable to other compilers/OS, I'm not sure
if Ted would want the kernel header declaration to be different than the
e2fsprogs header.  I've grown to like using enum for these kind of "flags"
definitions, since they are much more concrete than a bare "int flags"
declaration, and still better than "int flags; /* see EXT4_FT_* below */"
since the enum is a hard compiler linkage vs. just a comment, for the
same reasons that static inline functions are better than CPP macros.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists