LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Huacai Chen <chenhuacai@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Huacai Chen <chenhuacai@loongson.cn>,
	Ingo Molnar <mingo@redhat.com>,
	Darren Hart <dvhart@infradead.org>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	"open list:MIPS" <linux-mips@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Xuefeng Li <lixuefeng@loongson.cn>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>,
	Hongchen Zhang <zhanghongchen@loongson.cn>,
	vbabka@suse.cz, Mel Gorman <mgorman@suse.de>,
	david@redhat.com, willy@infradead.org,
	Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCH] futex: Fix fault_in_user_writeable()
Date: Tue, 17 Aug 2021 11:22:54 +0200	[thread overview]
Message-ID: <YRt/7tjFCpYn1I3d@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <CAAhV-H5TKLMi0GSQmh9RFK_k5eNRwx8AE8MjMKjJfbnyVYP-+w@mail.gmail.com>

On Tue, Aug 17, 2021 at 09:53:14AM +0800, Huacai Chen wrote:
> Hi, Davidlohr and Thomas,
> 
> On Tue, Aug 17, 2021 at 3:03 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Mon, Aug 16 2021 at 11:27, Davidlohr Bueso wrote:
> > > On Mon, 16 Aug 2021, Huacai Chen wrote:
> > >
> > >>fault_in_user_writeable() should verify R/W access but only verify W. In
> > >>most archs W implies R, but not true in MIPS and LoongArch, so fix it.
> > >
> > > Yuck for a find_vma() in futex.c. If this is a problem in MIPS, shouldn't
> > > the fix be there? Furthermore it's stated that fault_in_user_writeable():
> > >
> > > "Fault in user address and verify RW access"
> >
> > That seems to be wishful thinking given the fact that some architectures
> > do not imply R for FLAG_FAULT_WRITE.
> >
> > > And you guys seem to have proposed it already:
> > >
> > > https://lore.kernel.org/linux-mips/20200630005845.1239974-1-liulichao@loongson.cn/
> This works, but I don't think this is a MIPS problem, so does Thomas
> Bogendoerfer. Because write-only page is valid in MIPS (so Thomas
> rejected this patch).
> 
> >
> > That's surely one way to fix that. If that does not work for whatever
> > reason, then we really don't want this find_vma() hack there, but rather
> > something like:
> I don't know why find_vma() is unacceptable here, there is also
> find_vma() in fixup_user_fault().
> 
> >
> >     if (IS_ENABLED(CONFIG_ARCH_USER_FAULT_VOODOO) && get_user(&tmp, uaddr))
> >         return -EFAULT;
> get_user() may be better than find_vma(), but can we drop
> CONFIG_ARCH_USER_FAULT_VOODOO here? On those "W implies R" archs,
> get_user() always success, this can simplify the logic.

AFAICT that whole W implies R thing goes much deeper,
mm/gup.c:vma_permits_fault() has:

	vm_flags_t vm_flags = write ? VM_WRITE : VM_READ;

So unless someone wants to go fix the core MM and eradicate all such
assumptions, I'd suggest going with the 'easy' route and fix the arch.

This patch is probably broken and will likely break lots of things...

---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7ca22e6e694a..fc587dbb90b4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -478,6 +478,7 @@ enum fault_flag {
 	FAULT_FLAG_REMOTE =		1 << 7,
 	FAULT_FLAG_INSTRUCTION =	1 << 8,
 	FAULT_FLAG_INTERRUPTIBLE =	1 << 9,
+	FAULT_FLAG_READ =		1 << 10,
 };
 
 /*
diff --git a/kernel/futex.c b/kernel/futex.c
index fcc0570868b7..2c0970759919 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -735,7 +735,7 @@ static int fault_in_user_writeable(u32 __user *uaddr)
 
 	mmap_read_lock(mm);
 	ret = fixup_user_fault(mm, (unsigned long)uaddr,
-			       FAULT_FLAG_WRITE, NULL);
+			       FAULT_FLAG_READ|FAULT_FLAG_WRITE, NULL);
 	mmap_read_unlock(mm);
 
 	return ret < 0 ? ret : 0;
diff --git a/mm/gup.c b/mm/gup.c
index e805c1748bbf..37c8bfbe5196 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1222,11 +1222,17 @@ static long __get_user_pages(struct mm_struct *mm,
 static bool vma_permits_fault(struct vm_area_struct *vma,
 			      unsigned int fault_flags)
 {
+	bool read    = !!(fault_flags & FAULT_FLAG_READ);
 	bool write   = !!(fault_flags & FAULT_FLAG_WRITE);
 	bool foreign = !!(fault_flags & FAULT_FLAG_REMOTE);
-	vm_flags_t vm_flags = write ? VM_WRITE : VM_READ;
+	vm_flags_t vm_flags = 0;
 
-	if (!(vm_flags & vma->vm_flags))
+	if (read)
+		vm_flags |= VM_READ;
+	if (write)
+		vm_flags |= VM_WRITE;
+
+	if ((vma->vm_flags & vm_flags) != vm_flags)
 		return false;
 
 	/*

      parent reply	other threads:[~2021-08-17  9:23 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16  6:54 Huacai Chen
2021-08-16 18:27 ` Davidlohr Bueso
2021-08-16 19:03   ` Thomas Gleixner
2021-08-17  1:53     ` Huacai Chen
2021-08-17  7:07       ` Thomas Gleixner
2021-08-17  7:38         ` Huacai Chen
2021-08-17  9:05           ` Thomas Gleixner
2021-08-17  9:45             ` Peter Zijlstra
2021-08-17 12:27               ` Huacai Chen
2021-08-17  9:22       ` Peter Zijlstra [this message]

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=YRt/7tjFCpYn1I3d@hirez.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=chenhuacai@gmail.com \
    --cc=chenhuacai@loongson.cn \
    --cc=dave.hansen@intel.com \
    --cc=dave@stgolabs.net \
    --cc=david@redhat.com \
    --cc=dvhart@infradead.org \
    --cc=jiaxun.yang@flygoat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=lixuefeng@loongson.cn \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=tsbogend@alpha.franken.de \
    --cc=vbabka@suse.cz \
    --cc=willy@infradead.org \
    --cc=zhanghongchen@loongson.cn \
    --subject='Re: [PATCH] futex: Fix fault_in_user_writeable()' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).