From: Jason Gunthorpe <jgg@ziepe.ca>
To: David Hildenbrand <david@redhat.com>
Cc: Liu Zixian <liuzixian4@huawei.com>,
akpm@linux-foundation.org, linmiaohe@huawei.com,
louhongxiang@huawei.com, linux-mm@kvack.org,
hushiyuan@huawei.com, stable@vger.kernel.org
Subject: Re: [PATCH v2] fix mmap return value when vma is merged after call_mmap()
Date: Fri, 4 Dec 2020 11:10:28 -0400 [thread overview]
Message-ID: <20201204151028.GZ5487@ziepe.ca> (raw)
In-Reply-To: <d59e9e5a-1d6e-e7dc-21ec-17777fe9f7a2@redhat.com>
On Fri, Dec 04, 2020 at 03:11:54PM +0100, David Hildenbrand wrote:
> On 03.12.20 09:53, Liu Zixian wrote:
> > On success, mmap should return the begin address of newly mapped area,
> > but patch "mm: mmap: merge vma after call_mmap() if possible"
> > set vm_start of newly merged vma to return value addr.
> > Users of mmap will get wrong address if vma is merged after call_mmap().
> > We fix this by moving the assignment to addr before merging vma.
> >
> > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible")
> > Signed-off-by: Liu Zixian <liuzixian4@huawei.com>
> > v2:
> > We want to do "addr = vma->vm_start;" unconditionally,
> > so move assignment to addr before if(unlikely) block.
> > mm/mmap.c | 26 ++++++++++++--------------
> > 1 file changed, 12 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index d91ecb00d38c..5c8b4485860d 100644
> > +++ b/mm/mmap.c
> > @@ -1808,6 +1808,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > if (error)
> > goto unmap_and_free_vma;
> >
> > + /* Can addr have changed??
> > + *
> > + * Answer: Yes, several device drivers can do it in their
> > + * f_op->mmap method. -DaveM
> > + * Bug: If addr is changed, prev, rb_link, rb_parent should
> > + * be updated for vma_link()
> > + */
>
>
> Why do we tolerate device drivers doing such stuff at all?
> WARN_ON_ONCE() is just as good as BUG_ON() in many environments. If we
> support it, then no warning. If we don't support it, then why tolerate it?
The commit that introduced this seemed pretty clear it is to catch
possibly wrong drivers. I suppose the idea was to give a migration
time where things would "work" and drivers could be fixed. Since it
has now been 8 years it should be either dropped or turned into:
/* Drivers are not permitted to change vm_start */
if (WARN_ON(addr != vma->vm_start)) {
err = EINVAL
goto unmap_and_free_vma
}
commit 2897b4d29d9fca82a57b09b8a216a5d604966e4b
Author: Joonsoo Kim <js1304@gmail.com>
Date: Wed Dec 12 13:51:53 2012 -0800
mm: WARN_ON_ONCE if f_op->mmap() change vma's start address
During reviewing the source code, I found a comment which mention that
after f_op->mmap(), vma's start address can be changed. I didn't verify
that it is really possible, because there are so many f_op->mmap()
implementation. But if there are some mmap() which change vma's start
address, it is possible error situation, because we already prepare prev
vma, rb_link and rb_parent and these are related to original address.
So add WARN_ON_ONCE for finding that this situtation really happens.
Jason
next prev parent reply other threads:[~2020-12-04 15:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-03 8:53 [PATCH v2] fix mmap return value when vma is merged after call_mmap() Liu Zixian
2020-12-03 22:25 ` Andrew Morton
2020-12-04 2:29 ` Liu Zixian
2020-12-04 14:11 ` David Hildenbrand
2020-12-04 15:10 ` Jason Gunthorpe [this message]
2020-12-04 15:25 ` Matthew Wilcox
2020-12-04 16:04 ` Jason Gunthorpe
2020-12-04 19:53 ` Matthew Wilcox
2020-12-04 15:11 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201204151028.GZ5487@ziepe.ca \
--to=jgg@ziepe.ca \
--cc=akpm@linux-foundation.org \
--cc=david@redhat.com \
--cc=hushiyuan@huawei.com \
--cc=linmiaohe@huawei.com \
--cc=linux-mm@kvack.org \
--cc=liuzixian4@huawei.com \
--cc=louhongxiang@huawei.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.