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
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
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-partia... 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@i...) 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@i...)
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'
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-partia... 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@i...) 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@i...)
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?
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-partia... 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@i...) 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@i...)
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.
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?
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.
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-partia... 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@i...) 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@i...)
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!
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-partia... 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@i...) 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@i...)
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.
linux-stable-mirror@lists.linaro.org
-
Ian Abbott -
Jann Horn -
kernel test robot