[patch] mm: recheck lock rlim after f_op->mmap() method

[patch] mm: recheck lock rlim after f_op->mmap() method

[Dmitry Monakhov]

Some device drivers can change vm_flags in their f_op->mmap
method. In order to be on the safe side we have to recheck
lock rlimit. Now we have to check lock rlimit from two places,
let's move this common code to helper functon.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 mm/mmap.c |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 906ed40..5c89f1d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -885,6 +885,18 @@ void vm_stat_account(struct mm_struct *mm, unsigned long flags,
 }
 #endif /* CONFIG_PROC_FS */
 
+static int check_lock_limit(unsigned long delta, struct mm_struct* mm)
+{
+	unsigned long locked, lock_limit;
+	locked = delta >> PAGE_SHIFT;
+	locked += mm->locked_vm;
+	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
+	lock_limit >>= PAGE_SHIFT;
+	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
+		return -EAGAIN;
+	return 0;
+}
+
 /*
  * The caller must hold down_write(current->mm->mmap_sem).
  */
@@ -954,13 +966,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
 	}
 	/* mlock MCL_FUTURE? */
 	if (vm_flags & VM_LOCKED) {
-		unsigned long locked, lock_limit;
-		locked = len >> PAGE_SHIFT;
-		locked += mm->locked_vm;
-		lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
-		lock_limit >>= PAGE_SHIFT;
-		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
-			return -EAGAIN;
+		error = check_lock_limit(len, mm);
+		if (error)
+			return error;
 	}
 
 	inode = file ? file->f_path.dentry->d_inode : NULL;
@@ -1101,6 +1109,17 @@ munmap_back:
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
+		
+		if (vma->vm_flags & VM_LOCKED
+				&& !(vm_flags & VM_LOCKED)) {
+		/*
+		 * VM_LOCKED was added in f_op->mmap() method,
+		 * so we have to recheck limit.
+		 */
+			error = check_lock_limit(len, mm);
+			if (error)
+				goto unmap_and_free_vma;
+		}
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
 		if (error)
-- 
1.5.2.2

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Hugh Dickins]

On Mon, 9 Jul 2007, Dmitry Monakhov wrote:
> Some device drivers can change vm_flags in their f_op->mmap
> method. In order to be on the safe side we have to recheck
> lock rlimit. Now we have to check lock rlimit from two places,
> let's move this common code to helper functon.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)

Or would this simpler patch be the right one?  I suspect the
mspec driver only says VM_LOCKED because of a deep-seated but
irrational fear that its pages might fall into reclaim.

(I'd like to take out VM_RESERVED too, but that can always happen
another, indefinitely postponed time; there are others of those.)

Hugh

--- 2.6.22/drivers/char/mspec.c	2007-04-26 04:08:32.000000000 +0100
+++ linux/drivers/char/mspec.c	2007-07-10 18:12:11.000000000 +0100
@@ -265,7 +265,7 @@ mspec_mmap(struct file *file, struct vm_
 	vdata->refcnt = ATOMIC_INIT(1);
 	vma->vm_private_data = vdata;
 
-	vma->vm_flags |= (VM_IO | VM_LOCKED | VM_RESERVED | VM_PFNMAP);
+	vma->vm_flags |= (VM_IO | VM_RESERVED | VM_PFNMAP);
 	if (vdata->type == MSPEC_FETCHOP || vdata->type == MSPEC_UNCACHED)
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	vma->vm_ops = &mspec_vm_ops;

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Dmitry Monakhov]

On 18:27 Втр 10 Июл     , Hugh Dickins wrote:
> On Mon, 9 Jul 2007, Dmitry Monakhov wrote:
> > Some device drivers can change vm_flags in their f_op->mmap
> > method. In order to be on the safe side we have to recheck
> > lock rlimit. Now we have to check lock rlimit from two places,
> > let's move this common code to helper functon.
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
> >  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> Or would this simpler patch be the right one?  I suspect the
> mspec driver only says VM_LOCKED because of a deep-seated but
> irrational fear that its pages might fall into reclaim.
No mspec is not the only one :( , in my case it was fglrx (ati driver).
BTW: where is comment about it in do_mmap_pgoff:  
	/* Can addr have changed??
	 *
	 * Answer: Yes, several device drivers can do
	 * it in their
	 *         f_op->mmap method. -DaveM
	 */
	addr = vma->vm_start;
	pgoff = vma->vm_pgoff;
	vm_flags = vma->vm_flags;
> 
> (I'd like to take out VM_RESERVED too, but that can always happen
> another, indefinitely postponed time; there are others of those.)
> 
> Hugh
[skip]

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1063 bytes --]

