On Mon, 11 Mar 2013 17:24:29 -0700 Michel Lespinasse <[email protected]> wrote: > > --- a/mm/fremap.c~mm-fremapc-fix-oops-on-error-path > > +++ a/mm/fremap.c > > @@ -163,7 +163,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsign > > * and that the remapped range is valid and fully within > > * the single existing vma. > > */ > > - if (!vma || !(vma->vm_flags & VM_SHARED)) > > + vm_flags = vma->vm_flags; > > + if (!vma || !(vm_flags & VM_SHARED)) > > goto out; > > Your commit message indicates the vm_flags load here doesn't generate any > code, but this seems very brittle and compiler dependent. If the compiler was > to generate an actual load here, the issue with vma == NULL would reappear.
I didn't try very hard. I have a surprisingly strong dislike of adding "= 0" everywhere just to squish warnings. There are actually quite a lot of places where this function could use s/vma->vm_flags/vm_flags/ and might save a bit of code as a result. But the function's pretty straggly and I stopped doing it. > > > if (!vma->vm_ops || !vma->vm_ops->remap_pages) > > @@ -254,7 +255,8 @@ get_write_lock: > > */ > > > > out: > > - vm_flags = vma->vm_flags; > > + if (vma) > > + vm_flags = vma->vm_flags; > > if (likely(!has_write_lock)) > > up_read(&mm->mmap_sem); > > else > > > > Would the following work ? I think it's simpler, and with the compiler > I'm using here it doesn't emit warnings: > > diff --git a/mm/fremap.c b/mm/fremap.c > index 0cd4c11488ed..329507e832fb 100644 > --- a/mm/fremap.c > +++ b/mm/fremap.c > @@ -254,7 +254,8 @@ get_write_lock: > */ > > out: > - vm_flags = vma->vm_flags; > + if (!err) > + vm_flags = vma->vm_flags; > if (likely(!has_write_lock)) > up_read(&mm->mmap_sem); > else Yes, this will work. gcc-4.4.4 does generate the warning with this. Testing `err' was my v1, but it is not obvious that err==0 always correlates with vma!= NULL. This is true (I checked), and it had better be true in the future, but it just feels safer and simpler to test `vma' directly. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
