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

[Nathan Lynch <nathan.lynch@amd.com>]

Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
> 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.

They have different behaviors with respect to *signals* and the
wake_up() variant used, but not device interrupts.

dmatest_callback() employs wake_up_all(), which means this change
introduces no beneficial difference in the wakeup behavior. The dmatest
thread gets woken on receipt of the completion interrupt either way.

And to reiterate, the change regresses the combination of dmatest and
the task freezer, which is a use case people have cared about,
apparently.

>> 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()")