Re: [PATCH] LoongArch: Fix lockdep static memory detection

[WANG Xuerui]

Hi,

On 9/15/23 22:07, Guenter Roeck wrote:
> Hi Helge,
>
> On 9/15/23 03:10, Helge Deller wrote:
> > On 9/15/23 11:23, Huacai Chen wrote:
> > > On Fri, Sep 15, 2023 at 4:16 PM Helge Deller <deller@xxxxxx> wrote:
> > > > On 9/15/23 05:22, Huacai Chen wrote:
> > > > > Hi Helge,
> > > > >
> > > > > On Wed, Sep 13, 2023 at 3:18 AM Helge Deller <deller@xxxxxx> wrote:
> > > > > > Since commit 0a6b58c5cd0d ("lockdep: fix static memory detection 
> > > > > > even
> > > > > > more") the lockdep code uses is_kernel_core_data(), 
> > > > > > is_kernel_rodata()
> > > > > > and init_section_contains() to verify if a lock is located inside a
> > > > > > kernel static data section.
> > > > > >
> > > > > > This change triggers a failure on LoongArch, for which the 
> > > > > > vmlinux.lds.S
> > > > > > script misses to put the locks (as part of in the .data.rel symbols)
> > > > > > into the Linux data section.
> > > > > > This patch fixes the lockdep problem by moving *(.data.rel*) symbols
> > > > > > into the kernel data section (from _sdata to _edata).
> > > > > >
> > > > > > Additionally, move other wrongly assigned symbols too:
> > > > > > - altinstructions into the _initdata section,
> > > >
> > > > > I think altinstructions cannot  be put into _initdata because it will
> > > > > be used by modules.
> > > >
> > > > No.
> > > > arch/loongarch/kernel/vmlinux.lds.S is used for the static parts of 
> > > > the kernel
> > > > and altinstructions are replaced before modules are loaded.
> > > > For altinstructions in modules the linker script 
> > > > scripts/module.lds.S is used.
> >
> > > OK, then what about .got/.plt? It seems arm64 also doesn't put them in
> > > the data section.
> >
> > arm64 seems to throw away all plt entries already at link time (and 
> > just keeps
> > the got.plt in the read-only data section).
> > It even checks at link time, that there are no plt entries in the 
> > binary:
> >          ASSERT(SIZEOF(.plt) == 0, "Unexpected run-time procedure 
> > linkages detected!")
> >
> > I don't know for loongarch, but if you need the plt entries for 
> > loongarch, it's
> > safest & best to put them into the read-only data section too, which 
> > is what my patch does.
> > Up to now, you have them completely outside of code & data sections.
> >
> > In the end you need to decide for your platform. My patch is a 
> > suggestion, which I think
> > is correct (untested by me, but Guenter replied he tested it).
> > But to fix the lockdep problem at minimum the move of the .data.rel 
> > section
> > is needed.
>
> Just my $0.02 .. it might make sense to concentrate on the minimum to 
> get the immediate
> problem fixed. Loongarch maintainers can then decide at their own pace 
> if they want
> to apply any of the other changes you suggested. After all, unless I 
> am missing
> something, those additional changes are not really needed in stable 
> releases.

Sorry for coming late, but as reviewer of arch/loongarch, I'd agree with 
Guenter and Helge here: let's fix the immediate problem and investigate 
the rest later -- it's not like the problems are *definitely* orthogonal 
in this case, and at least *some* progress would be appreciated.

I'll try to reproduce the problem and test the fix during the weekend, 
so hopefully Huacai can get the fix in before -rc2 or -rc3. Thanks for 
the attention and fix.

--
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/