On Tue, 10 Jul 2007, Dmitry Monakhov wrote:
> On 18:27 Втр 10 Июл     , Hugh Dickins wrote:
> > Or would this simpler patch be the right one?  I suspect the
> > mspec driver only says VM_LOCKED because of a deep-seated but
> > irrational fear that its pages might fall into reclaim.
> No mspec is not the only one :( , in my case it was fglrx (ati driver).

Ah, then fglrx is wrong too.  We don't want to check locked_vm,
but we don't much want it to wrap negative later.  Perhaps we
should mask VM_LOCKED off, perhaps we should BUG, perhaps we
should ask ATI/AMD to change it (a BUG would be persuasive ;)

> BTW: where is comment about it in do_mmap_pgoff:  

Sorry, I don't understand.  You'd like a comment saying which flags
the driver may change and which it may not?  Hmmm... not today.

> 	/* Can addr have changed??
> 	 *
> 	 * Answer: Yes, several device drivers can do
> 	 * it in their
> 	 *         f_op->mmap method. -DaveM
> 	 */
> 	addr = vma->vm_start;
> 	pgoff = vma->vm_pgoff;
> 	vm_flags = vma->vm_flags;

Hugh

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Jes Sorensen]

Hugh Dickins wrote:
> On Mon, 9 Jul 2007, Dmitry Monakhov wrote:
>> Some device drivers can change vm_flags in their f_op->mmap
>> method. In order to be on the safe side we have to recheck
>> lock rlimit. Now we have to check lock rlimit from two places,
>> let's move this common code to helper functon.
>>
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
>>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> Or would this simpler patch be the right one?  I suspect the
> mspec driver only says VM_LOCKED because of a deep-seated but
> irrational fear that its pages might fall into reclaim.
> 
> (I'd like to take out VM_RESERVED too, but that can always happen
> another, indefinitely postponed time; there are others of those.)

Hi Hugh,

My mind is rusty on this one, so I checked with Robin Holt and his
is too .... most likely mspec has it for legacy problems, if your
change shows up to cause new problems, we'll fix them when they appear.
So from our side, it's fine to go ahead with this patch.

Cheers,
Jes

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Christoph Hellwig]

On Tue, Jul 10, 2007 at 06:27:05PM +0100, Hugh Dickins wrote:
> On Mon, 9 Jul 2007, Dmitry Monakhov wrote:
> > Some device drivers can change vm_flags in their f_op->mmap
> > method. In order to be on the safe side we have to recheck
> > lock rlimit. Now we have to check lock rlimit from two places,
> > let's move this common code to helper functon.
> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
> >  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> Or would this simpler patch be the right one?  I suspect the
> mspec driver only says VM_LOCKED because of a deep-seated but
> irrational fear that its pages might fall into reclaim.

Looks good.  We probably should add a debug check to do_mmap_pgoff
so that ->mmap methods don't change flags that are not for drivers.

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Dmitry Monakhov]

On 10:32 Срд 11 Июл     , Christoph Hellwig wrote:
> On Tue, Jul 10, 2007 at 06:27:05PM +0100, Hugh Dickins wrote:
> > On Mon, 9 Jul 2007, Dmitry Monakhov wrote:
> > > Some device drivers can change vm_flags in their f_op->mmap
> > > method. In order to be on the safe side we have to recheck
> > > lock rlimit. Now we have to check lock rlimit from two places,
> > > let's move this common code to helper functon.
> > > 
> > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > > ---
> > >  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
> > >  1 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > Or would this simpler patch be the right one?  I suspect the
> > mspec driver only says VM_LOCKED because of a deep-seated but
> > irrational fear that its pages might fall into reclaim.
> 
> Looks good.  We probably should add a debug check to do_mmap_pgoff
> so that ->mmap methods don't change flags that are not for drivers.
As result fglrx totally goes crazy, because it change vm_flags
even from  ->nopage() calback :)
> 

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Christoph Hellwig]

