Re: [PATCH v1] dmaengine: dmatest: Fix dmatest waiting less when interrupted

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Nathan Lynch <nathan.lynch@amd.com> writes:

> Hi Vinicius,
>
> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
>> Nathan Lynch <nathan.lynch@amd.com> writes:
>>> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
>>>> Change the "wait for operation finish" logic to take interrupts into
>>>> account.
>>>>
>>>> When using dmatest with idxd DMA engine, it's possible that during
>>>> longer tests, the interrupt notifying the finish of an operation
>>>> happens during wait_event_freezable_timeout(), which causes dmatest to
>>>> cleanup all the resources, some of which might still be in use.
>>>>
>>>> This fix ensures that the wait logic correctly handles interrupts,
>>>> preventing premature cleanup of resources.
>>>>
>>>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>>> Closes: https://lore.kernel.org/oe-lkp/202502171134.8c403348-lkp@intel.com
>>>
>>> Given the report at the URL above I'm struggling to follow the rationale
>>> for this change. It looks like a use-after-free in idxd while
>>> resetting/unbinding the device, and I can't see how changing whether
>>> dmatest threads perform freezeable waits would change this.
>>>
>>
>> I think that the short version is that the reproducition script triggers
>> different problems on different platforms/configurations.
>>
>> The solution I proposed fixes a problem I was seeing on a SPR system, on
>> a GNR system (that I was only able to get later) I see something more similar
>> to this particular splat (currently working on the fix).
>>
>> Seeing this question, I realize that I should have added a note to the
>> commit detailing this.
>>
>> So I am planning on proposing two (this and another) fixes for the same
>> report, hoping that it's not that confusing/unusual.
>
> I'm still confused... why is wait_event_freezable_timeout() the wrong
> API for dmatest to use, and how could changing it to
> wait_event_timeout() cause it to "take interrupts into account" that it
> didn't before?
>

My understanding (and testing) is that wait_event_timeout() will block
for the duration even in the face of interrupts, 'freezable' will not.

> AFAIK the only change made here is that dmatest threads effectively
> become unfreezeable, which is contrary to prior authors' intentions:
>
> commit 981ed70d8e4f ("dmatest: make dmatest threads freezable")
> commit adfa543e7314 ("dmatest: don't use set_freezable_with_signal()")
>

Cheers,
-- 
Vinicius