Fix for vma merging refcounting bug

Fix for vma merging refcounting bug

Post by Stephen C. Tweedi » Sat, 10 May 2003 14:40:11



When a new vma can be merged simultaneously with its two immediate
neighbours in both directions, vma_merge() extends the predecessor vma
and deletes the successor.  However, if the vma maps a file, it fails to
fput() when doing the delete, leaving the file's refcount inconsistent.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#                  ChangeSet    1.1083  -> 1.1084
#                  mm/mmap.c    1.79    -> 1.80  
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------

# Fix vma merging problem leading to file refcount getting out of sync.
# --------------------------------------------
#
diff -Nru a/mm/mmap.c b/mm/mmap.c
--- a/mm/mmap.c Fri May  9 13:26:53 2003

                        spin_unlock(lock);
                        if (need_up)
                                up(&inode->i_mapping->i_shared_sem);
+                       if (file)
+                               fput(file);

                        mm->map_count--;
                        kmem_cache_free(vm_area_cachep, next);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

Fix for vma merging refcounting bug

Post by Andrea Arcangel » Sun, 11 May 2003 18:40:13



> When a new vma can be merged simultaneously with its two immediate
> neighbours in both directions, vma_merge() extends the predecessor vma
> and deletes the successor.  However, if the vma maps a file, it fails to
> fput() when doing the delete, leaving the file's refcount inconsistent.

> # This is a BitKeeper generated patch for the following project:
> # Project Name: Linux kernel tree
> # This patch format is intended for GNU patch command version 2.5 or higher.
> # This patch includes the following deltas:
> #             ChangeSet    1.1083  -> 1.1084
> #             mm/mmap.c    1.79    -> 1.80  
> #
> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------

> # Fix vma merging problem leading to file refcount getting out of sync.
> # --------------------------------------------
> #
> diff -Nru a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c    Fri May  9 13:26:53 2003
> +++ b/mm/mmap.c    Fri May  9 13:26:53 2003

>                    spin_unlock(lock);
>                    if (need_up)
>                            up(&inode->i_mapping->i_shared_sem);
> +                  if (file)
> +                          fput(file);

>                    mm->map_count--;
>                    kmem_cache_free(vm_area_cachep, next);

great catch! nobody could notice it in practice but it's definitely
needed, especially after long uptimes it could be noticeable ;), thanks!

I'm attaching for review what I'm applying to my -aa tree, to fix the
above and the other issue with the non-ram vma merging fixed in 2.5.

Please review, thanks again!

--- CGL/include/linux/mm.h.~1~  2003-05-07 23:39:00.000000000 +0200

                mm->mmap_cache = prev;
 }

+#define VM_SPECIAL (VM_IO | VM_DONTCOPY | VM_DONTEXPAND | VM_RESERVED)
+
 #define can_vma_merge(vma, vm_flags) __can_vma_merge(vma, vm_flags, NULL, 0, 0)
 static inline int __can_vma_merge(struct vm_area_struct * vma, unsigned long vm_flags,
                                  struct file * file, unsigned long vm_pgoff, unsigned long offset)
 {
-       if (vma->vm_file == file && vma->vm_flags == vm_flags) {
+       if (vma->vm_file == file && vma->vm_flags == vm_flags &&
+           likely((!vma->vm_ops || !vma->vm_ops->close) && !vma->vm_private_data &&
+                  !(vm_flags & VM_SPECIAL))) {
                if (file) {
                        if (vma->vm_pgoff == vm_pgoff + offset) {
                                if ((long) offset > 0 && vm_pgoff + offset < vm_pgoff)
--- CGL/mm/mmap.c.~1~   2003-05-07 23:39:42.000000000 +0200

                        spin_unlock(lock);
                        if (need_unlock)
                                unlock_vma_mappings(next);
+                       if (file)
+                               fput(file);

                        mm->map_count--;
                        kmem_cache_free(vm_area_cachep, next);

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

Fix for vma merging refcounting bug

Post by Stephen C. Tweedi » Mon, 12 May 2003 22:10:05


Hi,



> > When a new vma can be merged simultaneously with its two immediate
> > neighbours in both directions, vma_merge() extends the predecessor vma
> > and deletes the successor.  However, if the vma maps a file, it fails to
> > fput() when doing the delete, leaving the file's refcount inconsistent.
> great catch! nobody could notice it in practice

Yep --- I only noticed it because I was running a quick-and-dirty vma
merging test and wanted to test on a shmfs file, and noticed that the
temporary shmfs filesystem became unmountable afterwards.  Test
attached, in case anybody is interested (it's the third test, mapping a
file page by page in two interleaved passes, which triggers this case.)

Quote:> I'm attaching for review what I'm applying to my -aa tree, to fix the
> above and the other issue with the non-ram vma merging fixed in 2.5.

Looks OK.

Cheers,
 Stephen

  vma-merge.c
2K Download
 
 
 

Fix for vma merging refcounting bug

Post by Andrea Arcangel » Thu, 15 May 2003 01:00:24



> Hi,



> > > When a new vma can be merged simultaneously with its two immediate
> > > neighbours in both directions, vma_merge() extends the predecessor vma
> > > and deletes the successor.  However, if the vma maps a file, it fails to
> > > fput() when doing the delete, leaving the file's refcount inconsistent.

> > great catch! nobody could notice it in practice

> Yep --- I only noticed it because I was running a quick-and-dirty vma
> merging test and wanted to test on a shmfs file, and noticed that the
> temporary shmfs filesystem became unmountable afterwards.  Test
> attached, in case anybody is interested (it's the third test, mapping a
> file page by page in two interleaved passes, which triggers this case.)

> > I'm attaching for review what I'm applying to my -aa tree, to fix the
> > above and the other issue with the non-ram vma merging fixed in 2.5.

> Looks OK.

actually I just noticed the fput is never been buggy in my tree:

        if (!file || !rb_parent || !vma_merge(mm, prev, rb_parent, addr, addr + len, vma->vm_flags, file, pgoff)) {
                vma_link(mm, vma, prev, rb_link, rb_parent);
                if (correct_wcount)
                        atomic_inc(&file->f_dentry->d_inode->i_writecount);
        } else {
                if (file) {
                        if (correct_wcount)
                                atomic_inc(&file->f_dentry->d_inode->i_writecount);
                        fput(file);
                        ^^^^^^^^^
                }
                kmem_cache_free(vm_area_cachep, vma);
        }

so this was a merging bug in 2.5

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

Fix for vma merging refcounting bug

Post by Andrea Arcangel » Thu, 15 May 2003 01:10:13




> > Hi,



> > > > When a new vma can be merged simultaneously with its two immediate
> > > > neighbours in both directions, vma_merge() extends the predecessor vma
> > > > and deletes the successor.  However, if the vma maps a file, it fails to
> > > > fput() when doing the delete, leaving the file's refcount inconsistent.

> > > great catch! nobody could notice it in practice

> > Yep --- I only noticed it because I was running a quick-and-dirty vma
> > merging test and wanted to test on a shmfs file, and noticed that the
> > temporary shmfs filesystem became unmountable afterwards.  Test
> > attached, in case anybody is interested (it's the third test, mapping a
> > file page by page in two interleaved passes, which triggers this case.)

> > > I'm attaching for review what I'm applying to my -aa tree, to fix the
> > > above and the other issue with the non-ram vma merging fixed in 2.5.

> > Looks OK.

> actually I just noticed the fput is never been buggy in my tree:

>    if (!file || !rb_parent || !vma_merge(mm, prev, rb_parent, addr, addr + len, vma->vm_flags, file, pgoff)) {
>            vma_link(mm, vma, prev, rb_link, rb_parent);
>            if (correct_wcount)
>                    atomic_inc(&file->f_dentry->d_inode->i_writecount);
>    } else {
>            if (file) {
>                    if (correct_wcount)
>                            atomic_inc(&file->f_dentry->d_inode->i_writecount);
>                    fput(file);
>                    ^^^^^^^^^
>            }
>            kmem_cache_free(vm_area_cachep, vma);
>    }

> so this was a merging bug in 2.5

Apologies, I was on the laptop reading that code wrong and I sent the
email too early, the patch I posted was correct for my tree too indeed.

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

1. vma rbtree and vma merging

I rewrote the vma lookup engine with a rbtree, it drops many browse of
the tree in many places this way and it drops the complexity of
rebalancing the tree as well. There is still some room for improvements
(get_unmapped_area walks the tree one more time in the non MAP_FIXED
case and we could checkpoint there but it would pollute some other code
and I thought it was a lower prio).

It also hopefully does the right merging for the anon mappings for mmap,
mremap and mprotect in all the cases (I got some mremap bit from Ben's
patch).  It's still very early work so don't apply it in any production
box but it works for me so far (running kde pre-2.9 over it, konqueror,
mutt, emacs, and some test programs and found no problems yet). I also
included it under the name of 70_mmap-rb-3 in the 2.4.9aa1 patchkit but
please make sure to back it out from the patchkit before doing any
production work. It needs to be tested better and it also needs to be
benchmarked carefully (should be a speed improvement).

against 2.4.9 vanilla:

        ftp://ftp.kernel.org/pub/linux/kernel/people/andrea/patches/v2.4/2.4....

Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

2. 2 Passwords for a BSDI Shell Account?

3. VMA Merging

4. ANN: W3C CSS Validator FAQ

5. Merge expired license bug not fully fixed by patch

6. The 2.6 kernel tree

7. fix obj vma sorting

8. Naive framebuffer question

9. RFCOMM BUG - Modules Refcount - 2.5.73

10. Netscapes libraries fix / Does not fix java bugs though

11. SUMMARY: SVR4: bugs and bug fixes

12. Slab cache name fixes / reiserfs boot bug fix.

13. linux.fix.temp: Linux bug fix report template