microbit_i2c: Fix coredump when dump-vmstate

[PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peng Liang 3 years, 6 months ago

VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
However, microbit_i2c_vmstate doesn't follow it.  Let's change it.

Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection")
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 hw/i2c/microbit_i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
index 802473982082..e92f9f84ea81 100644
--- a/hw/i2c/microbit_i2c.c
+++ b/hw/i2c/microbit_i2c.c
@@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS),
         VMSTATE_UINT32(read_idx, MicrobitI2CState),
+        VMSTATE_END_OF_LIST()
     },
 };
 
-- 
2.26.2

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 10/19/20 11:34 AM, Peng Liang wrote:
> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.

It might be easy to add a Coccinelle script to avoid future errors.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection")
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>   hw/i2c/microbit_i2c.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
> index 802473982082..e92f9f84ea81 100644
> --- a/hw/i2c/microbit_i2c.c
> +++ b/hw/i2c/microbit_i2c.c
> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate = {
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS),
>           VMSTATE_UINT32(read_idx, MicrobitI2CState),
> +        VMSTATE_END_OF_LIST()
>       },
>   };
>   
>

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peng Liang 3 years, 6 months ago

On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
> On 10/19/20 11:34 AM, Peng Liang wrote:
>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
> 
> It might be easy to add a Coccinelle script to avoid future errors.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>>
>> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer
>> detection")
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>   hw/i2c/microbit_i2c.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
>> index 802473982082..e92f9f84ea81 100644
>> --- a/hw/i2c/microbit_i2c.c
>> +++ b/hw/i2c/microbit_i2c.c
>> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate
>> = {
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState,
>> MICROBIT_I2C_NREGS),
>>           VMSTATE_UINT32(read_idx, MicrobitI2CState),
>> +        VMSTATE_END_OF_LIST()
>>       },
>>   };
>>  
> 
> 
> .

Thanks! I'll try to add one.

-- 
Thanks,
Peng

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peng Liang 3 years, 6 months ago

On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
> On 10/19/20 11:34 AM, Peng Liang wrote:
>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
> 
> It might be easy to add a Coccinelle script to avoid future errors.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
end of VMStateDescription.fields.  For those who are not defined as
compound literals, it works well.  However, I cannot make it work for
those defined as compound literals.  And Julia doesn't think compound
literals are supported currently[1].  So maybe currently it's hard to
check the error using Coccinelle :(

Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find
the errors, I didn't find other similar errors.

[1]
https://lore.kernel.org/cocci/alpine.DEB.2.22.394.2010201143330.2736@hadrien/T/#t

Thanks,
Peng

>>
>> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer
>> detection")
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>   hw/i2c/microbit_i2c.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
>> index 802473982082..e92f9f84ea81 100644
>> --- a/hw/i2c/microbit_i2c.c
>> +++ b/hw/i2c/microbit_i2c.c
>> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate
>> = {
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState,
>> MICROBIT_I2C_NREGS),
>>           VMSTATE_UINT32(read_idx, MicrobitI2CState),
>> +        VMSTATE_END_OF_LIST()
>>       },
>>   };
>>  
> 
> 
> .

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 10/20/20 1:17 PM, Peng Liang wrote:
> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
>> On 10/19/20 11:34 AM, Peng Liang wrote:
>>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
>>
>> It might be easy to add a Coccinelle script to avoid future errors.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
> 
> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
> end of VMStateDescription.fields.  For those who are not defined as
> compound literals, it works well.  However, I cannot make it work for
> those defined as compound literals.  And Julia doesn't think compound
> literals are supported currently[1].  So maybe currently it's hard to
> check the error using Coccinelle :(

Interesting.

> 
> Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find
> the errors, I didn't find other similar errors.

Thanks for giving it a try. We could commit and run the script
in a gitlab-ci job to avoid such regressions.

> 
> [1]
> https://lore.kernel.org/cocci/alpine.DEB.2.22.394.2010201143330.2736@hadrien/T/#t
> 
> Thanks,
> Peng
> 
>>>
>>> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer
>>> detection")
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>> ---
>>>    hw/i2c/microbit_i2c.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
>>> index 802473982082..e92f9f84ea81 100644
>>> --- a/hw/i2c/microbit_i2c.c
>>> +++ b/hw/i2c/microbit_i2c.c
>>> @@ -83,6 +83,7 @@ static const VMStateDescription microbit_i2c_vmstate
>>> = {
>>>        .fields = (VMStateField[]) {
>>>            VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState,
>>> MICROBIT_I2C_NREGS),
>>>            VMSTATE_UINT32(read_idx, MicrobitI2CState),
>>> +        VMSTATE_END_OF_LIST()
>>>        },
>>>    };
>>>   
>>
>>
>> .
>

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peng Liang 3 years, 6 months ago

