| Message ID | 529849b0-2ba9-85bf-c57f-0abe93cfdc42@gpxsee.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | XDMA crashes on zero-length sg entries | expand |
Hi Martin, Based on the dmaengine document https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html The sg_len used for dmaengine_prep_slave_sg should be returned by dma_map_sg. This implies there should not be zero segment for the first sg_len segments. I think that is why we need to provide 'sg_len' for dmaengine_prep_slave_sg(). With 'sg_len', we do not need to worry about zero length sg in the driver callback. Thanks, Lizhi On 3/16/23 10:03, Martin Tůma wrote: > Hi, > The Xilinx XDMA driver crashes when the scatterlist provided to > xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len() > returns zero. As I do get such sgl from v4l2 I suppose that this > is a valid scenario and not a bug in our parent mgb4 driver. Also > the documentation for sg_dma_len() suggests that there may be > zero-length entries. > > The following trivial patch fixes the crash: > > diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c > index 462109c61653..cd5fcd911c50 100644 > --- a/drivers/dma/xilinx/xdma.c > +++ b/drivers/dma/xilinx/xdma.c > @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct > scatterlist *sgl, > for_each_sg(sgl, sg, sg_len, i) { > addr = sg_dma_address(sg); > rest = sg_dma_len(sg); > + if (!rest) > + break; > > do { > len = min_t(u32, rest, XDMA_DESC_BLEN_MAX); > > M.
Hi On 16. 03. 23 19:25, Lizhi Hou wrote: > Hi Martin, > > Based on the dmaengine document > > https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html > > The sg_len used for dmaengine_prep_slave_sg should be returned by > dma_map_sg. > > This implies there should not be zero segment for the first sg_len > segments. > I do get the mapping from vb2_dma_sg_plane_desc() which internally uses dma_map_sg_attrs() and yet in some special cases sg_dma_len() returns zero. So this is a "real-life" scenario. I'm still investigating what the root cause is for getting such empty mappings from v4l2, but the DMA driver should IMHO not crash the kernel in such case anyway. I have looked into some randomly chosen kernel code that uses sg_dma_len() and all usages that I have seen are prepared for the case the length is zero. All but one driver simply stop the iteration while the remaining one reported this case as an error. M. > I think that is why we need to provide 'sg_len' for > dmaengine_prep_slave_sg(). > > With 'sg_len', we do not need to worry about zero length sg in the > driver callback. > > > Thanks, > > Lizhi > > On 3/16/23 10:03, Martin Tůma wrote: >> Hi, >> The Xilinx XDMA driver crashes when the scatterlist provided to >> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len() >> returns zero. As I do get such sgl from v4l2 I suppose that this >> is a valid scenario and not a bug in our parent mgb4 driver. Also >> the documentation for sg_dma_len() suggests that there may be >> zero-length entries. >> >> The following trivial patch fixes the crash: >> >> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c >> index 462109c61653..cd5fcd911c50 100644 >> --- a/drivers/dma/xilinx/xdma.c >> +++ b/drivers/dma/xilinx/xdma.c >> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct >> scatterlist *sgl, >> for_each_sg(sgl, sg, sg_len, i) { >> addr = sg_dma_address(sg); >> rest = sg_dma_len(sg); >> + if (!rest) >> + break; >> >> do { >> len = min_t(u32, rest, XDMA_DESC_BLEN_MAX); >> >> M.
Hi Martin, On 3/17/23 04:18, Martin Tůma wrote: > Hi > > On 16. 03. 23 19:25, Lizhi Hou wrote: >> Hi Martin, >> >> Based on the dmaengine document >> >> https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html >> >> The sg_len used for dmaengine_prep_slave_sg should be returned by >> dma_map_sg. >> >> This implies there should not be zero segment for the first sg_len >> segments. >> > > I do get the mapping from vb2_dma_sg_plane_desc() which internally > uses dma_map_sg_attrs() and yet in some special cases sg_dma_len() > returns zero. So this is a "real-life" scenario. I'm still investigating This sounds odd to me. dma_map_sg() is supposed to return number of Non-zero sg because the zero length sg will be merged. > what the root cause is for getting such empty mappings from v4l2, but > the DMA driver should IMHO not crash the kernel in such case anyway. I checked some dma driver and I did not see obvious code to check the sg len. e.g. https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/dma/k3dma.c#L531 > I have looked into some randomly chosen kernel code that uses > sg_dma_len() and all usages that I have seen are prepared for the case > the length is zero. All but one driver simply stop the iteration while > the remaining one reported this case as an error. Are those dma drivers? Adding a check sounds fine to me. And I am going to create a patch anyway. :) Thanks, Lizhi > > M. > >> I think that is why we need to provide 'sg_len' for >> dmaengine_prep_slave_sg(). >> >> With 'sg_len', we do not need to worry about zero length sg in the >> driver callback. >> >> >> Thanks, >> >> Lizhi >> >> On 3/16/23 10:03, Martin Tůma wrote: >>> Hi, >>> The Xilinx XDMA driver crashes when the scatterlist provided to >>> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len() >>> returns zero. As I do get such sgl from v4l2 I suppose that this >>> is a valid scenario and not a bug in our parent mgb4 driver. Also >>> the documentation for sg_dma_len() suggests that there may be >>> zero-length entries. >>> >>> The following trivial patch fixes the crash: >>> >>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c >>> index 462109c61653..cd5fcd911c50 100644 >>> --- a/drivers/dma/xilinx/xdma.c >>> +++ b/drivers/dma/xilinx/xdma.c >>> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, >>> struct scatterlist *sgl, >>> for_each_sg(sgl, sg, sg_len, i) { >>> addr = sg_dma_address(sg); >>> rest = sg_dma_len(sg); >>> + if (!rest) >>> + break; >>> >>> do { >>> len = min_t(u32, rest, XDMA_DESC_BLEN_MAX); >>> >>> M. >
Hi, On 17. 03. 23 18:19, Lizhi Hou wrote: > Hi Martin, > > On 3/17/23 04:18, Martin Tůma wrote: >> Hi >> >> On 16. 03. 23 19:25, Lizhi Hou wrote: >>> Hi Martin, >>> >>> Based on the dmaengine document >>> >>> https://kernel.org/doc/html/v6.3-rc2/driver-api/dmaengine/client.html >>> >>> The sg_len used for dmaengine_prep_slave_sg should be returned by >>> dma_map_sg. >>> >>> This implies there should not be zero segment for the first sg_len >>> segments. >>> >> >> I do get the mapping from vb2_dma_sg_plane_desc() which internally >> uses dma_map_sg_attrs() and yet in some special cases sg_dma_len() >> returns zero. So this is a "real-life" scenario. I'm still investigating > This sounds odd to me. dma_map_sg() is supposed to return number of > Non-zero sg because the zero length sg will be merged. >> what the root cause is for getting such empty mappings from v4l2, but >> the DMA driver should IMHO not crash the kernel in such case anyway. > > I checked some dma driver and I did not see obvious code to check the sg > len. e.g. > > https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/dma/k3dma.c#L531 > Didn't examinate this one in detail, but I don't think this one would crash either. In your case, the problem is, that you do not pre-allocate the descriptor in case sg_dma_len(sg) is zero and then access it in: desc->bytes = cpu_to_le32(len); which is the exact line where xdma crashes. M. >> I have looked into some randomly chosen kernel code that uses >> sg_dma_len() and all usages that I have seen are prepared for the case >> the length is zero. All but one driver simply stop the iteration while >> the remaining one reported this case as an error. > > Are those dma drivers? > > Adding a check sounds fine to me. And I am going to create a patch > anyway. :) > > > Thanks, > > Lizhi > >> >> M. >> >>> I think that is why we need to provide 'sg_len' for >>> dmaengine_prep_slave_sg(). >>> >>> With 'sg_len', we do not need to worry about zero length sg in the >>> driver callback. >>> >>> >>> Thanks, >>> >>> Lizhi >>> >>> On 3/16/23 10:03, Martin Tůma wrote: >>>> Hi, >>>> The Xilinx XDMA driver crashes when the scatterlist provided to >>>> xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len() >>>> returns zero. As I do get such sgl from v4l2 I suppose that this >>>> is a valid scenario and not a bug in our parent mgb4 driver. Also >>>> the documentation for sg_dma_len() suggests that there may be >>>> zero-length entries. >>>> >>>> The following trivial patch fixes the crash: >>>> >>>> diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c >>>> index 462109c61653..cd5fcd911c50 100644 >>>> --- a/drivers/dma/xilinx/xdma.c >>>> +++ b/drivers/dma/xilinx/xdma.c >>>> @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, >>>> struct scatterlist *sgl, >>>> for_each_sg(sgl, sg, sg_len, i) { >>>> addr = sg_dma_address(sg); >>>> rest = sg_dma_len(sg); >>>> + if (!rest) >>>> + break; >>>> >>>> do { >>>> len = min_t(u32, rest, XDMA_DESC_BLEN_MAX); >>>> >>>> M. >>
diff --git a/drivers/dma/xilinx/xdma.c b/drivers/dma/xilinx/xdma.c index 462109c61653..cd5fcd911c50 100644 --- a/drivers/dma/xilinx/xdma.c +++ b/drivers/dma/xilinx/xdma.c @@ -487,6 +487,8 @@ xdma_prep_device_sg(struct dma_chan *chan, struct scatterlist *sgl, for_each_sg(sgl, sg, sg_len, i) { addr = sg_dma_address(sg); rest = sg_dma_len(sg); + if (!rest) + break; do {
Hi, The Xilinx XDMA driver crashes when the scatterlist provided to xdma_prep_device_sg() contains an empty entry, i.e. sg_dma_len() returns zero. As I do get such sgl from v4l2 I suppose that this is a valid scenario and not a bug in our parent mgb4 driver. Also the documentation for sg_dma_len() suggests that there may be zero-length entries. The following trivial patch fixes the crash: len = min_t(u32, rest, XDMA_DESC_BLEN_MAX); M.