On Wed, Jul 11, 2007 at 02:12:45PM +0400, Dmitry Monakhov wrote:
> > > Or would this simpler patch be the right one?  I suspect the
> > > mspec driver only says VM_LOCKED because of a deep-seated but
> > > irrational fear that its pages might fall into reclaim.
> > 
> > Looks good.  We probably should add a debug check to do_mmap_pgoff
> > so that ->mmap methods don't change flags that are not for drivers.
> As result fglrx totally goes crazy, because it change vm_flags
> even from  ->nopage() calback :)

Well, everyone with half a brain knows that fglrx is not just legally
problematic but an utter piece of junk.  We should add more debug checks
to stop it from doing such stupid things.  And yes, chaning flags from
->nopage() does not just deserve a warning but a panic.

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Hugh Dickins]

On Wed, 11 Jul 2007, Christoph Hellwig wrote:
> On Wed, Jul 11, 2007 at 02:12:45PM +0400, Dmitry Monakhov wrote:
> > > > Or would this simpler patch be the right one?  I suspect the
> > > > mspec driver only says VM_LOCKED because of a deep-seated but
> > > > irrational fear that its pages might fall into reclaim.

I was perhaps too unkind: that fear was not irrational in 2.4,
when reclaim scanned vmas; but in 2.6, pages have to be on one
of reclaim's LRUs to fall vulnerable to it.

> > > 
> > > Looks good.  We probably should add a debug check to do_mmap_pgoff
> > > so that ->mmap methods don't change flags that are not for drivers.

We could indeed, though I'd rather not jump into that: something
to do when we tidy up those driver mmaps (something I promised to
do 18 months ago?), there's a lot of pointless flag setting.

> > As result fglrx totally goes crazy, because it change vm_flags
> > even from  ->nopage() calback :)

That must be an exciting new State of the Art version of fglrx.

Looking at what I downloaded for inspection a year ago, I can't
see any sign of that.  But I do see lots of random stabs at setting
different vm_flags in different mmap cases (VM_IO, VM_SHM, VM_RESERVED,
VM_LOCKED): of which the VM_IO serves some point, VM_SHM is defunct,
VM_RESERVED will be defunct, and VM_LOCKED can confuse us.

But interestingly, where they set VM_LOCKED, they did increment
vm_locked: so there shouldn't be that issue of wrapping negative
on munmap, which had worried me.  And now I look at Dmitry's
patch again, I see that he was indeed assuming that the driver
had done that incrementation.

> Well, everyone with half a brain knows that fglrx is not just legally
> problematic but an utter piece of junk.  We should add more debug checks
> to stop it from doing such stupid things.  And yes, chaning flags from
> ->nopage() does not just deserve a warning but a panic.

Well, they'll get what they deserve.  I'm not convinced Dmitry's right
about their nopage; and I'm not going to waste any more time working
out ways to protect ourselves from them.  Let's stick with the mspec.

Hugh

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Hugh Dickins]

On Wed, 11 Jul 2007, Jes Sorensen wrote:
> Hugh Dickins wrote:
> > 
> > Or would this simpler patch be the right one?  I suspect the
> > mspec driver only says VM_LOCKED because of a deep-seated but
> > irrational fear that its pages might fall into reclaim.
> 
> My mind is rusty on this one, so I checked with Robin Holt and his
> is too .... most likely mspec has it for legacy problems, if your
> change shows up to cause new problems, we'll fix them when they appear.
> So from our side, it's fine to go ahead with this patch.

Thanks a lot, Jes: you're right, it probably did something useful
(though incorrectly) in 2.4, but we'd better get rid of it now.
I've Cc'ed Andrew, hoping he'll magically pick up the patch...


[PATCH] mspec_mmap don't set VM_LOCKED

mspec_mmap was setting VM_LOCKED (without adjusting locked_vm):
don't do that, it serves no purpose in 2.6, other than to mess
up the locked_vm accounting - mspec's pages won't get reclaimed
anyway.  Thanks to Dmitry Monakhov for raising the issue.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
Acked-by: Jes Sorensen <jes@sgi.com>

