* [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.