Re: Memory corruption with CONFIG_SWIOTLB_DYNAMIC=y

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 8 Nov 2023 15:45:49 +0100
Petr Tesarik <petr.tesarik1@huawei-partners.com> wrote:

> On 11/8/2023 3:32 PM, Halil Pasic wrote:
> > On Wed, 8 Nov 2023 12:04:12 +0100
> > Petr Tesarik <petr.tesarik1@huawei-partners.com> wrote:
> > [..]  
> >>>
> >>> For the sake of simplicity let us assume we only have the min_align_mask
> >>> requirement. Then I believe the worst case is that we need 
> >>> (orig_addr & min_align_mask & PAGE_MASK)  + (min_align_mask & ~PAGE_MASK)
> >>> extra space to fit.
> >>>
> >>> Depending on how the semantics pan out one may be able to replace
> >>> min_align_mask with combined_mask.
> >>>
> >>> Is your point that for large combined_mask values 
> >>> _get_free_pages(GFP_NOWAIT | __GFP_NOWARN, required_order) is not
> >>> likely to complete successfully?    
> >>
> >> Yes, that's the reason. OTOH it's probably worth a try. The point is
> >> that mapping a DMA buffer is allowed to fail, so callers should be
> >> prepared anyway.
> >>
> >> And for the case you reported initially, I don't think there is any need
> >> to preserve bit 11 (0x800) from the original buffer's physical address,
> >> which is enough to fix it. See also my other email earlier today.  
> > 
> > Hm. Do you mean "[PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations
> > with CONFIG_SWIOTLB_DYNAMIC" or a different one?
> > 
> > I only see "[PATCH 1/1] swiotlb: fix out-of-bounds TLB allocations
> > with CONFIG_SWIOTLB_DYNAMIC" but I don't think that one takes
> > care of "I don't think there is any need to preserve bit 11 (0x800)
> > from the original buffer's physical address".  
> 
> Yes, I mean only this patch. I want to fix memory corruption fast, while
> waiting for more feedback on my understanding of the alignment masks.
> What I'm trying to say is that your specific use case may not even need
> a bigger allocation if the page alignment should be interpreted differently.
> 
> Again, thank you for your in-depth inspection, because it's not totally
> clear how the various alignment parameters should be interpreted. It's
> difficult to write correct code then...

I fully understand. Thanks for tackling this. We decided to go with a bug
report and not with a fix because of the very same reasons: lack of
clarity on how certain things are supposed to work. Let us see how
the discussion develops. :)

Regards,
Halil

> 
> Petr T