--- 2.6.22/drivers/char/mspec.c	2007-04-26 04:08:32.000000000 +0100
+++ linux/drivers/char/mspec.c	2007-07-10 18:12:11.000000000 +0100
@@ -265,7 +265,7 @@ mspec_mmap(struct file *file, struct vm_
 	vdata->refcnt = ATOMIC_INIT(1);
 	vma->vm_private_data = vdata;
 
-	vma->vm_flags |= (VM_IO | VM_LOCKED | VM_RESERVED | VM_PFNMAP);
+	vma->vm_flags |= (VM_IO | VM_RESERVED | VM_PFNMAP);
 	if (vdata->type == MSPEC_FETCHOP || vdata->type == MSPEC_UNCACHED)
 		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	vma->vm_ops = &mspec_vm_ops;

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Andrew Morton]

On Mon, 9 Jul 2007 22:49:17 +0400 Dmitry Monakhov <dmonakhov@sw.ru> wrote:

> Some device drivers can change vm_flags in their f_op->mmap
> method. In order to be on the safe side we have to recheck
> lock rlimit. Now we have to check lock rlimit from two places,
> let's move this common code to helper functon.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 906ed40..5c89f1d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -885,6 +885,18 @@ void vm_stat_account(struct mm_struct *mm, unsigned long flags,
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +static int check_lock_limit(unsigned long delta, struct mm_struct* mm)
> +{
> +	unsigned long locked, lock_limit;
> +	locked = delta >> PAGE_SHIFT;
> +	locked += mm->locked_vm;
> +	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> +	lock_limit >>= PAGE_SHIFT;
> +	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> +		return -EAGAIN;
> +	return 0;
> +}
> +
>  /*
>   * The caller must hold down_write(current->mm->mmap_sem).
>   */
> @@ -954,13 +966,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
>  	}
>  	/* mlock MCL_FUTURE? */
>  	if (vm_flags & VM_LOCKED) {
> -		unsigned long locked, lock_limit;
> -		locked = len >> PAGE_SHIFT;
> -		locked += mm->locked_vm;
> -		lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> -		lock_limit >>= PAGE_SHIFT;
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			return -EAGAIN;
> +		error = check_lock_limit(len, mm);
> +		if (error)
> +			return error;
>  	}
>  
>  	inode = file ? file->f_path.dentry->d_inode : NULL;
> @@ -1101,6 +1109,17 @@ munmap_back:
>  		error = file->f_op->mmap(file, vma);
>  		if (error)
>  			goto unmap_and_free_vma;
> +		
> +		if (vma->vm_flags & VM_LOCKED
> +				&& !(vm_flags & VM_LOCKED)) {
> +		/*
> +		 * VM_LOCKED was added in f_op->mmap() method,
> +		 * so we have to recheck limit.
> +		 */
> +			error = check_lock_limit(len, mm);
> +			if (error)
> +				goto unmap_and_free_vma;
> +		}

Worried.  As far as the filesytem is concerned, its mmap has succeeded.

But now we're taking the unmap_and_free_vma path _after_ ->mmap() has
"succeeded".  So we will now tell userspace that the mmap syscall has
failed, even though the fs thinks it succeeded, if you follow me.  And this
is a new thing.  

Could it cause bad things to happen?  Well, if filesystems had a
file_operations.munmap() then yeah, we should have called that in your new
code.  But filesystems don't have a ->munmap() method.

Still.  Can we think of any way in which this change could lead to resource
leaks or to any other such problems?

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Christoph Hellwig]

On Wed, Jul 11, 2007 at 07:03:36PM +0100, Hugh Dickins wrote:
> Well, they'll get what they deserve.  I'm not convinced Dmitry's right
> about their nopage; and I'm not going to waste any more time working
> out ways to protect ourselves from them.  Let's stick with the mspec.

I'm not just concerned about the ati driver.  There's a lot of driver
that do or did really stupid stuff with mmap, including in-tree drivers.
the actual mmap() code is not actually a fastpath so additional sanity
checks can't hurt.  It might not be worth the effort for ->nopage, though.

Re: [patch] mm: recheck lock rlim after f_op->mmap() method

[Hugh Dickins]

