linux-kernel - Re: [PATCH v3] riscv: patch: Fixup lockdep warning in stop

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y9pwo5iC7hrPm/wk@wendy>
Date:   Wed, 1 Feb 2023 14:01:07 +0000
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Changbin Du <changbin.du@...wei.com>
CC:     Guo Ren <guoren@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Hui Wang <hw.huiwang@...wei.com>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        Changbin Du <changbin.du@...il.com>,
        Zong Li <zong.li@...ive.com>
Subject: Re: [PATCH v3] riscv: patch: Fixup lockdep warning in stop_machine

On Thu, Feb 02, 2023 at 05:00:31AM +0800, Changbin Du wrote:
> On Tue, Jan 31, 2023 at 07:50:20AM +0000, Conor Dooley wrote:
> > On Tue, Jan 31, 2023 at 03:26:33PM +0800, Guo Ren wrote:
> [snip]
> > > > >
> > > > > -     /*
> > > > > -      * Before reaching here, it was expected to lock the text_mutex
> > > > > -      * already, so we don't need to give another lock here and could
> > > > > -      * ensure that it was safe between each cores.
> > > > > -      */
> > > > > -     lockdep_assert_held(&text_mutex);
> > > >
> > > > I must admit, patches like this do concern me a little, as a someone
> > > > unfamiliar with the world of probing and tracing.
> > > > Seeing an explicit check that the lock was held, leads me to believe
> > > > that the original author (Zong Li I think) thought that the text_mutex
> > > > lock was insufficient.
> > > > Do you think that their fear is unfounded? Explaining why it is safe to
> > > > remove this assertion in the commit message would go a long way towards
> > > > easing my anxiety!
> > > >
> > > > Also, why delete the comment altogether? The comment provides some
> > > > information that doesn't appear to become invalid, even with the
> > > > assertion removed?
> > > Stop_machine separated the mutex context and made a lockdep warning.
> > > So text_mutex can't be used here. We need to find another check
> > > solution. I agree with the patch.
> > 
> > Whether or not you agree with the change is not the point (with your SoB
> > I'd hope you agree with it).
> > I understand that you two are trying to fix a false positive lockdep
> > warning, but what I am asking for an explanation as to why the original
> > author's fear is unfounded.
> > Surely, having added the assertion, they were not thinking of the same
> > code path that you guys are hitting the false positive on?
> > 
> The assertion is reasonable since the fixmap entry is shared. The text_mutex
> does should be held before entering that function. But the false positive cases
> make some functions (ftrace for example) difficult to use due to warning log
> storm.
> 
> Either the lockdep should be fixed for stop_machine, or remove the assertion
> simply now (we can keep the comments). (or do the assertion conditionly?)

How would you suggest checking it conditionally?

Also, if you persist in removing the assertion, there is a comment in
arch/riscv/kernel/ftrace.c that would need to be updated. (L129-ish)

The comment you removed in this patch seems valid both before and after
though, so I don't see a compelling reason for its removal.

> And this is not a riscv only problem but common for architectures which use
> stop_machine to patch text. (arm for example)
> 
> > Perhaps Zong themselves can tell us what the original fear was?


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)