Hi, Russell, I have missed you from the To: list of this series by mistake, I'm sorry about that. - Péter On 2017-11-14 16:32, Peter Ujfalusi wrote: > Hi, > > With the introduction of .device_synchronize callback it was thought that the > race caused crash observed in vchan_complete is fixed, but unfortunately it can > still happen. > > The observed scenario (really hard to reproduce) is: > Cyclic mode > - DMA period interrupt > - call to vchan_cyclic_callback() which sets vc->cyclic to vd and schedules the > vchan_complete tasklet > - .terminate_all is called > - we make sure that no further DMA irqs are going to be handled, but the > tasklet had been already scheduled > - we free up the descriptor to avoid leaking memory > - the vchan_complete tasklet starts to execute and it checks vc->cyclic, which > is not NULL, saves the vd pointer (which points to an already freed up memory) > and try to access it later to call the callback > - the tasklet_kill() in .device_synchronize will make sure that the tasklet is > going to finish it's execution if it is already scheduled, it can only help if > the tasklet is yet to be executed. > > At this point it is just matter of luck if the vc->cyclic is still pointing to > an unchanged memory location or it is taken into use and thus it is corrupted. > > My first approach was to just set vc->cyclic to NULL in the .terminate_all > callback, but that still have theoritical race: if the vchan_complete is > executing and it saves the vd from vc->cyclic (protected by the vc->lock). If at > that point the .terminate_all is called it will wait for the lock and start > executing. _if_ the .terminate_all free up the vd before the vchan_complete > reaches the point when it is going to call the callback, then we have the race. > > The series will do this: > - the drivers should call vchan_terminate_vdesc() instead of directly freeing up > the descriptor. vchan_terminate_vdesc() will save the vd as vc->vd_terminated > and will set the vc->cyclic to NULL, all while holding the lock. > - the drivers must implement the .device_synchronize callback and within the > vchan_synchronize() we free up the vc->vd_terminated after we killed the > tasklet. > > I have tested this on platforms using TI's eDMA and sDMA and have not seen any > side effect so far and a client tested similar set on a setup where it was easy > to reproduce the race. > > By looking for similar patterns in other drivers I have implemented the fix for > the ones where it looked straight forward. > > Regards, > Peter > --- > Peter Ujfalusi (10): > dmaengine: virt-dma: Add helper to free/reuse a descriptor > dmaengine: virt-dma: Support for race free transfer termination > dmaengine: omap-dma: Use vchan_terminate_vdesc() instead of desc_free > dmaengine: edma: Use vchan_terminate_vdesc() instead of desc_free > dmaengine: bcm2835-dma: Use vchan_terminate_vdesc() instead of > desc_free > dmaengine: dma-jz4780: Use vchan_terminate_vdesc() instead of > desc_free > dmaengine: amba-pl08x: Use vchan_terminate_vdesc() instead of > desc_free > dmaengine: img-mdc-dma: Use vchan_terminate_vdesc() instead of > desc_free > dmaengine: k3dma: Use vchan_terminate_vdesc() instead of desc_free > dmaengine: s3c24xx-dma: Use vchan_terminate_vdesc() instead of > desc_free > > drivers/dma/amba-pl08x.c | 11 ++++++++++- > drivers/dma/bcm2835-dma.c | 10 +++++++++- > drivers/dma/dma-jz4780.c | 10 +++++++++- > drivers/dma/edma.c | 7 ++----- > drivers/dma/img-mdc-dma.c | 17 ++++++++++++----- > drivers/dma/k3dma.c | 10 +++++++++- > drivers/dma/omap-dma.c | 2 +- > drivers/dma/s3c24xx-dma.c | 11 ++++++++++- > drivers/dma/virt-dma.c | 5 +---- > drivers/dma/virt-dma.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 10 files changed, 107 insertions(+), 20 deletions(-) > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
