Re: Regression: memory corruption on Atmel SAMA5D31

[Tudor.Ambarus]

On 4/9/22 16:02, Thorsten Leemhuis wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi, this is your Linux kernel regression tracker. Top-posting for once,
> to make this easily accessible to everyone.
> 
> Can somebody please provide a status update what the outcome of this
> thread? It started as a regression report, that's why I'm tracking it --

Hi, Thorsten,

There are some concurrency bugs in the at-hdmac (DMA) driver, I'm handling them
and will come with a resolution. Disabling the DMA showed the bug is no more
reproducible.

> but seems nothing happened for a while. Was it fixed? Did it fall
> through the cracks? Or did it turn out that this is not a regression? If

Not yet sure if it's a regression or not, as the bugs are there since the
beginning. Maybe they are just harder to reproduce.

> the latter: please feel free to include a paragraph like "#regzbot
> invalid: a few words why this is invalid in the lengths of a mail subject"
> 

Will come with a follow up after I fix the DMA bugs.

Cheers,
ta
> Ciao, Thorsten
> 
> #regzbot poke
> 
> On 10.03.22 11:40, Peter Rosin wrote:
>> On 2022-03-10 10:58, Peter Rosin wrote:
>>> [bringing this threadlet back to the lists, hope that's ok]
>>>
>>> On 2022-03-10 09:27, Nicolas Ferre wrote:
>>>>  From that article:
>>>> https://lwn.net/Articles/885941/
>>>>
>>>> I read:
>>>>
>>>> "Koschel included a patch fixing a bug in the USB subsystem where the
>>>> iterator passed to this macro was used after the exit from the macro,
>>>> which is a dangerous thing to do. Depending on what happens within the
>>>> list, the contents of that iterator could be something surprising, even
>>>> in the absence of speculative execution. Koschel fixed the problem by
>>>> reworking the code in question to stop using the iterator after the loop. "
>>>>
>>>> USB subsystem, "struct list_head *next, *prev;"... Some keywords present
>>>> there... worth a try?
>>>>
>>>> Regards,
>>>>    Nicolas
>>>
>>> gr_udc.c is not built with the config that is in use, which is sad because
>>> it looked like a good candidate.
>>
>> at91_usba_udc.c, which is included, has the same pattern. But alas, doing
>> the equivalent patch there does not fix things either. I.e. (whitespace
>> damaged)
>>
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -863,6 +863,7 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>         struct usba_request *req;
>>         unsigned long flags;
>>         u32 status;
>> +       bool found = false;
>>
>>         DBG(DBG_GADGET | DBG_QUEUE, "ep_dequeue: %s, req %p\n",
>>                         ep->ep.name, _req);
>> @@ -870,11 +871,13 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>         spin_lock_irqsave(&udc->lock, flags);
>>
>>         list_for_each_entry(req, &ep->queue, queue) {
>> -               if (&req->req == _req)
>> +               if (&req->req == _req) {
>> +                       found = true;
>>                         break;
>> +               }
>>         }
>>
>> -       if (&req->req != _req) {
>> +       if (!found) {
>>                 spin_unlock_irqrestore(&udc->lock, flags);
>>                 return -EINVAL;
>>         }
>>
>> The test started out with 3 good hashes though, so I got my hopes up. But
>> no, it's about the same failure rate as usual. I have the feeling that I
>> will never again trust a single sha256sum...
>>
>> Cheers,
>> Peter
>