linux-ext4 - Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_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] [day] [month] [year] [list]

Message-ID: <20240816163522.GB8108@mit.edu>
Date: Fri, 16 Aug 2024 12:35:22 -0400
From: "Theodore Ts'o" <tytso@....edu>
To: Markus Elfring <Markus.Elfring@....de>
Cc: "Darrick J. Wong" <djwong@...nel.org>,
        Kemeng Shi <shikemeng@...weicloud.com>, linux-ext4@...r.kernel.org,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

On Fri, Aug 16, 2024 at 08:56:45AM +0200, Markus Elfring wrote:
> >>>> Release inode_bitmap_bh from ext4_read_inode_bitmap in
> >>>> ext4_mark_inode_used to avoid buffer_head leak.
> >>>> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
> >>>> is NULL.
> >>>
> >>> 1. I suggest to split such changes into separate update steps.
> >>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81
> >> It's acceptable to me, but I'm not sure if it worth separate patches
> >> to others. I will do separate in next version if no person is against
> >> this.
> >
> > No, that suggestion is stupid.
> 
> Please reconsider such a view a bit more.

Darrick is absolutely correct; that suggestion is.... ill-considered.

> >                                 There's no reason to generate even more
> > patches for a three line fix, it's very obvious that you're fixing a
> > missing resource release and rearranging the first error out
> > accordingly.
> 
> You would probably like to distinguish the severity for two changes,
> wouldn't you?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n168
> 
> Under which circumstances can you accept the separation of development concerns better?

We will sometimes do minor cleanups in the course of fixing a bug.  In
this particular case, the cleanup is so minor, that if someone
suggested it as a stand-alone cleanup patch, I'd reject it as adding
noise, and not being worth the extra commit.

Blindly following rules is a bad idea; that's because software
programming is an engineering discpline, which means we are often
trading off multiple goals, each of which are good in and of
themselves.  For example, extra patch noise, such as fixing
whitespace, or changing a goto errout to a return, makes zero
difference to the generated code, only a very tiny margial improvement
in the readability in the code base; and also increases the chance
that some future bug fix won't backport cleanly to older LTS kernels.

I expect ext4 developers to use their good judgement, and not just
blindly follow rules, even good rules which may make sense 80% or even
95% of the time in the submitting-patches.rst file.

Markus, perhaps you could good "blindly following rules" and read some
of the eassays found from that web search, if you need more
explorations of that topic.

Best regards, 

						- Ted