linux-ext4 - Re: [PATCH v4 5/7] ext2fs: Add helper functions to access inode numbers

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:   Thu, 17 May 2018 13:55:53 +0300
From:   Artem Blagodarenko <artem.blagodarenko@...il.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>, adilger.kernel@...ger.ca,
        Theodore Ts'o <tytso@....edu>,
        Alexey Lyashkov <alexey.lyashkov@...il.com>
Subject: Re: [PATCH v4 5/7] ext2fs: Add helper functions to access inode
 numbers

Hello Theodore,

I need your suggestions. There are many e2fsprogs libs interfaces which use ext2_ino_t type for parameters. 
Obviously, we need save previous 32-bit versions of this interfaces for already compiled code and add new 64-bit inodes  somehow. 
I tried Andreas’ idea. Changed ext2_ino_t to 64bit and added ext2_ino32_t, but things become too complicated.
ext2_ino_t is quite popular type. Adding two versions of all this code is quite large patch. I am not sure you will be agree to accept it.

So I want to discuss some alternatives:

1) We could compile and link two lib versions: one with 32bit ext2_ino_t and another with 64bit.
Pass some macross that says “ext2_ino_t” is *bit now. User can link both libraries
(functions have same names, but different prototypes). I believe some extra cleanup is needed.
 There are some local variables and functions parameters which have type “bitness” hardcoded. 
But probably this less work then make both interface versions.

2) we could use LD Version Scripts
https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html
This approach looks elegant, but still need more work to be done

Any other ideas? 

Thanks,

Artem Blagodarenko.

> On 4 May 2018, at 12:34, Andreas Dilger <adilger@...ger.ca> wrote:
> 
> On May 4, 2018, at 1:09 AM, c17828 <artem.blagodarenko@...il.com> wrote:
>> 
>> From: Artem Blagodarenko <artem.blagodarenko@...il.com>
>> 
>> 64-bit inodes counter uses extra fields to store hight part.
>> Let's incapsulate inode number reading and writing to extend
>> counter in next commits.
>> 
>> Lustre-bug: https://jira.hpdd.intel.com/browse/LU-9309
>> Signed-off-by: Artem Blagodarenko <artem.blagodarenko@...il.com>
>> ---
> 
> So this patch appears fine by itself, but I don't see in the 6/7 patch
> where all of the interfaces in lib/ext2fs/ext2fs.h that use ext2fs_ino_t
> are updated to add a new "version 2" interface that takes ext2fs_ino64_t
> instead?
> 
> This would otherwise break ABI compatibility, and likely also apps
> that are using the old interfaces with a 32-bit inode number.
> 
> It seems to me that you have fixed the places where there can be a
> 64-bit *count* of inodes, but you haven't fixed all of the places that
> have a 64-bit inode *number* (i.e. passed as a parameter to a function).
> 
> I think one good option would be to introduce a new "ext2fs_ino32_t"
> and *not* add "ext2fs_ino64_t".  Then change the ext2fs_ino_t to be
> 64-bit and make the old interfaces use ext2fs_ino32_t.  That way newly
> compiled applications using libext2fs will use the new 64-bit interfaces
> and the old ones would just need the old interfaces for compatibility.
> I'm not sure what Ted thinks about that, however.
> 
> I believe there is also a "library ABI checker" tool somewhere in the
> build system, but I don't know how to activate it.
> 
> Cheers, Andreas
> 
> 
> 
> 
>