| Message ID | 1478233517-3571-12-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 04, 2016 at 05:25:07AM +0100, Jan Kara wrote: > We don't check whether vma->vm_ops is NULL in do_shared_fault() so > there's hardly any point in checking it in wp_page_shared() or > wp_pfn_shared() which get called only for shared file mappings as well. > > Signed-off-by: Jan Kara <jack@suse.cz> Well, I'm not sure about this. do_shared_fault() doesn't have the check since we checked it upper by stack: see vma_is_anonymous() in handle_pte_fault(). In principal, it should be fine. But random crappy driver has potential to blow it up. > --- > mm/memory.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index 7be96a43d5ac..26b2858e6a12 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -2275,7 +2275,7 @@ static int wp_pfn_shared(struct vm_fault *vmf) > { > struct vm_area_struct *vma = vmf->vma; > > - if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) { > + if (vma->vm_ops->pfn_mkwrite) { > int ret; > > pte_unmap_unlock(vmf->pte, vmf->ptl); > @@ -2305,7 +2305,7 @@ static int wp_page_shared(struct vm_fault *vmf, struct page *old_page) > > get_page(old_page); > > - if (vma->vm_ops && vma->vm_ops->page_mkwrite) { > + if (vma->vm_ops->page_mkwrite) { > int tmp; > > pte_unmap_unlock(vmf->pte, vmf->ptl); > -- > 2.6.6 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Wed 16-11-16 01:28:19, Kirill A. Shutemov wrote: > On Fri, Nov 04, 2016 at 05:25:07AM +0100, Jan Kara wrote: > > We don't check whether vma->vm_ops is NULL in do_shared_fault() so > > there's hardly any point in checking it in wp_page_shared() or > > wp_pfn_shared() which get called only for shared file mappings as well. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > Well, I'm not sure about this. > > do_shared_fault() doesn't have the check since we checked it upper by > stack: see vma_is_anonymous() in handle_pte_fault(). > > In principal, it should be fine. But random crappy driver has potential to > blow it up. Ok, so do you prefer me to keep this patch or discard it? Either is fine with me. It was just a cleanup I wrote when factoring out the functionality. Honza > > > --- > > mm/memory.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/memory.c b/mm/memory.c > > index 7be96a43d5ac..26b2858e6a12 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -2275,7 +2275,7 @@ static int wp_pfn_shared(struct vm_fault *vmf) > > { > > struct vm_area_struct *vma = vmf->vma; > > > > - if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) { > > + if (vma->vm_ops->pfn_mkwrite) { > > int ret; > > > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > @@ -2305,7 +2305,7 @@ static int wp_page_shared(struct vm_fault *vmf, struct page *old_page) > > > > get_page(old_page); > > > > - if (vma->vm_ops && vma->vm_ops->page_mkwrite) { > > + if (vma->vm_ops->page_mkwrite) { > > int tmp; > > > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > -- > > 2.6.6 > > > > -- > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > the body to majordomo@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > -- > Kirill A. Shutemov
On Wed, Nov 16, 2016 at 02:29:18PM +0100, Jan Kara wrote: > On Wed 16-11-16 01:28:19, Kirill A. Shutemov wrote: > > On Fri, Nov 04, 2016 at 05:25:07AM +0100, Jan Kara wrote: > > > We don't check whether vma->vm_ops is NULL in do_shared_fault() so > > > there's hardly any point in checking it in wp_page_shared() or > > > wp_pfn_shared() which get called only for shared file mappings as well. > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > Well, I'm not sure about this. > > > > do_shared_fault() doesn't have the check since we checked it upper by > > stack: see vma_is_anonymous() in handle_pte_fault(). > > > > In principal, it should be fine. But random crappy driver has potential to > > blow it up. > > Ok, so do you prefer me to keep this patch or discard it? Either is fine with > me. It was just a cleanup I wrote when factoring out the functionality. I would rather drop it. Eventually, we need to make sure that all file-backed vma has vm_ops. I tried to do this once, but that back-fired...
On Wed 16-11-16 17:27:55, Kirill A. Shutemov wrote: > On Wed, Nov 16, 2016 at 02:29:18PM +0100, Jan Kara wrote: > > On Wed 16-11-16 01:28:19, Kirill A. Shutemov wrote: > > > On Fri, Nov 04, 2016 at 05:25:07AM +0100, Jan Kara wrote: > > > > We don't check whether vma->vm_ops is NULL in do_shared_fault() so > > > > there's hardly any point in checking it in wp_page_shared() or > > > > wp_pfn_shared() which get called only for shared file mappings as well. > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > > Well, I'm not sure about this. > > > > > > do_shared_fault() doesn't have the check since we checked it upper by > > > stack: see vma_is_anonymous() in handle_pte_fault(). > > > > > > In principal, it should be fine. But random crappy driver has potential to > > > blow it up. > > > > Ok, so do you prefer me to keep this patch or discard it? Either is fine with > > me. It was just a cleanup I wrote when factoring out the functionality. > > I would rather drop it. > > Eventually, we need to make sure that all file-backed vma has vm_ops. > I tried to do this once, but that back-fired... OK, will do. Honza
diff --git a/mm/memory.c b/mm/memory.c index 7be96a43d5ac..26b2858e6a12 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2275,7 +2275,7 @@ static int wp_pfn_shared(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; - if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) { + if (vma->vm_ops->pfn_mkwrite) { int ret; pte_unmap_unlock(vmf->pte, vmf->ptl); @@ -2305,7 +2305,7 @@ static int wp_page_shared(struct vm_fault *vmf, struct page *old_page) get_page(old_page); - if (vma->vm_ops && vma->vm_ops->page_mkwrite) { + if (vma->vm_ops->page_mkwrite) { int tmp; pte_unmap_unlock(vmf->pte, vmf->ptl);
We don't check whether vma->vm_ops is NULL in do_shared_fault() so there's hardly any point in checking it in wp_page_shared() or wp_pfn_shared() which get called only for shared file mappings as well. Signed-off-by: Jan Kara <jack@suse.cz> --- mm/memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)