* [PATCH] scripts/gdb: fix list_for_each @ 2020-09-22 14:28 George Prekas 2020-09-22 14:32 ` Jan Kiszka 0 siblings, 1 reply; 6+ messages in thread From: George Prekas @ 2020-09-22 14:28 UTC (permalink / raw) To: linux-kernel; +Cc: Jan Kiszka, Kieran Bingham If the next pointer is NULL, list_for_each gets stuck in an infinite loop. Signed-off-by: George Prekas <prekageo@amazon.com> --- scripts/gdb/linux/lists.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py index c487ddf09d38..424a91c1aa8b 100644 --- a/scripts/gdb/linux/lists.py +++ b/scripts/gdb/linux/lists.py @@ -27,6 +27,8 @@ def list_for_each(head): raise TypeError("Must be struct list_head not {}" .format(head.type)) + if head['next'] == 0: + return node = head['next'].dereference() while node.address != head.address: yield node.address -- 2.24.3.AMZN ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scripts/gdb: fix list_for_each 2020-09-22 14:28 [PATCH] scripts/gdb: fix list_for_each George Prekas @ 2020-09-22 14:32 ` Jan Kiszka 2020-09-22 17:17 ` Prekas, George 0 siblings, 1 reply; 6+ messages in thread From: Jan Kiszka @ 2020-09-22 14:32 UTC (permalink / raw) To: George Prekas, linux-kernel; +Cc: Kieran Bingham On 22.09.20 16:28, George Prekas wrote: > If the next pointer is NULL, list_for_each gets stuck in an infinite > loop. > > Signed-off-by: George Prekas <prekageo@amazon.com> > --- > scripts/gdb/linux/lists.py | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py > index c487ddf09d38..424a91c1aa8b 100644 > --- a/scripts/gdb/linux/lists.py > +++ b/scripts/gdb/linux/lists.py > @@ -27,6 +27,8 @@ def list_for_each(head): > raise TypeError("Must be struct list_head not {}" > .format(head.type)) > > + if head['next'] == 0: > + return > node = head['next'].dereference() > while node.address != head.address: > yield node.address Obviously, infinite loops are bad and should be avoided. But NULL is bug, isn't it? Shouldn't we report such a corruption? Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scripts/gdb: fix list_for_each 2020-09-22 14:32 ` Jan Kiszka @ 2020-09-22 17:17 ` Prekas, George 2020-09-22 19:11 ` Kieran Bingham 0 siblings, 1 reply; 6+ messages in thread From: Prekas, George @ 2020-09-22 17:17 UTC (permalink / raw) To: Jan Kiszka, linux-kernel; +Cc: Kieran Bingham On 9/22/2020 9:32 AM, Jan Kiszka wrote: > > On 22.09.20 16:28, George Prekas wrote: >> If the next pointer is NULL, list_for_each gets stuck in an infinite >> loop. >> >> Signed-off-by: George Prekas <prekageo@amazon.com> >> --- >> scripts/gdb/linux/lists.py | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py >> index c487ddf09d38..424a91c1aa8b 100644 >> --- a/scripts/gdb/linux/lists.py >> +++ b/scripts/gdb/linux/lists.py >> @@ -27,6 +27,8 @@ def list_for_each(head): >> raise TypeError("Must be struct list_head not {}" >> .format(head.type)) >> >> + if head['next'] == 0: >> + return >> node = head['next'].dereference() >> while node.address != head.address: >> yield node.address > > Obviously, infinite loops are bad and should be avoided. But NULL is > bug, isn't it? Shouldn't we report such a corruption? > Hi Jan, Is it a bug? Or does it mean that the list is empty? Let me give some background. If you do the following: $ qemu-system-x86_64 -nographic -m 1024 -kernel build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null & $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set auto-load safe-path /' -ex 'lx-symbols' You will see: loading vmlinux scanning for modules in /home/ubuntu/linux-5.8.10 no module object found for '' And the last line repeats forever. This happens because modules.next == NULL. This is the Python stack trace: File ".../symbols.py", line 174, in invoke self.load_all_symbols() File ".../symbols.py", line 161, in load_all_symbols [self.load_module_symbols(module) for module in module_list] File ".../symbols.py", line 161, in <listcomp> [self.load_module_symbols(module) for module in module_list] File ".../modules.py", line 30, in module_list for module in lists.list_for_each_entry(modules, module_ptr_type, "list"): File ".../lists.py", line 41, in list_for_each_entry for node in list_for_each(head): File ".../lists.py", line 31, in list_for_each traceback.print_stack() This patch tries to fix the above problem. George ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scripts/gdb: fix list_for_each 2020-09-22 17:17 ` Prekas, George @ 2020-09-22 19:11 ` Kieran Bingham 2020-09-23 13:13 ` George Prekas 0 siblings, 1 reply; 6+ messages in thread From: Kieran Bingham @ 2020-09-22 19:11 UTC (permalink / raw) To: Prekas, George, Jan Kiszka, linux-kernel Hi George, On 22/09/2020 18:17, Prekas, George wrote: > > On 9/22/2020 9:32 AM, Jan Kiszka wrote: >> >> On 22.09.20 16:28, George Prekas wrote: >>> If the next pointer is NULL, list_for_each gets stuck in an infinite >>> loop. >>> >>> Signed-off-by: George Prekas <prekageo@amazon.com> >>> --- >>> scripts/gdb/linux/lists.py | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py >>> index c487ddf09d38..424a91c1aa8b 100644 >>> --- a/scripts/gdb/linux/lists.py >>> +++ b/scripts/gdb/linux/lists.py >>> @@ -27,6 +27,8 @@ def list_for_each(head): >>> raise TypeError("Must be struct list_head not {}" >>> .format(head.type)) >>> >>> + if head['next'] == 0: >>> + return >>> node = head['next'].dereference() >>> while node.address != head.address: >>> yield node.address >> >> Obviously, infinite loops are bad and should be avoided. But NULL is >> bug, isn't it? Shouldn't we report such a corruption? >> > > Hi Jan, > > Is it a bug? Or does it mean that the list is empty? A correctly initialised (empty) list_head has the next, and prev pointers pointing to itself See: https://elixir.bootlin.com/linux/latest/source/include/linux/list.h#L33 You can see that the implementation of list_empty() checks for this at: https://elixir.bootlin.com/linux/latest/source/include/linux/list.h#L280 /** * list_empty - tests whether a list is empty * @head: the list to test. */ static inline int list_empty(const struct list_head *head) { return READ_ONCE(head->next) == head; } > Let me give some background. If you do the following: > > $ qemu-system-x86_64 -nographic -m 1024 -kernel > build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null & > $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set > auto-load safe-path /' -ex 'lx-symbols' I suspect this is trying to load modules before the kernel is actually fully loaded and running, so nothing is yet initialised. > You will see: > > loading vmlinux > scanning for modules in /home/ubuntu/linux-5.8.10 > no module object found for '' > > And the last line repeats forever. This happens because modules.next == > NULL. This is the Python stack trace: > > File ".../symbols.py", line 174, in invoke > self.load_all_symbols() > File ".../symbols.py", line 161, in load_all_symbols > [self.load_module_symbols(module) for module in module_list] > File ".../symbols.py", line 161, in <listcomp> > [self.load_module_symbols(module) for module in module_list] > File ".../modules.py", line 30, in module_list > for module in lists.list_for_each_entry(modules, module_ptr_type, > "list"): > File ".../lists.py", line 41, in list_for_each_entry > for node in list_for_each(head): > File ".../lists.py", line 31, in list_for_each > traceback.print_stack() > > This patch tries to fix the above problem. Does it fix it for you ? I expect it allows the boot process to continue, but the lx-symbols command will not have completed successfully (or rather I expect it will not have found anything to load). I suspect adding defensive checks in here might be helpful but I think the reality is the code is being called at the wrong time. The fact that it 'can' be called at the wrong time is where we might need to be more defensive. > George -- Kieran ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scripts/gdb: fix list_for_each 2020-09-22 19:11 ` Kieran Bingham @ 2020-09-23 13:13 ` George Prekas 2020-09-23 21:17 ` Kieran Bingham 0 siblings, 1 reply; 6+ messages in thread From: George Prekas @ 2020-09-23 13:13 UTC (permalink / raw) To: kbingham, Jan Kiszka, linux-kernel Hi Kieran, On 9/22/2020 2:11 PM, Kieran Bingham wrote: > Hi George, > > On 22/09/2020 18:17, Prekas, George wrote: >> >> On 9/22/2020 9:32 AM, Jan Kiszka wrote: >>> >>> On 22.09.20 16:28, George Prekas wrote: >>>> If the next pointer is NULL, list_for_each gets stuck in an infinite >>>> loop. >>>> >>>> Signed-off-by: George Prekas <prekageo@amazon.com> >>>> --- >>>> scripts/gdb/linux/lists.py | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py >>>> index c487ddf09d38..424a91c1aa8b 100644 >>>> --- a/scripts/gdb/linux/lists.py >>>> +++ b/scripts/gdb/linux/lists.py >>>> @@ -27,6 +27,8 @@ def list_for_each(head): >>>> raise TypeError("Must be struct list_head not {}" >>>> .format(head.type)) >>>> >>>> + if head['next'] == 0: >>>> + return >>>> node = head['next'].dereference() >>>> while node.address != head.address: >>>> yield node.address >>> >>> Obviously, infinite loops are bad and should be avoided. But NULL is >>> bug, isn't it? Shouldn't we report such a corruption? >>> >> >> Hi Jan, >> >> Is it a bug? Or does it mean that the list is empty? > > A correctly initialised (empty) list_head has the next, and prev > pointers pointing to itself > You are right, actually. > >> Let me give some background. If you do the following: >> >> $ qemu-system-x86_64 -nographic -m 1024 -kernel >> build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null & >> $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set >> auto-load safe-path /' -ex 'lx-symbols' > > I suspect this is trying to load modules before the kernel is actually > fully loaded and running, so nothing is yet initialised. > > >> You will see: >> >> loading vmlinux >> scanning for modules in /home/ubuntu/linux-5.8.10 >> no module object found for '' >> >> And the last line repeats forever. This happens because modules.next == >> NULL. This is the Python stack trace: >> >>[...] >> >> This patch tries to fix the above problem. > > Does it fix it for you ? > > I expect it allows the boot process to continue, but the lx-symbols > command will not have completed successfully (or rather I expect it will > not have found anything to load). > > I suspect adding defensive checks in here might be helpful but I think > the reality is the code is being called at the wrong time. > > The fact that it 'can' be called at the wrong time is where we might > need to be more defensive. > At that point in time, the kernel has not even started so it does not have any loaded modules. In fact, as you said, the modules linked list is uninitialized. So with this patch, lx-symbols does not get stuck in an infinite loop and loads only the vmlinux symbols. Maybe, I should rephrase the commit message to say that list_for_each gets stuck in an infinite loop on uninitialized linked lists. Do you think that list_for_each should handle uninitialized lists? If yes, how do you propose to handle them? 1. Treat them as empty lists (this patch). 2. Print a warning and treat them as empty lists. 3. Raise exception and treat them as empty lists. I would go with option 1. For traversal purposes an uninitialized list is the same as an empty list; it has no elements. I am happy, though, to change the patch to another option if you believe it would be better. -- George ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scripts/gdb: fix list_for_each 2020-09-23 13:13 ` George Prekas @ 2020-09-23 21:17 ` Kieran Bingham 0 siblings, 0 replies; 6+ messages in thread From: Kieran Bingham @ 2020-09-23 21:17 UTC (permalink / raw) To: George Prekas, Jan Kiszka, linux-kernel On 23/09/2020 14:13, George Prekas wrote: > Hi Kieran, > > On 9/22/2020 2:11 PM, Kieran Bingham wrote: >> Hi George, >> >> On 22/09/2020 18:17, Prekas, George wrote: >>> >>> On 9/22/2020 9:32 AM, Jan Kiszka wrote: >>>> >>>> On 22.09.20 16:28, George Prekas wrote: >>>>> If the next pointer is NULL, list_for_each gets stuck in an infinite >>>>> loop. >>>>> >>>>> Signed-off-by: George Prekas <prekageo@amazon.com> >>>>> --- >>>>> scripts/gdb/linux/lists.py | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py >>>>> index c487ddf09d38..424a91c1aa8b 100644 >>>>> --- a/scripts/gdb/linux/lists.py >>>>> +++ b/scripts/gdb/linux/lists.py >>>>> @@ -27,6 +27,8 @@ def list_for_each(head): >>>>> raise TypeError("Must be struct list_head not {}" >>>>> .format(head.type)) >>>>> >>>>> + if head['next'] == 0: >>>>> + return >>>>> node = head['next'].dereference() >>>>> while node.address != head.address: >>>>> yield node.address >>>> >>>> Obviously, infinite loops are bad and should be avoided. But NULL is >>>> bug, isn't it? Shouldn't we report such a corruption? >>>> >>> >>> Hi Jan, >>> >>> Is it a bug? Or does it mean that the list is empty? >> >> A correctly initialised (empty) list_head has the next, and prev >> pointers pointing to itself >> > > You are right, actually. > >> >>> Let me give some background. If you do the following: >>> >>> $ qemu-system-x86_64 -nographic -m 1024 -kernel >>> build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null & >>> $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set >>> auto-load safe-path /' -ex 'lx-symbols' >> >> I suspect this is trying to load modules before the kernel is actually >> fully loaded and running, so nothing is yet initialised. >> >> >>> You will see: >>> >>> loading vmlinux >>> scanning for modules in /home/ubuntu/linux-5.8.10 >>> no module object found for '' >>> >>> And the last line repeats forever. This happens because modules.next == >>> NULL. This is the Python stack trace: >>> >>>[...] >>> >>> This patch tries to fix the above problem. >> >> Does it fix it for you ? >> >> I expect it allows the boot process to continue, but the lx-symbols >> command will not have completed successfully (or rather I expect it will >> not have found anything to load). >> >> I suspect adding defensive checks in here might be helpful but I think >> the reality is the code is being called at the wrong time. >> >> The fact that it 'can' be called at the wrong time is where we might >> need to be more defensive. >> > > At that point in time, the kernel has not even started so it does not > have any loaded modules. In fact, as you said, the modules linked list > is uninitialized. So with this patch, lx-symbols does not get stuck in > an infinite loop and loads only the vmlinux symbols. > > Maybe, I should rephrase the commit message to say that list_for_each > gets stuck in an infinite loop on uninitialized linked lists. > > Do you think that list_for_each should handle uninitialized lists? If > yes, how do you propose to handle them? > > 1. Treat them as empty lists (this patch). > 2. Print a warning and treat them as empty lists. > 3. Raise exception and treat them as empty lists. > > I would go with option 1. For traversal purposes an uninitialized list > is the same as an empty list; it has no elements. I am happy, though, to > change the patch to another option if you believe it would be better. I would choose 2 personally. While debugging, if anyone hits an uninitialised linked-list - that's a problem they want to know about, not ignore. -- Kieran > -- > George > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-23 21:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 14:28 [PATCH] scripts/gdb: fix list_for_each George Prekas 2020-09-22 14:32 ` Jan Kiszka 2020-09-22 17:17 ` Prekas, George 2020-09-22 19:11 ` Kieran Bingham 2020-09-23 13:13 ` George Prekas 2020-09-23 21:17 ` Kieran Bingham
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.