On Wed, Dec 11, 2024 at 5:21 AM Tao Liu <l...@redhat.com> wrote:
> Hi Kazu,
>
> On Tue, Dec 10, 2024 at 7:02 PM HAGIO KAZUHITO(萩尾 一仁)
> <k-hagio...@nec.com> wrote:
> >
> > Hi Tao,
> >
> > On 2024/12/10 12:39, Tao Liu wrote:
> >
> > > I can also reproduce this issue on 6.13.0-rc2 machine. The
> > > mod_symname_hash_install() function missed the case if one spn was
> > > installed 2 more times, which as a result, will cause the infinite
> > > loop.
> > >
> > > The reason for installing 2 more times is because the addresses
> > > overlap between 2 kernel modules. See the following:
> > >
> > > crash> sym -l
> > > ffffffffc0800000 MODULE TEXT START: qemu_fw_cfg
> > > ffffffffc0800000 (t) __pfx_fw_cfg_sysfs_attr_show
> > > ffffffffc0800010 (t) fw_cfg_sysfs_attr_show
> > > ...
> > > ffffffffc0801000 MODULE TEXT START: serio_raw
> > > ffffffffc0801000 (t) __pfx_serio_raw_poll
> > > ffffffffc0801000 MODULE TEXT END: qemu_fw_cfg
> > > ...
> > > ffffffffc0802000 MODULE TEXT END: serio_raw
> > >
> > > For module qemu_fw_cfg, 3 symbols of ffffffffc0801000 will be
> > > installed. Then for module serio_raw, 3 symbols of ffffffffc0801000
> > > will be reinstalled. Due to the lack of repeat symbol check, there
> > > will be symbol reinstall, and infinite loop:
> > >
> > > sp->name_hash_next = spn; <<----- name_hash_next point to itself if
> sp==spn
> > >
> > > I think Kazu's "base + size - 1" solution is reasonable, this will let
> > > qemu_fw_cfg install range from ffffffffc0800000 to
> > > (ffffffffc0801000-1), so no overlap for 2 modules. In addition, I'd
> > > like to add the following code as well, to skip if a symbol has
> > > already been installed:
> > >
> > > diff --git a/symbols.c b/symbols.c
> > > index a26bdc1..e1a62bb 100644
> > > --- a/symbols.c
> > > +++ b/symbols.c
> > > @@ -1233,6 +1233,9 @@ mod_symname_hash_install(struct syment *spn)
> > > return;
> > > }
> > > for (; sp; sp = sp->name_hash_next) {
> > > + if (spn == sp)
> > > + return;
> > > +
> > > if (!sp->name_hash_next ||
> > > spn->value < sp->name_hash_next->value) {
> > > spn->name_hash_next = sp->name_hash_next;
> > >
> > > What do you think?
> >
> > thank you for checking the issue, it's ok to add it for future safety.
>
> Thanks for your comments, so ack for the above code changes.
>
>
Applied(with the code changes):
https://github.com/crash-utility/crash/commit/e44a9a9d808c83fb846060f65e5aaa9d30b6e2c4
> Thanks,
> Tao Liu
>
> >
> > Thanks,
> > Kazu
>
>
--
Crash-utility mailing list -- devel@lists.crash-utility.osci.io
To unsubscribe send an email to devel-le...@lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
