[Crash-utility] Re: [PATCH] Fix infinite loop during module symbols initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Kazu,

On Tue, Dec 10, 2024 at 7:02 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> 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.

Thanks,
Tao Liu

>
> Thanks,
> Kazu
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux