linux-ext4 - Re: Possible circular dependency between i_data

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: <20240715103205.GA38263@system.software.com>
Date: Mon, 15 Jul 2024 19:32:05 +0900
From: Byungchul Park <byungchul@...com>
To: "Vlastimil Babka (SUSE)" <vbabka@...nel.org>
Cc: Theodore Ts'o <tytso@....edu>, Hyeonggon Yoo <42.hyeyoo@...il.com>,
	Linux Memory Management List <linux-mm@...ck.org>,
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
	max.byungchul.park@...com,
	Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
	kernel_team@...ynix.com
Subject: Re: Possible circular dependency between i_data_sem and folio lock
 in ext4 filesystem

On Fri, Jul 12, 2024 at 11:23:36PM +0200, Vlastimil Babka (SUSE) wrote:
> On 7/12/24 7:31 AM, Byungchul Park wrote:
> > On Fri, Jul 12, 2024 at 01:44:20PM +0900, Byungchul Park wrote:
> >> 
> >> What a funny guy...  He did neither 1) insisting it's a bug in your code
> >> nor 3) insisting DEPT is a great tool, but just asking if there's any
> >> locking rules based on the *different acqusition order* between folio
> >> lock and i_data_sem that he observed anyway.
> >> 
> >> I don't think you are a guy who introduces bugs, but the thing is it's
> >> hard to find a *document* describing locking rules.  Anyone could get
> >> fairly curious about the different acquisition order.  It's an open
> >> source project.  You are responsible for appropriate document as well.
> >> 
> >> I don't understand why you act to DEPT like that by the way.  You don't
> >> have to becasue:
> >> 
> >>    1. I added the *EXPERIMENTAL* tag in Kconfig as you suggested, which
> >>       will prevent autotesting until it's considered stable.  However,
> >>       the report from DEPT can be a good hint to someone.
> >> 
> >>    2. DEPT can locate code where needs to be documented even if it's not
> >>       a real bug.  It could even help better documentation.
> >> 
> >> DEPT hurts neither code nor performance unless enabling it.
> 
> enabling means building with CONFIG_DEPT right?

Yes.

> >> > If you want to add lock annotations into the struct page or even
> >> > struct folio, I cordially invite you to try running that by the mm
> >> > developers, who will probably tell you why that is a terrible idea
> >> > since it bloats a critical data structure.
> 
> I doubt anyone will object making struct page larger for a non-production
> debugging config option, which AFAIU DEPT is, i.e. in the same area as
> LOCKDEP or KASAN etc... I can see at least KMSAN already adds some fields to
> struct page already.

I think so.

> >> I already said several times.  Doesn't consume struct page.
> > 
> > Sorry for that.  I've changed the code so the current version consumes
> > it by about two words if enabled.  I can place it to page_ext as before
> > if needed.
> 
> page_ext is useful if you have a debugging feature that can be compiled in
> but adds no overhead (memory, nor cpu thanks to static keys) unless enabled
> on boot time, i.e. page_owner... so for DEPT it seems it would be an
> unnecessary complication.

Yeah, I will think it more.  However, maybe, as you said, it could
introduce a complication.  Thanks.

	Byungchul