2021-04-18 08:20:26

by Alexander Egorenkov

[permalink] [raw]
Subject: [PATCH 1/1] lib: scatterlist: Fix SGL length in sg_split() if !CONFIG_NEED_SG_DMA_LENGTH

If CONFIG_NEED_SG_DMA_LENGTH is NOT enabled then sg_dma_len() is an alias
for the length field in a SGL. In that case sg_split() wrongly resets
the length of split SGLs to zero after it was set correctly before.

Signed-off-by: Alexander Egorenkov <[email protected]>
---
lib/sg_split.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/sg_split.c b/lib/sg_split.c
index 60a0babebf2e..3d9b32084d78 100644
--- a/lib/sg_split.c
+++ b/lib/sg_split.c
@@ -92,7 +92,8 @@ static void sg_split_phys(struct sg_splitter *splitters, const int nb_splits)
out_sg->offset = 0;
}
sg_dma_address(out_sg) = 0;
- sg_dma_len(out_sg) = 0;
+ if (IS_ENABLED(CONFIG_NEED_SG_DMA_LENGTH))
+ sg_dma_len(out_sg) = 0;
in_sg = sg_next(in_sg);
}
out_sg[-1].length = split->length_last_sg;
--
2.31.1


2021-04-19 09:40:40

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/1] lib: scatterlist: Fix SGL length in sg_split() if !CONFIG_NEED_SG_DMA_LENGTH

On Sun, Apr 18, 2021 at 08:14:41AM +0000, Alexander Egorenkov wrote:
> If CONFIG_NEED_SG_DMA_LENGTH is NOT enabled then sg_dma_len() is an alias
> for the length field in a SGL. In that case sg_split() wrongly resets
> the length of split SGLs to zero after it was set correctly before.

Why is this routine messing with sg_dma_address and
sg_dma_lensg_dma_lensg_dma_len at all? This whole sg_spli() routine
seems rather dangerous because ownership and state of the DMA mapping is
entirely unclear.

2021-04-19 16:07:36

by Alexander Egorenkov

[permalink] [raw]
Subject: Re: [PATCH 1/1] lib: scatterlist: Fix SGL length in sg_split() if !CONFIG_NEED_SG_DMA_LENGTH

Christoph Hellwig <[email protected]> writes:

> On Sun, Apr 18, 2021 at 08:14:41AM +0000, Alexander Egorenkov wrote:
>> If CONFIG_NEED_SG_DMA_LENGTH is NOT enabled then sg_dma_len() is an alias
>> for the length field in a SGL. In that case sg_split() wrongly resets
>> the length of split SGLs to zero after it was set correctly before.
>
> Why is this routine messing with sg_dma_address and
> sg_dma_lensg_dma_lensg_dma_len at all? This whole sg_spli() routine
> seems rather dangerous because ownership and state of the DMA mapping is
> entirely unclear.

If i understood it correctly, then sg_split_phys() creates a new SGL
from the given one, so it makes sense to initialize DMA fields of the
new SGL. sg_split() allows one to split the given SGL into multiple ones
and the original one doesn't have to be dma-mapped which is indicated by the
parameter in_mapped_nents > 0.

Regards
Alex