linux-kernel - Re: [PATCH v2 2/2] tmpfs: Support 64-bit inums per-sb

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] [day] [month] [year] [list]

Message-ID: <20200102201437.GB1181932@chrisdown.name>
Date:   Thu, 2 Jan 2020 20:14:37 +0000
From:   Chris Down <chris@...isdown.name>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Matthew Wilcox <willy@...radead.org>,
        Jeff Layton <jlayton@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Tejun Heo <tj@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>, kernel-team@...com
Subject: Re: [PATCH v2 2/2] tmpfs: Support 64-bit inums per-sb

Amir Goldstein writes:
>> +config TMPFS_INODE64
>> +       bool "Use 64-bit ino_t by default in tmpfs"
>> +       depends on TMPFS && 64BIT
>> +       default n
>> +       help
>> +         tmpfs has historically used only inode numbers as wide as an unsigned
>> +         int. In some cases this can cause wraparound, potentially resulting in
>> +         multiple files with the same inode number on a single device. This option
>> +         makes tmpfs use the full width of ino_t by default, similarly to the
>> +         inode64 mount option.
>> +
>> +         tmpfs mounts that are used privately by the kernel and are not visible to
>> +         users are unaffected.
>
>Admins won't know what the line above means and they shouldn't care.
>It adds no information, so better remove it.

Sure thing.

>> +
>> +       /*
>> +        * Showing inode{64,32} might be useful even if it's the system default,
>> +        * since then people don't have to resort to checking both here and
>> +        * /proc/config.gz to confirm 64-bit inums were successfully applied
>> +        * (which may not even exist if IKCONFIG_PROC isn't enabled).
>> +        *
>> +        * We hide it when inode64 isn't the default and we are using 32-bit
>> +        * inodes, since that probably just means the feature isn't even under
>> +        * consideration.
>> +        *
>> +        * As such:
>> +        *
>> +        *                     +-----------------+-----------------+
>> +        *                     | TMPFS_INODE64=y | TMPFS_INODE64=n |
>> +        *  +------------------+-----------------+-----------------+
>> +        *  | full_inums=true  | show            | show            |
>> +        *  | full_inums=false | show            | hide            |
>> +        *  +------------------+-----------------+-----------------+
>> +        *
>> +        */
>> +       if (IS_ENABLED(CONFIG_TMPFS_INODE64) || !sbinfo->full_inums)
>
>Condition does not match comment - should be || sbinfo->full_inums)

Good catch! Thanks.

>> @@ -3915,6 +3969,7 @@ int shmem_init_fs_context(struct fs_context *fc)
>>         ctx->mode = 0777 | S_ISVTX;
>>         ctx->uid = current_fsuid();
>>         ctx->gid = current_fsgid();
>> +       ctx->full_inums = IS_ENABLED(CONFIG_TMPFS_INODE64);
>>
>
>This is the wrong place for this - it is also being set for the kern_mount.
>Follow the lead of shmem_default_max_inodes.

Hmm, full_inums is intended to be simply ignored for SB_KERNMOUNT though, so it 
seems harmless, but I agree maybe it makes the intent of the code clearer to 
move it to a more specific place.

Thanks! I'll fix these up for v3.