On Fri, 13 Jul 2007, Andrew Morton wrote:
> On Mon, 9 Jul 2007 22:49:17 +0400 Dmitry Monakhov <dmonakhov@sw.ru> wrote:
> 
> > Some device drivers can change vm_flags in their f_op->mmap
> > method. In order to be on the safe side we have to recheck
> > lock rlimit. Now we have to check lock rlimit from two places,
> > let's move this common code to helper functon.

I think you've made a mistake putting this one into your -mm tree:
I thought we'd replaced it all by the one-liner to mspec.c
[PATCH] mspec_mmap don't set VM_LOCKED

There is no justification for a driver to set VM_LOCKED these days
(even in 2.4 they were supposed to set VM_RESERVED instead).  So
all this to handle a driver setting VM_LOCKED is inappropriate.

mspec was the only driver doing it in tree, but ATI's fglrx is the
one Dmitry spotted (and it takes care to adjust the locked_vm count,
so that won't even wrap negative later - the worst that happens is
they prevent the process using their driver from locking as much
memory as it might wish to - or that's how it looked to me).

The only debate is what we might do in the way of extra checks after
coming back from a driver's ->mmap.  WARN_ON(vma->vm_flags & VM_LOCKED)?
And which other flags?  hch is keen to put a check in there, I'm less
interested, and haven't begun to think of all the things we might wish
to check if we're going to distrust the driver here, and wherever else.

But that would be the way to go, not Dmitry's check_lock_limit patch:
please remove it from -mm after all.

One more comment below...

> > 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  mm/mmap.c |   33 ++++++++++++++++++++++++++-------
> >  1 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 906ed40..5c89f1d 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -885,6 +885,18 @@ void vm_stat_account(struct mm_struct *mm, unsigned long flags,
> >  }
> >  #endif /* CONFIG_PROC_FS */
> >  
> > +static int check_lock_limit(unsigned long delta, struct mm_struct* mm)
> > +{
> > +	unsigned long locked, lock_limit;
> > +	locked = delta >> PAGE_SHIFT;
> > +	locked += mm->locked_vm;
> > +	lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> > +	lock_limit >>= PAGE_SHIFT;
> > +	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> > +		return -EAGAIN;
> > +	return 0;
> > +}
> > +
> >  /*
> >   * The caller must hold down_write(current->mm->mmap_sem).
> >   */
> > @@ -954,13 +966,9 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
> >  	}
> >  	/* mlock MCL_FUTURE? */
> >  	if (vm_flags & VM_LOCKED) {
> > -		unsigned long locked, lock_limit;
> > -		locked = len >> PAGE_SHIFT;
> > -		locked += mm->locked_vm;
> > -		lock_limit = current->signal->rlim[RLIMIT_MEMLOCK].rlim_cur;
> > -		lock_limit >>= PAGE_SHIFT;
> > -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> > -			return -EAGAIN;
> > +		error = check_lock_limit(len, mm);
> > +		if (error)
> > +			return error;
> >  	}
> >  
> >  	inode = file ? file->f_path.dentry->d_inode : NULL;
> > @@ -1101,6 +1109,17 @@ munmap_back:
> >  		error = file->f_op->mmap(file, vma);
> >  		if (error)
> >  			goto unmap_and_free_vma;
> > +		
> > +		if (vma->vm_flags & VM_LOCKED
> > +				&& !(vm_flags & VM_LOCKED)) {
> > +		/*
> > +		 * VM_LOCKED was added in f_op->mmap() method,
> > +		 * so we have to recheck limit.
> > +		 */
> > +			error = check_lock_limit(len, mm);
> > +			if (error)
> > +				goto unmap_and_free_vma;
> > +		}
> 
> Worried.  As far as the filesytem is concerned, its mmap has succeeded.
> 
> But now we're taking the unmap_and_free_vma path _after_ ->mmap() has
> "succeeded".  So we will now tell userspace that the mmap syscall has
> failed, even though the fs thinks it succeeded, if you follow me.  And this
> is a new thing.  
> 
> Could it cause bad things to happen?  Well, if filesystems had a
> file_operations.munmap() then yeah, we should have called that in your new
> code.  But filesystems don't have a ->munmap() method.
> 
> Still.  Can we think of any way in which this change could lead to resource
> leaks or to any other such problems?

All very good thoughts, but we don't even need to think them!

Hugh