On 10/20/2020 7:27 PM, Philippe Mathieu-Daudé wrote:
> On 10/20/20 1:17 PM, Peng Liang wrote:
>> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
>>> On 10/19/20 11:34 AM, Peng Liang wrote:
>>>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>>>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
>>>
>>> It might be easy to add a Coccinelle script to avoid future errors.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>
>> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
>> end of VMStateDescription.fields.  For those who are not defined as
>> compound literals, it works well.  However, I cannot make it work for
>> those defined as compound literals.  And Julia doesn't think compound
>> literals are supported currently[1].  So maybe currently it's hard to
>> check the error using Coccinelle :(
> 
> Interesting.
> 
>>
>> Thanks for my colleague Biaoxiang Ye, who wrote a shell script to find
>> the errors, I didn't find other similar errors.
> 
> Thanks for giving it a try. We could commit and run the script
> in a gitlab-ci job to avoid such regressions.
> 

The script will report all fields not defined as compound literals as
errors (the number is much smaller than that of all
VMStateDescription.fields).

-- 
Thanks,
Peng

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peter Maydell 3 years, 6 months ago

On Tue, 20 Oct 2020 at 12:17, Peng Liang <liangpeng10@huawei.com> wrote:
>
> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
> > On 10/19/20 11:34 AM, Peng Liang wrote:
> >> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
> >> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
> >
> > It might be easy to add a Coccinelle script to avoid future errors.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
>
> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
> end of VMStateDescription.fields.  For those who are not defined as
> compound literals, it works well.  However, I cannot make it work for
> those defined as compound literals.  And Julia doesn't think compound
> literals are supported currently[1].  So maybe currently it's hard to
> check the error using Coccinelle :(

I think we could probably significantly increase the chances that
people find "missing terminator" errors in the course of normal
debugging of their device if we made the terminator be something
other than "is field->name NULL". That condition is quite likely
to be satisfied by accident shortly after the real end-of-data
(because zeroes are easy to find in memory), whereas if the condition
is "field->flags is a magic number", for instance, then the chances of
it being satisfied by accident are very low, and so a simple "loop
through the field array until we find the end" is pretty likely to
hang/crash. (If we don't already have such a loop we might need to
add one in debug mode when a vmstate is registered.)

(This is why the REGINFO_SENTINEL used for Arm cpreg arrays is
not a simple all-zeroes value, incidentally.)

thanks
-- PMM

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peng Liang 3 years, 6 months ago

On 10/20/2020 7:27 PM, Peter Maydell wrote:
> On Tue, 20 Oct 2020 at 12:17, Peng Liang <liangpeng10@huawei.com> wrote:
>>
>> On 10/19/2020 6:35 PM, Philippe Mathieu-Daudé wrote:
>>> On 10/19/20 11:34 AM, Peng Liang wrote:
>>>> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
>>>> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
>>>
>>> It might be easy to add a Coccinelle script to avoid future errors.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>
>> I tried to add a Coccinelle script to add VMSTATE_END_OF_LIST() to the
>> end of VMStateDescription.fields.  For those who are not defined as
>> compound literals, it works well.  However, I cannot make it work for
>> those defined as compound literals.  And Julia doesn't think compound
>> literals are supported currently[1].  So maybe currently it's hard to
>> check the error using Coccinelle :(
> 
> I think we could probably significantly increase the chances that
> people find "missing terminator" errors in the course of normal
> debugging of their device if we made the terminator be something
> other than "is field->name NULL". That condition is quite likely
> to be satisfied by accident shortly after the real end-of-data
> (because zeroes are easy to find in memory), whereas if the condition
> is "field->flags is a magic number", for instance, then the chances of
> it being satisfied by accident are very low, and so a simple "loop
> through the field array until we find the end" is pretty likely to
> hang/crash. (If we don't already have such a loop we might need to
> add one in debug mode when a vmstate is registered.)
> 
> (This is why the REGINFO_SENTINEL used for Arm cpreg arrays is
> not a simple all-zeroes value, incidentally.)
> 
> thanks
> -- PMM
> .
> 

I found that field->flags is a bit-or field, so maybe all 0xf or other
magic number is still meaningful?  Can we use field->version_id or
field->struct_version_id as the condition?  I found they are all int
type but used as non-negative, so can we use
field->version_id/field->struct_version_id == magic number (for example,
-1) as a sentinel?

-- 
Thanks,
Peng

Re: [PATCH] microbit_i2c: Fix coredump when dump-vmstate

Posted by Peter Maydell 3 years, 6 months ago

On Mon, 19 Oct 2020 at 10:36, Peng Liang <liangpeng10@huawei.com> wrote:
>
> VMStateDescription.fields should be end with VMSTATE_END_OF_LIST().
> However, microbit_i2c_vmstate doesn't follow it.  Let's change it.
>
> Fixes: 9d68bf564e ("arm: Stub out NRF51 TWI magnetometer/accelerometer detection")
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---



Applied to target-arm.next, thanks.

-- PMM