linux-kernel - Re: [PATCH 2/4] ext4: fix BUG in ext4_mb_new_inode

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: <ZLq9MROKiyet9Oce@mit.edu>
Date:   Fri, 21 Jul 2023 13:15:29 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Baokun Li <libaokun1@...wei.com>
Cc:     "Ritesh Harjani (IBM)" <ritesh.list@...il.com>,
        linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca, jack@...e.cz,
        ojaswin@...ux.ibm.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH 2/4] ext4: fix BUG in ext4_mb_new_inode_pa() due to
 overflow

On Fri, Jul 21, 2023 at 05:13:13PM +0800, Baokun Li wrote:
> > Doing this with helpers, IMO is not useful as we also saw above.
> 
> I still think it is necessary to add a helper to make the code more concise.
> 
> Ted, do you think the fex_end() helper function is needed here?
> 
> I think we might need your advice to end this discussion. 😅

Having helper functions doesn't bother me all _that_ much --- so long
as they are named appropriately.  The readibility issues start with
the fact that the helper function uses loff_t as opposed to
ext4_lblk_t, and because someone looking at fex_end() would need
additional thinking to figure out what it did.  If renamed it to be
fex_logical_end() and made it take an ext4_lblk_t, I think it would be
better.

The bigger complaint that I have, although it's not the fault of your
patch, is the use of "ext4_free_extent" all over ext4/mballoc.c when
it's much more often used for allocating blocks.  So the name of the
structure and the "fex" in "fex_end" or "fex_logical_end" is also
confusing.

Hmm... how about naming helper function extent_logial_end()?

And at some point we might want to do a global search and replace
changing ext4_free_extent to something like ext4_mballoc_extent, and
changing the structure element names.  Perhaps:

       fe_logical  --->  ex_lblk
       fe_start    --->  ex_cluster_start
       fe_group    --->  ex_group
       fe_len      --->  ex_cluster_len

This is addressing problems where "start" can mean the starting
physical block, the starting logical block, or the starting cluster
relative to the beginning of the block group.

There is also documentation which is just wrong.  For example:

/**
 * ext4_trim_all_free -- function to trim all free space in alloc. group
 * @sb:			super block for file system
 * @group:		group to be trimmed
 * @start:		first group block to examine
 * @max:		last group block to examine

start and max should be "first group cluster to examine" and "last
group cluster to examine", respectively.

The bottom line is that there are much larger opportunities to make
the code more maintainable than worrying about two new helper
functions.  :-)

Cheers,

					- Ted