Re: [bug report] mm: replace vma->vm_flags direct modifications with modifier calls

[David Hildenbrand <david@redhat.com>]

On 12.07.23 20:49, Matthew Wilcox wrote:
> On Wed, Jul 12, 2023 at 05:55:47PM +0200, David Hildenbrand wrote:
>> On 12.07.23 17:52, Matthew Wilcox wrote:
>>> On Wed, Jul 12, 2023 at 08:01:18AM -0700, Suren Baghdasaryan wrote:
>>>> Are you suggesting to break remap_pfn_range() into two stages
>>>> (remap_pfn_range_prepare() then remap_pfn_range())?
>>>> If so, there are many places remap_pfn_range() is called and IIUC all
>>>> of them would need to use that 2-stage approach (lots of code churn).
>>>> In addition, this is an exported function, so many more drivers might
>>>> expect the current behavior.
>>>
>>> You do not understand correctly.
>>>
>>> When somebody calls mmap, there are two reasonable implementations.
>>> Here's one:
>>>
>>>           .mmap = snd_dma_iram_mmap,
>>>
>>> static int snd_dma_iram_mmap(struct snd_dma_buffer *dmab,
>>>                                struct vm_area_struct *area)
>>> {
>>>           area->vm_page_prot = pgprot_writecombine(area->vm_page_prot);
>>>           return remap_pfn_range(area, area->vm_start,
>>>                                  dmab->addr >> PAGE_SHIFT,
>>>                                  area->vm_end - area->vm_start,
>>>                                  area->vm_page_prot);
>>> }
>>>
>>> This is _fine_.  It is not called from the fault path, it is called in
>>> process context.  Few locks are held (which ones aren't even
>>> documented!)
>>>
>>> The other way is to set vma->vm_ops.  The fault handler in vm_ops
>>> should not be calling remap_pfn_range().  It should be calling
>>> set_ptes().  I almost have this driver fixed up, but I have another
>>> meeting to go to now.
>>
>> Just a note that we still have to make sure that the VMA flags will be set
>> properly -- I guess at mmap time is the right time as I suggested above.
> 
> It actually does that already:
> 
> static int gru_file_mmap(struct file *file, struct vm_area_struct *vma)
> {
>          if ((vma->vm_flags & (VM_SHARED | VM_WRITE)) != (VM_SHARED | VM_WRITE))
>                  return -EPERM;
> 
>          if (vma->vm_start & (GRU_GSEG_PAGESIZE - 1) ||
>                                  vma->vm_end & (GRU_GSEG_PAGESIZE - 1))
>                  return -EINVAL;
> 
>          vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_LOCKED |
>                           VM_DONTCOPY | VM_DONTEXPAND | VM_DONTDUMP);
> 
> 
> This compiles, but obviously I don't have a spare HP supercomputer lying
> around for me to test whether it works.  Also set_ptes() was only just
> introduced to the mm tree, so doing something that needs backporting
> would take more effort (maybe having a private set_ptes() in the driver
> would be a good backport option that calls set_pte_at() in a loop).
> 
> 
> diff --git a/drivers/misc/sgi-gru/grumain.c b/drivers/misc/sgi-gru/grumain.c
> index 4eb4b9455139..c21bcb528f12 100644
> --- a/drivers/misc/sgi-gru/grumain.c
> +++ b/drivers/misc/sgi-gru/grumain.c
> @@ -951,6 +951,8 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
>   	}
>   
>   	if (!gts->ts_gru) {
> +		pte_t *ptep, pte;
> +
>   		STAT(load_user_context);
>   		if (!gru_assign_gru_context(gts)) {
>   			preempt_enable();
> @@ -964,9 +966,12 @@ vm_fault_t gru_fault(struct vm_fault *vmf)
>   		}
>   		gru_load_context(gts);
>   		paddr = gseg_physical_address(gts->ts_gru, gts->ts_ctxnum);
> -		remap_pfn_range(vma, vaddr & ~(GRU_GSEG_PAGESIZE - 1),
> -				paddr >> PAGE_SHIFT, GRU_GSEG_PAGESIZE,
> -				vma->vm_page_prot);
> +
> +		pte = pfn_pte(paddr / PAGE_SIZE, vma->vm_page_prot);
> +		ptep = vmf->pte - (vaddr % GRU_GSEG_PAGESIZE) / PAGE_SIZE;
> +		set_ptes(vma->vm_mm, vaddr & ~(GRU_GSEG_PAGESIZE - 1),
> +				ptep, pte_mkspecial(pte),
> +				GRU_GSEG_PAGESIZE / PAGE_SIZE);
>   	}
>   
>   	preempt_enable();
> 

Would we be able to fix it in stable simply by not triggering the 
vm_flags_set() in case these flags are already set?

-- 
Cheers,

David / dhildenb