linux-kernel - Re: [mm] 408579cd62: WARNING:suspicious_RCU

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: <20230704152548.afqtyylbrxzunq55@revolver>
Date:   Tue, 4 Jul 2023 11:25:48 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Oliver Sang <oliver.sang@...el.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>, lkp@...ts.01.org,
        lkp@...el.com
Subject: Re: [mm] 408579cd62: WARNING:suspicious_RCU_usage

* Oliver Sang <oliver.sang@...el.com> [230704 09:51]:
> hi, Linus,
> 
> On Mon, Jul 03, 2023 at 07:29:48PM -0700, Linus Torvalds wrote:
> > On Mon, 3 Jul 2023 at 18:48, Oliver Sang <oliver.sang@...el.com> wrote:
> > >
> > > by patch [1], we found the warning is not fixed.
> > 
> > Hmm. I already committed that "fix" as obvious, since the main
> > difference in commit 408579cd627a ("mm: Update do_vmi_align_munmap()
> > return semantics") around that validate_mm() call was how it did that
> > mmap_read_unlock().
> > 
> > > we also found there are some changes in stack backtrace. now it's as below:
> > > (detail dmesg is attached)
> > >
> > > [   26.412372][    T1] stack backtrace:
> > > [   26.412846][    T1] CPU: 0 PID: 1 Comm: systemd Not tainted 6.4.0-09908-gcb226fb1fb7a #1
> > > [   26.413506][    T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> > > [   26.414326][    T1] Call Trace:
> > > [   26.414605][    T1]  <TASK>
> > > [   26.414847][    T1]  dump_stack_lvl+0x73/0xc0
> > > [   26.415225][    T1]  lockdep_rcu_suspicious+0x1b7/0x280
> > > [   26.415669][    T1]  mas_start+0x280/0x400
> > > [   26.416037][    T1]  mas_find+0x27a/0x400
> > > [   26.416391][    T1]  validate_mm+0x8b/0x2c0
> > > [   26.416757][    T1]  __se_sys_brk+0xa35/0xc00
> > 
> > Ok, that is indeed a very different stack trace.
> > 
> > So maybe the fix is a real fix, but the first complaint shut up
> > lockdep, so this is the *second* and unrelated complaint.
> > 
> > And indeed: it turns out that do_vma_munmap() does this:
> > 
> >         ret = do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
> >         validate_mm(mm);
> > 
> > and so we have *another* validate_mm() that is now done outside the lock.
> > 
> > That one is actually pretty pointless. We've *just* validated the mm
> > already inside do_vmi_align_munmap(), except we only did it in one of
> > the two return cases.
> > 
> > So I think the fix is to just move that validate_mm() into the other
> > return case of do_vmi_align_munmap(), and remove it from the caller.
> > 
> > IOW, something like the attached (NOTE! This is in *addition* to the
> > previous patch, which is the same as the one you quoted, just with
> > slightly different whitespace as commit ae80b4041984: "mm: validate
> > the mm before dropping the mmap lock").
> 
> Thanks a lot for guidance!
> I applied below patch directly upon ae80b4041984, and confirmed the
> WARNING gone. Thanks
> 

Thanks for testing this.

I can clean more of this up now that the mmap locking has been changed.
For instance, we can drop a number of checks before a write (and all
read cases, if any remain) since there is no alteration without the
write lock.

Thanks,
Liam