Re: [PATCH] uio: Fix bus error that access memory mapped by physical

[wubian <wubian@uniontech.com>]

First,thanks for your reply

I haven’t found this problem on the x86 platform. I only found it on 
arm64. I used gdb to track memset and found that the bus error in 
glibc/sysdeps/aarch64/memset.S: dc zva, dst; reference "Architecture 
Reference ManualArmv8, for Armv8-A architecture profile" manual found 
that the dc assembly instruction（performance optimization） is related to 
the operation of the cache, that is to say, there is a bus error in the 
operation of the cache, and then check the "ARM Cortex-A Series 
Programmer's Guide for ARMv8-A " manual, found that the armv8 
architecture cache has data cache and write buffer, and there are two 
operation modes write-back and write-through, write operations in these 
two modes, the data flow will pass through the write buffer, and 
pgprot_noncached will prohibit data Cache and write buffer, this causes 
the dc command in memset to fail (bus error), and pgprot_writecombine 
does not prohibit write buffer, so the dc command in memset is 
successfully operated when pgprot_writecombine is used.

Regarding the bugfix issue, this is my first kernel patch submission, I 
may not be too clear about the boundary of the kernel bugfix. I think 
this issue is a bug, so I classify this submission as a bugfix.  At last 
hope to get your suggestion.

thanks,

wubian

On 2021/6/23 下午3:14, Greg KH wrote:
> On Wed, Jun 23, 2021 at 02:52:14PM +0800, wubian wrote:
>> On the arm64, register the uio driver and map a physical space
>> on the pci device to user space, then use memset write data to
>> the address space, a bus error will occur. This error is due to
>> the dc instruction(cache operation) used in the assembly of memset,
>> uio mapping physical memory will call pgprot_noncached() to set
>> non-cached and non-buffered, while pgprot_writecombine() has fewer
>> restrictions. It does not prohibit write buffer, so replacing
>> pgprot_noncached() with pgprot_writecombine() can solve this problem.
>>
>> Signed-off-by: wubian <wubian@uniontech.com>
>> ---
>>   drivers/uio/uio.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index ea96e319c8a0..09b04b20fa30 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -739,7 +739,11 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
>>   
>>   	vma->vm_ops = &uio_physical_vm_ops;
>>   	if (idev->info->mem[mi].memtype == UIO_MEM_PHYS)
>> +#if defined(CONFIG_ARM64)
>> +		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>> +#else
>>   		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +#endif
> This feels really wrong, shouldn't stuff like this be handled in the
> platform itself and not in the driver?
>
> And why is ARM64 special here?  Why not other arches?  What is odd about
> this platform?  We almost never want to use #if in .c files, why is it
> ok to do that here?
>
> And is this a bugfix?  If so, what commit does it fix?  Should it go to
> stable kernels, and if so, how far back?
>
> I need more information here :)
>
> thanks,
>
> greg k-h
>
>