[PATCH v2] comedi: Flush partial mappings in error case

[PATCH v2] comedi: Flush partial mappings in error case

[Jann Horn]

If some remap_pfn_range() calls succeeded before one failed, we still have
buffer pages mapped into the userspace page tables when we drop the buffer
reference with comedi_buf_map_put(bm). The userspace mappings are only
cleaned up later in the mmap error path.

Fix it by explicitly flushing all mappings in our VMA on the error path.

See commit 79a61cc3fc04 ("mm: avoid leaving partial pfn mappings around in
error case").

Cc: stable@vger.kernel.org
Fixes: ed9eccbe8970 ("Staging: add comedi core")
Signed-off-by: Jann Horn <jannh@google.com>
---
Note: compile-tested only; I don't actually have comedi hardware, and I
don't know anything about comedi.
---
Changes in v2:
- only do the zapping in the pfnmap path (Ian Abbott)
- use zap_vma_ptes() instead of zap_page_range_single() (Ian Abbott)
- Link to v1: https://lore.kernel.org/r/20241014-comedi-tlb-v1-1-4b699144b438@google.com
---
 drivers/comedi/comedi_fops.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 1b481731df96..68e5301e6281 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -2407,6 +2407,16 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
 
 			start += PAGE_SIZE;
 		}
+
+		/*
+		 * Leaving behind a partial mapping of a buffer we're about to
+		 * drop is unsafe, see remap_pfn_range_notrack().
+		 * We need to zap the range here ourselves instead of relying
+		 * on the automatic zapping in remap_pfn_range() because we call
+		 * remap_pfn_range() in a loop.
+		 */
+		if (retval)
+			zap_vma_ptes(vma, vma->vm_start, size);
 	}
 
 	if (retval == 0) {

---
base-commit: 6485cf5ea253d40d507cd71253c9568c5470cd27
change-id: 20241014-comedi-tlb-400246505961
-- 
Jann Horn <jannh@google.com>

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Ian Abbott]

On 15/10/2024 19:26, Jann Horn wrote:
> If some remap_pfn_range() calls succeeded before one failed, we still have
> buffer pages mapped into the userspace page tables when we drop the buffer
> reference with comedi_buf_map_put(bm). The userspace mappings are only
> cleaned up later in the mmap error path.
> 
> Fix it by explicitly flushing all mappings in our VMA on the error path.
> 
> See commit 79a61cc3fc04 ("mm: avoid leaving partial pfn mappings around in
> error case").
> 
> Cc: stable@vger.kernel.org
> Fixes: ed9eccbe8970 ("Staging: add comedi core")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> Note: compile-tested only; I don't actually have comedi hardware, and I
> don't know anything about comedi.

It is possible to test it by loading the comedi_test driver module, 
which creates a comedi device in software.  But then you need to induce 
an error somehow.  I did it by hacking the comedi_mmap() function to 
break the loop that calls remap_pfn_range() early with an error.

> ---
> Changes in v2:
> - only do the zapping in the pfnmap path (Ian Abbott)
> - use zap_vma_ptes() instead of zap_page_range_single() (Ian Abbott)
> - Link to v1: https://lore.kernel.org/r/20241014-comedi-tlb-v1-1-4b699144b438@google.com
> ---
>   drivers/comedi/comedi_fops.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> index 1b481731df96..68e5301e6281 100644
> --- a/drivers/comedi/comedi_fops.c
> +++ b/drivers/comedi/comedi_fops.c
> @@ -2407,6 +2407,16 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
>   
>   			start += PAGE_SIZE;
>   		}
> +
> +		/*
> +		 * Leaving behind a partial mapping of a buffer we're about to
> +		 * drop is unsafe, see remap_pfn_range_notrack().
> +		 * We need to zap the range here ourselves instead of relying
> +		 * on the automatic zapping in remap_pfn_range() because we call
> +		 * remap_pfn_range() in a loop.
> +		 */
> +		if (retval)
> +			zap_vma_ptes(vma, vma->vm_start, size);
>   	}
>   
>   	if (retval == 0) {
> 
> ---
> base-commit: 6485cf5ea253d40d507cd71253c9568c5470cd27
> change-id: 20241014-comedi-tlb-400246505961

I have tested it with the device created by the comedi_test.ko module 
using the comedi_test application from comedilib, by artificially 
breaking the partial mapping loop with an error half way to completion. 
I haven't noticed any problems from the zap_vma_ptes() call, so it is OK 
as far as I can tell.  (I don't really have any experience of calling 
functions like zap_vma_ptes().)

I note that there are a bunch of drivers in drivers/video/fbdev that 
also call remap_pfn_range() or io_remap_pfn_range() in a loop that 
probably require your attention!

Tested-by: Ian Abbott <abbotti@mev.co.uk>
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH v2] comedi: Flush partial mappings in error case

[kernel test robot]

Hi Jann,

kernel test robot noticed the following build errors:

[auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]

url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/comedi-Flush-partial-mappings-in-error-case/20241016-022809
base:   6485cf5ea253d40d507cd71253c9568c5470cd27
patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a%40google.com
patch subject: [PATCH v2] comedi: Flush partial mappings in error case
config: arm-randconfig-004-20241016 (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410170111.K30oyTWa-lkp@intel.com/

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/comedi/comedi_fops.o: in function `comedi_mmap':
>> comedi_fops.c:(.text+0x4be): undefined reference to `zap_vma_ptes'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] comedi: Flush partial mappings in error case

[kernel test robot]

Hi Jann,

kernel test robot noticed the following build errors:

[auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]

url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/comedi-Flush-partial-mappings-in-error-case/20241016-022809
base:   6485cf5ea253d40d507cd71253c9568c5470cd27
patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a%40google.com
patch subject: [PATCH v2] comedi: Flush partial mappings in error case
config: m68k-randconfig-r071-20241016 (https://download.01.org/0day-ci/archive/20241017/202410170234.hthSWOJg-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170234.hthSWOJg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410170234.hthSWOJg-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
>> ERROR: modpost: "zap_vma_ptes" [drivers/comedi/comedi.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Jann Horn]

On Wed, Oct 16, 2024 at 8:05 PM kernel test robot <lkp@intel.com> wrote:
> [auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/comedi-Flush-partial-mappings-in-error-case/20241016-022809
> base:   6485cf5ea253d40d507cd71253c9568c5470cd27
> patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a%40google.com
> patch subject: [PATCH v2] comedi: Flush partial mappings in error case
> config: arm-randconfig-004-20241016 (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410170111.K30oyTWa-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    arm-linux-gnueabi-ld: drivers/comedi/comedi_fops.o: in function `comedi_mmap':
> >> comedi_fops.c:(.text+0x4be): undefined reference to `zap_vma_ptes'

Ugh, this one is from a nommu build ("# CONFIG_MMU is not set"), it
makes sense that you can't zap PTEs when you don't have any PTEs at
all... what really impresses me about this is that the rest of the
code compiles on nommu. I'm pretty sure this codepath wouldn't
actually _work_ on nommu, but apparently compiling it works?

I don't know what the right fix is here - should the entire comedi
driver be gated on CONFIG_MMU, or only a subset of the mmap handler,
or something else?

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Jann Horn]

On Wed, Oct 16, 2024 at 8:36 PM kernel test robot <lkp@intel.com> wrote:
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/comedi-Flush-partial-mappings-in-error-case/20241016-022809
> base:   6485cf5ea253d40d507cd71253c9568c5470cd27
> patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a%40google.com
> patch subject: [PATCH v2] comedi: Flush partial mappings in error case
> config: m68k-randconfig-r071-20241016 (https://download.01.org/0day-ci/archive/20241017/202410170234.hthSWOJg-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 14.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170234.hthSWOJg-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410170234.hthSWOJg-lkp@intel.com/
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
> WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
> >> ERROR: modpost: "zap_vma_ptes" [drivers/comedi/comedi.ko] undefined!

And this one is also a nommu build, so I guess it's the same problem
as https://lore.kernel.org/r/202410170111.K30oyTWa-lkp@intel.com even
though the error is different.

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Ian Abbott]

On 16/10/2024 23:05, Jann Horn wrote:
> On Wed, Oct 16, 2024 at 8:05 PM kernel test robot <lkp@intel.com> wrote:
>> [auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/comedi-Flush-partial-mappings-in-error-case/20241016-022809
>> base:   6485cf5ea253d40d507cd71253c9568c5470cd27
>> patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-cafb0e27dd9a%40google.com
>> patch subject: [PATCH v2] comedi: Flush partial mappings in error case
>> config: arm-randconfig-004-20241016 (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/config)
>> compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170111.K30oyTWa-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202410170111.K30oyTWa-lkp@intel.com/
>>
>> All errors (new ones prefixed by >>):
>>
>>     arm-linux-gnueabi-ld: drivers/comedi/comedi_fops.o: in function `comedi_mmap':
>>>> comedi_fops.c:(.text+0x4be): undefined reference to `zap_vma_ptes'
> 
> Ugh, this one is from a nommu build ("# CONFIG_MMU is not set"), it
> makes sense that you can't zap PTEs when you don't have any PTEs at
> all... what really impresses me about this is that the rest of the
> code compiles on nommu. I'm pretty sure this codepath wouldn't
> actually _work_ on nommu, but apparently compiling it works?
> 
> I don't know what the right fix is here - should the entire comedi
> driver be gated on CONFIG_MMU, or only a subset of the mmap handler,
> or something else?

Given that it would also affect a lot of fbdev drivers that would also 
benefit from zapping partial mappings, I suggest that gating on 
CONFIG_MMU would not be the correct fix.


-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Ian Abbott]

On 17/10/2024 10:29, Ian Abbott wrote:
> On 16/10/2024 23:05, Jann Horn wrote:
>> On Wed, Oct 16, 2024 at 8:05 PM kernel test robot <lkp@intel.com> wrote:
>>> [auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/ 
>>> comedi-Flush-partial-mappings-in-error-case/20241016-022809
>>> base:   6485cf5ea253d40d507cd71253c9568c5470cd27
>>> patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1- 
>>> cafb0e27dd9a%40google.com
>>> patch subject: [PATCH v2] comedi: Flush partial mappings in error case
>>> config: arm-randconfig-004-20241016 (https://download.01.org/0day-ci/ 
>>> archive/20241017/202410170111.K30oyTWa-lkp@intel.com/config)
>>> compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/ 
>>> archive/20241017/202410170111.K30oyTWa-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new 
>>> version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild- 
>>> all/202410170111.K30oyTWa-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>     arm-linux-gnueabi-ld: drivers/comedi/comedi_fops.o: in function 
>>> `comedi_mmap':
>>>>> comedi_fops.c:(.text+0x4be): undefined reference to `zap_vma_ptes'
>>
>> Ugh, this one is from a nommu build ("# CONFIG_MMU is not set"), it
>> makes sense that you can't zap PTEs when you don't have any PTEs at
>> all... what really impresses me about this is that the rest of the
>> code compiles on nommu. I'm pretty sure this codepath wouldn't
>> actually _work_ on nommu, but apparently compiling it works?
>>
>> I don't know what the right fix is here - should the entire comedi
>> driver be gated on CONFIG_MMU, or only a subset of the mmap handler,
>> or something else?
> 
> Given that it would also affect a lot of fbdev drivers that would also 
> benefit from zapping partial mappings, I suggest that gating on 
> CONFIG_MMU would not be the correct fix.

Perhaps just add an #ifdef CONFIG_MMU around the affected call for now?

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

Re: [PATCH v2] comedi: Flush partial mappings in error case

[Jann Horn]

On Thu, Oct 17, 2024 at 11:48 AM Ian Abbott <abbotti@mev.co.uk> wrote:
> On 17/10/2024 10:29, Ian Abbott wrote:
> > On 16/10/2024 23:05, Jann Horn wrote:
> >> On Wed, Oct 16, 2024 at 8:05 PM kernel test robot <lkp@intel.com> wrote:
> >>> [auto build test ERROR on 6485cf5ea253d40d507cd71253c9568c5470cd27]
> >>>
> >>> url:    https://github.com/intel-lab-lkp/linux/commits/Jann-Horn/
> >>> comedi-Flush-partial-mappings-in-error-case/20241016-022809
> >>> base:   6485cf5ea253d40d507cd71253c9568c5470cd27
> >>> patch link:    https://lore.kernel.org/r/20241015-comedi-tlb-v2-1-
> >>> cafb0e27dd9a%40google.com
> >>> patch subject: [PATCH v2] comedi: Flush partial mappings in error case
> >>> config: arm-randconfig-004-20241016 (https://download.01.org/0day-ci/
> >>> archive/20241017/202410170111.K30oyTWa-lkp@intel.com/config)
> >>> compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
> >>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/
> >>> archive/20241017/202410170111.K30oyTWa-lkp@intel.com/reproduce)
> >>>
> >>> If you fix the issue in a separate patch/commit (i.e. not just a new
> >>> version of
> >>> the same patch/commit), kindly add following tags
> >>> | Reported-by: kernel test robot <lkp@intel.com>
> >>> | Closes: https://lore.kernel.org/oe-kbuild-
> >>> all/202410170111.K30oyTWa-lkp@intel.com/
> >>>
> >>> All errors (new ones prefixed by >>):
> >>>
> >>>     arm-linux-gnueabi-ld: drivers/comedi/comedi_fops.o: in function
> >>> `comedi_mmap':
> >>>>> comedi_fops.c:(.text+0x4be): undefined reference to `zap_vma_ptes'
> >>
> >> Ugh, this one is from a nommu build ("# CONFIG_MMU is not set"), it
> >> makes sense that you can't zap PTEs when you don't have any PTEs at
> >> all... what really impresses me about this is that the rest of the
> >> code compiles on nommu. I'm pretty sure this codepath wouldn't
> >> actually _work_ on nommu, but apparently compiling it works?
> >>
> >> I don't know what the right fix is here - should the entire comedi
> >> driver be gated on CONFIG_MMU, or only a subset of the mmap handler,
> >> or something else?
> >
> > Given that it would also affect a lot of fbdev drivers that would also
> > benefit from zapping partial mappings, I suggest that gating on
> > CONFIG_MMU would not be the correct fix.
>
> Perhaps just add an #ifdef CONFIG_MMU around the affected call for now?

Sure, I guess that works, though it's not particularly pretty.
(And this codepath looks like it won't really work on nommu either way...)

I'll change it that way for now.