Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Reply-To: kbingham@kernel.org
Subject: Re: [PATCH] scripts/gdb: fix list_for_each
To:     George Prekas <prekageo@amazon.com>,
        Jan Kiszka <jan.kiszka@siemens.com>,
        linux-kernel@vger.kernel.org
References: <3b382958-9f1d-a3d2-a239-09ba084227e6@amazon.com>
 <2516a051-306f-670b-1f9e-d46fc577c7f8@siemens.com>
 <109fe98d-4143-cfd3-b145-8d5fee189f63@amazon.com>
 <25111834-a414-e380-1e61-c1b1c0e766cb@kernel.org>
 <d694a1f7-e1c9-2631-12e2-a17abaf004da@amazon.com>
From:   Kieran Bingham <kbingham@kernel.org>
Autocrypt: addr=kbingham@kernel.org; keydata=
 mQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat
 V/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC
 rRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C
 potzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ
 cSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf
 Kr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8
 RXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko
 lPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq
 8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36
 Oe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtCRLaWVyYW4gQmlu
 Z2hhbSA8a2JpbmdoYW1Aa2VybmVsLm9yZz6JAlQEEwEKAD4CGwMFCwkIBwIGFQoJCAsCBBYC
 AwECHgECF4AWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTt0QUJCyJXZAAKCRChHkZyEKRh
 /QYVD/95rP50k7PUx8ZzRGlWJtw8pGkWzyohQtkSeDhMYhR5Ud6dVVOjJxdAzSxnzeFDHniW
 plJ4z9hpczgnXpb2WNpccup7YzcpadCHG2M1nVZPqY3Szvfi+vjIm3Aa370FJeuhXgU65aBi
 NQv+lJR5R6qdyEkjT4YLSGf35fdoH4bAGHIKHtZH0iRvGcpt9YrygkGpCREnqHvzjXYBzDm6
 /0/2Qcf0aV0fZMeZ/EhkIL/zy452BRavJ6xJKBbGadm/dIEQsEdzfH4nbcfmsBpL4QdBzwon
 WQesFTVBpGpYIuToX5CB6WyXWnqkfUwcd7riEMciWLxqW82nLpfK96V9Blmumlj5RXjzzsN1
 aYMU8lxyeesEMiUmZDLY34DSP9jTcSZFTQkJ+VkXIgCbM8gXY8hEJ4Y5wYTG5XXDOVmXxO/k
 oR+51rx1gCOdo2jCu2gH84gemZv/Y0MPdL+vOph8AiuEZAUxUglSaLwZoX+5y3tRP9Pwp6Il
 DWlEfDW9s9N7x77Z9UbtgoM7K3BzFv/rhG/PXY+WUjjxQHRQN3GOhVXOtdl+ICijXgmBnOCO
 vB3cPxprqTqOX1mMo/FbckKzLuiNnJX2hPRvGcWgwwhzrTPoVS6DockCI5bketVjEAX4kH3+
 g0C4VZF7UOhTfgKjcUz1FQNsep1UsePjQE81yt6zt7kCDQRWBP1mARAAzijkb+Sau4hAncr1
 JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uztih9fiUbSV3wfsWqg1Ut
 3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYId6MDC417f7vK3hCbCVIZ
 Sp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6mhf0V1YkspE5St814ETX
 pEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXlEn1aulcYyu20dRRxhkQ6
 iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5PKe56IGlpkjc8cO51lIx
 HkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R+rxfAVKM6V769P/hWoRG
 dgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCkNlXqI0W/who0iSVM+8+R
 myY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04/UqCMK/KnX8pwXEMCjz0
 h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1LoeTK396wc+4c3BfiC6pN
 tUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/vSj943LUeqEeRnIQpGH9
 BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmDMJggbwjIotypzIXfhHNC
 eTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj6qOPTd+Uk7NFzL65qkh8
 0ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNzPZ79NAmXLackAx3sOVFh
 k4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUpECzuuRBv8wX4OQl+hbWb
 B/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sxS0A8/atCHUXOboUsn54q
 dxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48jsbEYX0YQnzaj+jO6kJto
 ZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8pQk3kgDu7kb/7PRYrZvB
 sr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXcMW8zs8avFNuA9VpXt0Yu
 pJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPUAfYnB4JBDLmLzBFavQfv
 onSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4WZw01QYHU/GUV/zHJSFk
Message-ID: <0a687830-a334-60ba-7a65-f25342139e08@kernel.org>
Date:   Wed, 23 Sep 2020 22:17:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <d694a1f7-e1c9-2631-12e2-a17abaf004da@amazon.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
Precedence: bulk

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
>