* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Evgenii Stepanov @ 2019-05-24 4:23 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Linux ARM, Linux Memory Management List, LKML,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm,
open list:KERNEL SELFTEST FRAMEWORK, Vincenzo Frascino,
Will Deacon, Mark Rutland, Andrew Morton, Greg Kroah-Hartman,
Kees Cook, Yishai Hadas, Felix Kuehling, Alexander Deucher
In-Reply-To: <20190523090427.GA44383@arrakis.emea.arm.com>
On Thu, May 23, 2019 at 2:04 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, May 22, 2019 at 02:16:57PM -0700, Evgenii Stepanov wrote:
> > On Wed, May 22, 2019 at 4:49 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > > > This patch is a part of a series that extends arm64 kernel ABI to allow to
> > > > pass tagged user pointers (with the top byte set to something else other
> > > > than 0x00) as syscall arguments.
> > > >
> > > > This patch allows tagged pointers to be passed to the following memory
> > > > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> > > > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > > > remap_file_pages, shmat and shmdt.
> > > >
> > > > This is done by untagging pointers passed to these syscalls in the
> > > > prologues of their handlers.
> > >
> > > I'll go through them one by one to see if we can tighten the expected
> > > ABI while having the MTE in mind.
> > >
> > > > diff --git a/arch/arm64/kernel/sys.c b/arch/arm64/kernel/sys.c
> > > > index b44065fb1616..933bb9f3d6ec 100644
> > > > --- a/arch/arm64/kernel/sys.c
> > > > +++ b/arch/arm64/kernel/sys.c
> > > > @@ -35,10 +35,33 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> > > > {
> > > > if (offset_in_page(off) != 0)
> > > > return -EINVAL;
> > > > -
> > > > + addr = untagged_addr(addr);
> > > > return ksys_mmap_pgoff(addr, len, prot, flags, fd, off >> PAGE_SHIFT);
> > > > }
> > >
> > > If user passes a tagged pointer to mmap() and the address is honoured
> > > (or MAP_FIXED is given), what is the expected return pointer? Does it
> > > need to be tagged with the value from the hint?
> >
> > For HWASan the most convenient would be to use the tag from the hint.
> > But since in the TBI (not MTE) mode the kernel has no idea what
> > meaning userspace assigns to pointer tags, perhaps it should not try
> > to guess, and should return raw (zero-tagged) address instead.
>
> Then, just to relax the ABI for hwasan, shall we simply disallow tagged
> pointers on mmap() arguments? We can leave them in for
> mremap(old_address), madvise().
I think this would be fine. We should allow tagged in pointers in
mprotect though.
> > > With MTE, we may want to use this as a request for the default colour of
> > > the mapped pages (still under discussion).
> >
> > I like this - and in that case it would make sense to return the
> > pointer that can be immediately dereferenced without crashing the
> > process, i.e. with the matching tag.
>
> This came up from the Android investigation work where large memory
> allocations (using mmap) could be more efficiently pre-tagged by the
> kernel on page fault. Not sure about the implementation details yet.
>
> --
> Catalin
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: John Hubbard @ 2019-05-23 22:54 UTC (permalink / raw)
To: Ira Weiny
Cc: Jason Gunthorpe, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <050f56d0-1dda-036e-e508-3a7255ac7b59@nvidia.com>
On 5/23/19 3:50 PM, John Hubbard wrote:
> [...]
> I was thinking of it as a temporary measure, only up until, but not including the
> point where put_user_pages() becomes active. That is, the point when put_user_pages
> starts decrementing GUP_PIN_COUNTING_BIAS, instead of just forwarding to put_page().
>
> (For other readers, that's this patch:
>
> "mm/gup: debug tracking of get_user_pages() references"
>
> ...in https://github.com/johnhubbard/linux/tree/gup_dma_core )
>
Arggh, correction, I meant this patch:
"mm/gup: track gup-pinned pages"
...sorry for any confusion there.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: John Hubbard @ 2019-05-23 22:50 UTC (permalink / raw)
To: Ira Weiny
Cc: Jason Gunthorpe, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <20190523223701.GA15048@iweiny-DESK2.sc.intel.com>
On 5/23/19 3:37 PM, Ira Weiny wrote:
[...]
> I've dug in further and I see now that release_pages() implements (almost the
> same thing, see below) as put_page().
>
> However, I think we need to be careful here because put_page_testzero() calls
>
> page_ref_dec_and_test(page);
>
> ... and after your changes it will need to call ...
>
> page_ref_sub_return(page, GUP_PIN_COUNTING_BIAS);
>
> ... on a GUP page:
>
> So how do you propose calling release_pages() from within put_user_pages()? Or
> were you thinking this would be temporary?
I was thinking of it as a temporary measure, only up until, but not including the
point where put_user_pages() becomes active. That is, the point when put_user_pages
starts decrementing GUP_PIN_COUNTING_BIAS, instead of just forwarding to put_page().
(For other readers, that's this patch:
"mm/gup: debug tracking of get_user_pages() references"
...in https://github.com/johnhubbard/linux/tree/gup_dma_core )
>
> That said, there are 2 differences I see between release_pages() and put_page()
>
> 1) release_pages() will only work for a MEMORY_DEVICE_PUBLIC page and not all
> devmem pages...
> I think this is a bug, patch to follow shortly.
>
> 2) release_pages() calls __ClearPageActive() while put_page() does not
>
> I have no idea if the second difference is a bug or not. But it smells of
> one...
>
> It would be nice to know if the open coding of put_page is really a performance
> benefit or not. It seems like an attempt to optimize the taking of the page
> data lock.
>
> Does anyone have any information about the performance advantage here?
>
> Given the changes above it seems like it would be a benefit to merge the 2 call
> paths more closely to make sure we do the right thing.
>
Yes, it does. Maybe best to not do the temporary measure, then, while this stuff
gets improved. I'll look at your other patch...
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: Ira Weiny @ 2019-05-23 22:37 UTC (permalink / raw)
To: John Hubbard
Cc: Jason Gunthorpe, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <0bd9859f-8eb0-9148-6209-08ae42665626@nvidia.com>
On Thu, May 23, 2019 at 12:13:59PM -0700, John Hubbard wrote:
> On 5/23/19 12:04 PM, Ira Weiny wrote:
> > On Thu, May 23, 2019 at 10:46:38AM -0700, John Hubbard wrote:
> > > On 5/23/19 10:32 AM, Jason Gunthorpe wrote:
> > > > On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
> > > > > > @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> > > > > > * ib_umem_odp_map_dma_single_page().
> > > > > > */
> > > > > > if (npages - (j + 1) > 0)
> > > > > > - release_pages(&local_page_list[j+1],
> > > > > > - npages - (j + 1));
> > > > > > + put_user_pages(&local_page_list[j+1],
> > > > > > + npages - (j + 1));
> > > > >
> > > > > I don't know if we discussed this before but it looks like the use of
> > > > > release_pages() was not entirely correct (or at least not necessary) here. So
> > > > > I think this is ok.
> > > >
> > > > Oh? John switched it from a put_pages loop to release_pages() here:
> > > >
> > > > commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
> > > > Author: John Hubbard <jhubbard@nvidia.com>
> > > > Date: Mon Mar 4 11:46:45 2019 -0800
> > > >
> > > > RDMA/umem: minor bug fix in error handling path
> > > > 1. Bug fix: fix an off by one error in the code that cleans up if it fails
> > > > to dma-map a page, after having done a get_user_pages_remote() on a
> > > > range of pages.
> > > > 2. Refinement: for that same cleanup code, release_pages() is better than
> > > > put_page() in a loop.
> > > >
> > > > And now we are going to back something called put_pages() that
> > > > implements the same for loop the above removed?
> > > >
> > > > Seems like we are going in circles?? John?
> > > >
> > >
> > > put_user_pages() is meant to be a drop-in replacement for release_pages(),
> > > so I made the above change as an interim step in moving the callsite from
> > > a loop, to a single call.
> > >
> > > And at some point, it may be possible to find a way to optimize put_user_pages()
> > > in a similar way to the batching that release_pages() does, that was part
> > > of the plan for this.
> > >
> > > But I do see what you mean: in the interim, maybe put_user_pages() should
> > > just be calling release_pages(), how does that change sound?
> >
> > I'm certainly not the expert here but FWICT release_pages() was originally
> > designed to work with the page cache.
> >
> > aabfb57296e3 mm: memcontrol: do not kill uncharge batching in free_pages_and_swap_cache
> >
> > But at some point it was changed to be more general?
> >
> > ea1754a08476 mm, fs: remove remaining PAGE_CACHE_* and page_cache_{get,release} usage
> >
> > ... and it is exported and used outside of the swapping code... and used at
> > lease 1 place to directly "put" pages gotten from get_user_pages_fast()
> > [arch/x86/kvm/svm.c]
> >
> > From that it seems like it is safe.
> >
> > But I don't see where release_page() actually calls put_page() anywhere? What
> > am I missing?
> >
>
> For that question, I recall having to look closely at this function, as well:
>
> void release_pages(struct page **pages, int nr)
> {
> int i;
> LIST_HEAD(pages_to_free);
> struct pglist_data *locked_pgdat = NULL;
> struct lruvec *lruvec;
> unsigned long uninitialized_var(flags);
> unsigned int uninitialized_var(lock_batch);
>
> for (i = 0; i < nr; i++) {
> struct page *page = pages[i];
>
> /*
> * Make sure the IRQ-safe lock-holding time does not get
> * excessive with a continuous string of pages from the
> * same pgdat. The lock is held only if pgdat != NULL.
> */
> if (locked_pgdat && ++lock_batch == SWAP_CLUSTER_MAX) {
> spin_unlock_irqrestore(&locked_pgdat->lru_lock, flags);
> locked_pgdat = NULL;
> }
>
> if (is_huge_zero_page(page))
> continue;
>
> /* Device public page can not be huge page */
> if (is_device_public_page(page)) {
> if (locked_pgdat) {
> spin_unlock_irqrestore(&locked_pgdat->lru_lock,
> flags);
> locked_pgdat = NULL;
> }
> put_devmap_managed_page(page);
> continue;
> }
>
> page = compound_head(page);
> if (!put_page_testzero(page))
>
> ^here is where it does the put_page() call, is that what
> you were looking for?
Yes I saw that...
I've dug in further and I see now that release_pages() implements (almost the
same thing, see below) as put_page().
However, I think we need to be careful here because put_page_testzero() calls
page_ref_dec_and_test(page);
... and after your changes it will need to call ...
page_ref_sub_return(page, GUP_PIN_COUNTING_BIAS);
... on a GUP page:
So how do you propose calling release_pages() from within put_user_pages()? Or
were you thinking this would be temporary?
That said, there are 2 differences I see between release_pages() and put_page()
1) release_pages() will only work for a MEMORY_DEVICE_PUBLIC page and not all
devmem pages...
I think this is a bug, patch to follow shortly.
2) release_pages() calls __ClearPageActive() while put_page() does not
I have no idea if the second difference is a bug or not. But it smells of
one...
It would be nice to know if the open coding of put_page is really a performance
benefit or not. It seems like an attempt to optimize the taking of the page
data lock.
Does anyone have any information about the performance advantage here?
Given the changes above it seems like it would be a benefit to merge the 2 call
paths more closely to make sure we do the right thing.
Ira
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Khalid Aziz @ 2019-05-23 21:49 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm, Szabolcs Nagy, Will Deacon, dri-devel,
Linux Memory Management List, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgenii Stepanov,
linux-media, Kevin Brodsky, Kees Cook, Ruben Ayrapetyan,
Andrey Konovalov, Ramana Radhakrishnan, Alex Williamson, Yishai
In-Reply-To: <20190523201105.oifkksus4rzcwqt4@mbp>
On 5/23/19 2:11 PM, Catalin Marinas wrote:
> Hi Khalid,
>
> On Thu, May 23, 2019 at 11:51:40AM -0600, Khalid Aziz wrote:
>> On 5/21/19 6:04 PM, Kees Cook wrote:
>>> As an aside: I think Sparc ADI support in Linux actually side-stepped
>>> this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must
>>> be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI
>>> for kernel code.") I think this was a mistake we should not repeat for
>>> arm64 (we do seem to be at least in agreement about this, I think).
>>>
>>> [1] https://lore.kernel.org/patchwork/patch/654481/
>>
>> That is a very early version of the sparc ADI patch. Support for tagged
>> addresses in syscalls was added in later versions and is in the patch
>> that is in the kernel.
>
> I tried to figure out but I'm not familiar with the sparc port. How did
> you solve the tagged address going into various syscall implementations
> in the kernel (e.g. sys_write)? Is the tag removed on kernel entry or it
> ends up deeper in the core code?
>
Another spot I should point out in ADI patch - Tags are not stored in
VMAs and IOMMU does not support ADI tags on M7. ADI tags are stripped
before userspace addresses are passed to IOMMU in the following snippet
from the patch:
diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
index 5335ba3c850e..357b6047653a 100644
--- a/arch/sparc/mm/gup.c
+++ b/arch/sparc/mm/gup.c
@@ -201,6 +202,24 @@ int __get_user_pages_fast(unsigned long start, int
nr_pages
, int write,
pgd_t *pgdp;
int nr = 0;
+#ifdef CONFIG_SPARC64
+ if (adi_capable()) {
+ long addr = start;
+
+ /* If userspace has passed a versioned address, kernel
+ * will not find it in the VMAs since it does not store
+ * the version tags in the list of VMAs. Storing version
+ * tags in list of VMAs is impractical since they can be
+ * changed any time from userspace without dropping into
+ * kernel. Any address search in VMAs will be done with
+ * non-versioned addresses. Ensure the ADI version bits
+ * are dropped here by sign extending the last bit before
+ * ADI bits. IOMMU does not implement version tags.
+ */
+ addr = (addr << (long)adi_nbits()) >> (long)adi_nbits();
+ start = addr;
+ }
+#endif
start &= PAGE_MASK;
addr = start;
len = (unsigned long) nr_pages << PAGE_SHIFT;
--
Khalid
^ permalink raw reply related
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Khalid Aziz @ 2019-05-23 21:42 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm, Szabolcs Nagy, Will Deacon, dri-devel,
Linux Memory Management List, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgenii Stepanov,
linux-media, Kevin Brodsky, Kees Cook, Ruben Ayrapetyan,
Andrey Konovalov, Ramana Radhakrishnan, Alex Williamson, Yishai
In-Reply-To: <20190523201105.oifkksus4rzcwqt4@mbp>
On 5/23/19 2:11 PM, Catalin Marinas wrote:
> Hi Khalid,
>
> On Thu, May 23, 2019 at 11:51:40AM -0600, Khalid Aziz wrote:
>> On 5/21/19 6:04 PM, Kees Cook wrote:
>>> As an aside: I think Sparc ADI support in Linux actually side-stepped
>>> this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must
>>> be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI
>>> for kernel code.") I think this was a mistake we should not repeat for
>>> arm64 (we do seem to be at least in agreement about this, I think).
>>>
>>> [1] https://lore.kernel.org/patchwork/patch/654481/
>>
>> That is a very early version of the sparc ADI patch. Support for tagged
>> addresses in syscalls was added in later versions and is in the patch
>> that is in the kernel.
>
> I tried to figure out but I'm not familiar with the sparc port. How did
> you solve the tagged address going into various syscall implementations
> in the kernel (e.g. sys_write)? Is the tag removed on kernel entry or it
> ends up deeper in the core code?
Tag is not removed from the user addresses. Kernel passes tagged
addresses to copy_from_user and copy_to_user. MMU checks the tag
embedded in the address when kernel accesses userspace addresses. This
maintains the ADI integrity even when userspace attempts to access any
userspace addresses through system calls.
On sparc, access_ok() is defined as:
#define access_ok(addr, size) __access_ok((unsigned long)(addr), size)
#define __access_ok(addr, size) (__user_ok((addr) & get_fs().seg, (size)))
#define __user_ok(addr, size) ({ (void)(size); (addr) < STACK_TOP; })
STACK_TOP for M7 processor (which is the first sparc processor to
support ADI) is 0xfff8000000000000UL. Tagged addresses pass the
access_ok() check fine. Any tag mismatches that happen during kernel
access to userspace addresses are handled by do_mcd_err().
--
Khalid
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Kees Cook @ 2019-05-23 21:31 UTC (permalink / raw)
To: Catalin Marinas
Cc: enh, Evgenii Stepanov, Andrey Konovalov, Khalid Aziz, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <20190523174345.6sv3kcipkvlwfmox@mbp>
On Thu, May 23, 2019 at 06:43:46PM +0100, Catalin Marinas wrote:
> On Thu, May 23, 2019 at 09:38:19AM -0700, Kees Cook wrote:
> > What on this front would you be comfortable with? Given it's a new
> > feature isn't it sufficient to have a CONFIG (and/or boot option)?
>
> I'd rather avoid re-building kernels. A boot option would do, unless we
> see value in a per-process (inherited) personality or prctl. The
I think I've convinced myself that the normal<->TBI ABI control should
be a boot parameter. More below...
> > What about testing tools that intentionally insert high bits for syscalls
> > and are _expecting_ them to fail? It seems the TBI series will break them?
> > In that case, do we need to opt into TBI as well?
>
> If there are such tools, then we may need a per-process control. It's
> basically an ABI change.
syzkaller already attempts to randomly inject non-canonical and
0xFFFF....FFFF addresses for user pointers in syscalls in an effort to
find bugs like CVE-2017-5123 where waitid() via unchecked put_user() was
able to write directly to kernel memory[1].
It seems that using TBI by default and not allowing a switch back to
"normal" ABI without a reboot actually means that userspace cannot inject
kernel pointers into syscalls any more, since they'll get universally
stripped now. Is my understanding correct, here? i.e. exploiting
CVE-2017-5123 would be impossible under TBI?
If so, then I think we should commit to the TBI ABI and have a boot
flag to disable it, but NOT have a process flag, as that would allow
attackers to bypass the masking. The only flag should be "TBI or MTE".
If so, can I get top byte masking for other architectures too? Like,
just to strip high bits off userspace addresses? ;)
(Oh, in looking I see this is implemented with sign-extension... why
not just a mask? So it'll either be valid userspace address or forced
into the non-canonical range?)
[1] https://salls.github.io/Linux-Kernel-CVE-2017-5123/
> > Alright, the tl;dr appears to be:
> > - you want more assurances that we can find __user stripping in the
> > kernel more easily. (But this seems like a parallel problem.)
>
> Yes, and that we found all (most) cases now. The reason I don't see it
> as a parallel problem is that, as maintainer, I promise an ABI to user
> and I'd rather stick to it. I don't want, for example, ncurses to stop
> working because of some ioctl() rejecting tagged pointers.
But this is what I don't understand: it would need to be ncurses _using
TBI_, that would stop working (having started to work before, but then
regress due to a newly added one-off bug). Regular ncurses will be fine
because it's not using TBI. So The Golden Rule isn't violated, and by
definition, it's a specific regression caused by some bug (since TBI
would have had to have worked _before_ in the situation to be considered
a regression now). Which describes the normal path for kernel
development... add feature, find corner cases where it doesn't work,
fix them, encounter new regressions, fix those, repeat forever.
> If it's just the occasional one-off bug I'm fine to deal with it. But
> no-one convinced me yet that this is the case.
You believe there still to be some systemic cases that haven't been
found yet? And even if so -- isn't it better to work on that
incrementally?
> As for the generic driver code (filesystems or other subsystems),
> without some clear direction for developers, together with static
> checking/sparse, on how user pointers are cast to longs (one example),
> it would become my responsibility to identify and fix them up with any
> kernel release. This series is not providing such guidance, just adding
> untagged_addr() in some places that we think matter.
What about adding a nice bit of .rst documentation that describes the
situation and shows how to use untagged_addr(). This is the kind of
kernel-wide change that "everyone" needs to know about, and shouldn't
be the arch maintainer's sole responsibility to fix.
> > - we might need to opt in to TBI with a prctl()
>
> Yes, although still up for discussion.
I think I've talked myself out of it. I say boot param only! :)
So what do you say to these next steps:
- change untagged_addr() to use a static branch that is controlled with
a boot parameter.
- add, say, Documentation/core-api/user-addresses.rst to describe
proper care and handling of user space pointers with untagged_addr(),
with examples based on all the cases seen so far in this series.
- continue work to improve static analysis.
Thanks for wading through this with me! :)
--
Kees Cook
^ permalink raw reply
* Re: [RFC][PATCH] kernel.h: Add generic roundup_64() macro
From: Linus Torvalds @ 2019-05-23 21:19 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Ben Skeggs, David Airlie, Daniel Vetter, Leon Romanovsky,
Doug Ledford, Jason Gunthorpe, Darrick J. Wong, linux-xfs,
dri-devel, nouveau, linux-rdma, Andrew Morton
In-Reply-To: <20190523133648.591f9e78@gandalf.local.home>
On Thu, May 23, 2019 at 10:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> >
> > Of course, you probably want the usual "at least use 'int'" semantics,
> > in which case the "type" should be "(x)+0":
> >
> > #define round_up(x, y) size_fn((x)+0, round_up_size, x, y)
> >
> > and the 8-bit and 16-bit cases will never be used.
>
> I'm curious to what the advantage of that is?
Let's say that you have a structure with a 'unsigned char' member,
because the value range is 0-255.
What happens if you do
x = round_up(p->member, 4);
and the value is 255?
Now, if you stay in 'unsigned char' the end result is 0. If you follow
the usual C integer promotion rules ("all arithmetic promotes to at
least 'int'"), you get 256.
Most people probably expect 256, and that implies that even if you
pass an 'unsigned char' to an arithmetic function like this, you
expect any math to be done in 'int'. Doing the "(x)+0" forces that,
because the "+0" changes the type of the expression from "unsigned
char" to "int" due to C integer promotion.
Yes. The C integer type rules are subtle and sometimes surprising. One
of the things I've wanted is to have some way to limit silent
promotion (and silent truncation!), and cause warnings. 'sparse' does
some of that with some special-case types (ie __bitwise), but it's
pretty limited.
Linus
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Catalin Marinas @ 2019-05-23 20:11 UTC (permalink / raw)
To: Khalid Aziz
Cc: Kees Cook, Evgenii Stepanov, Andrey Konovalov, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling
In-Reply-To: <6049844a-65f5-f513-5b58-7141588fef2b@oracle.com>
Hi Khalid,
On Thu, May 23, 2019 at 11:51:40AM -0600, Khalid Aziz wrote:
> On 5/21/19 6:04 PM, Kees Cook wrote:
> > As an aside: I think Sparc ADI support in Linux actually side-stepped
> > this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must
> > be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI
> > for kernel code.") I think this was a mistake we should not repeat for
> > arm64 (we do seem to be at least in agreement about this, I think).
> >
> > [1] https://lore.kernel.org/patchwork/patch/654481/
>
> That is a very early version of the sparc ADI patch. Support for tagged
> addresses in syscalls was added in later versions and is in the patch
> that is in the kernel.
I tried to figure out but I'm not familiar with the sparc port. How did
you solve the tagged address going into various syscall implementations
in the kernel (e.g. sys_write)? Is the tag removed on kernel entry or it
ends up deeper in the core code?
Thanks.
--
Catalin
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-23 19:47 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523193959.GA5658@redhat.com>
On Thu, May 23, 2019 at 03:39:59PM -0400, Jerome Glisse wrote:
> On Thu, May 23, 2019 at 04:10:38PM -0300, Jason Gunthorpe wrote:
> >
> > On Thu, May 23, 2019 at 02:24:58PM -0400, Jerome Glisse wrote:
> > > I can not take mmap_sem in range_register, the READ_ONCE is fine and
> > > they are no race as we do take a reference on the hmm struct thus
> >
> > Of course there are use after free races with a READ_ONCE scheme, I
> > shouldn't have to explain this.
>
> Well i can not think of anything again here the mm->hmm can not
> change while driver is calling hmm_range_register()
Oh of course! It is so confusing because the new code makes it look like
it could be changing...
> so if you want i can remove the READ_ONCE() this does not change
> anything.
Please just write it as:
/* The caller must hold a valid struct hmm_mirror to call this api,
* and a valid hmm_mirror guarantees mm->hmm is valid.
*/
range->hmm = mm->hmm;
kref_get(&range->hmm->kref);
All the READ_ONCE/kref_get_not_zero/!hmm just obfuscates the reality
that hmm is non-NULL and constant here or the caller is using the API
wrong.
kref_get will reliably oops or WARN_ON if it is misused which is a
fine amount of debugging.
Jason
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-23 19:39 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523191038.GG12159@ziepe.ca>
On Thu, May 23, 2019 at 04:10:38PM -0300, Jason Gunthorpe wrote:
>
> On Thu, May 23, 2019 at 02:24:58PM -0400, Jerome Glisse wrote:
> > I can not take mmap_sem in range_register, the READ_ONCE is fine and
> > they are no race as we do take a reference on the hmm struct thus
>
> Of course there are use after free races with a READ_ONCE scheme, I
> shouldn't have to explain this.
Well i can not think of anything again here the mm->hmm can not
change while driver is calling hmm_range_register() so if you
want i can remove the READ_ONCE() this does not change anything.
> If you cannot take the read mmap sem (why not?), then please use my
> version and push the update to the driver through -mm..
Please see previous threads on why it was a failure.
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: Jason Gunthorpe @ 2019-05-23 19:17 UTC (permalink / raw)
To: John Hubbard
Cc: Ira Weiny, john.hubbard@gmail.com, Andrew Morton,
linux-mm@kvack.org, LKML, linux-rdma@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Doug Ledford, Mike Marciniszyn,
Dennis Dalessandro, Christian Benvenuti, Jan Kara
In-Reply-To: <fa6d7d7c-13a3-0586-6384-768ebb7f0561@nvidia.com>
On Thu, May 23, 2019 at 10:46:38AM -0700, John Hubbard wrote:
> On 5/23/19 10:32 AM, Jason Gunthorpe wrote:
> > On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
> > > > @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> > > > * ib_umem_odp_map_dma_single_page().
> > > > */
> > > > if (npages - (j + 1) > 0)
> > > > - release_pages(&local_page_list[j+1],
> > > > - npages - (j + 1));
> > > > + put_user_pages(&local_page_list[j+1],
> > > > + npages - (j + 1));
> > >
> > > I don't know if we discussed this before but it looks like the use of
> > > release_pages() was not entirely correct (or at least not necessary) here. So
> > > I think this is ok.
> >
> > Oh? John switched it from a put_pages loop to release_pages() here:
> >
> > commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
> > Author: John Hubbard <jhubbard@nvidia.com>
> > Date: Mon Mar 4 11:46:45 2019 -0800
> >
> > RDMA/umem: minor bug fix in error handling path
> > 1. Bug fix: fix an off by one error in the code that cleans up if it fails
> > to dma-map a page, after having done a get_user_pages_remote() on a
> > range of pages.
> > 2. Refinement: for that same cleanup code, release_pages() is better than
> > put_page() in a loop.
> >
> > And now we are going to back something called put_pages() that
> > implements the same for loop the above removed?
> >
> > Seems like we are going in circles?? John?
> >
>
> put_user_pages() is meant to be a drop-in replacement for release_pages(),
> so I made the above change as an interim step in moving the callsite from
> a loop, to a single call.
>
> And at some point, it may be possible to find a way to optimize put_user_pages()
> in a similar way to the batching that release_pages() does, that was part
> of the plan for this.
>
> But I do see what you mean: in the interim, maybe put_user_pages() should
> just be calling release_pages(), how does that change sound?
It would have made it more consistent.. But it seems this isn't a
functional problem in this patch
Jason
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: John Hubbard @ 2019-05-23 19:13 UTC (permalink / raw)
To: Ira Weiny
Cc: Jason Gunthorpe, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <20190523190423.GA19578@iweiny-DESK2.sc.intel.com>
On 5/23/19 12:04 PM, Ira Weiny wrote:
> On Thu, May 23, 2019 at 10:46:38AM -0700, John Hubbard wrote:
>> On 5/23/19 10:32 AM, Jason Gunthorpe wrote:
>>> On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
>>>>> @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
>>>>> * ib_umem_odp_map_dma_single_page().
>>>>> */
>>>>> if (npages - (j + 1) > 0)
>>>>> - release_pages(&local_page_list[j+1],
>>>>> - npages - (j + 1));
>>>>> + put_user_pages(&local_page_list[j+1],
>>>>> + npages - (j + 1));
>>>>
>>>> I don't know if we discussed this before but it looks like the use of
>>>> release_pages() was not entirely correct (or at least not necessary) here. So
>>>> I think this is ok.
>>>
>>> Oh? John switched it from a put_pages loop to release_pages() here:
>>>
>>> commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
>>> Author: John Hubbard <jhubbard@nvidia.com>
>>> Date: Mon Mar 4 11:46:45 2019 -0800
>>>
>>> RDMA/umem: minor bug fix in error handling path
>>> 1. Bug fix: fix an off by one error in the code that cleans up if it fails
>>> to dma-map a page, after having done a get_user_pages_remote() on a
>>> range of pages.
>>> 2. Refinement: for that same cleanup code, release_pages() is better than
>>> put_page() in a loop.
>>>
>>> And now we are going to back something called put_pages() that
>>> implements the same for loop the above removed?
>>>
>>> Seems like we are going in circles?? John?
>>>
>>
>> put_user_pages() is meant to be a drop-in replacement for release_pages(),
>> so I made the above change as an interim step in moving the callsite from
>> a loop, to a single call.
>>
>> And at some point, it may be possible to find a way to optimize put_user_pages()
>> in a similar way to the batching that release_pages() does, that was part
>> of the plan for this.
>>
>> But I do see what you mean: in the interim, maybe put_user_pages() should
>> just be calling release_pages(), how does that change sound?
>
> I'm certainly not the expert here but FWICT release_pages() was originally
> designed to work with the page cache.
>
> aabfb57296e3 mm: memcontrol: do not kill uncharge batching in free_pages_and_swap_cache
>
> But at some point it was changed to be more general?
>
> ea1754a08476 mm, fs: remove remaining PAGE_CACHE_* and page_cache_{get,release} usage
>
> ... and it is exported and used outside of the swapping code... and used at
> lease 1 place to directly "put" pages gotten from get_user_pages_fast()
> [arch/x86/kvm/svm.c]
>
> From that it seems like it is safe.
>
> But I don't see where release_page() actually calls put_page() anywhere? What
> am I missing?
>
For that question, I recall having to look closely at this function, as well:
void release_pages(struct page **pages, int nr)
{
int i;
LIST_HEAD(pages_to_free);
struct pglist_data *locked_pgdat = NULL;
struct lruvec *lruvec;
unsigned long uninitialized_var(flags);
unsigned int uninitialized_var(lock_batch);
for (i = 0; i < nr; i++) {
struct page *page = pages[i];
/*
* Make sure the IRQ-safe lock-holding time does not get
* excessive with a continuous string of pages from the
* same pgdat. The lock is held only if pgdat != NULL.
*/
if (locked_pgdat && ++lock_batch == SWAP_CLUSTER_MAX) {
spin_unlock_irqrestore(&locked_pgdat->lru_lock, flags);
locked_pgdat = NULL;
}
if (is_huge_zero_page(page))
continue;
/* Device public page can not be huge page */
if (is_device_public_page(page)) {
if (locked_pgdat) {
spin_unlock_irqrestore(&locked_pgdat->lru_lock,
flags);
locked_pgdat = NULL;
}
put_devmap_managed_page(page);
continue;
}
page = compound_head(page);
if (!put_page_testzero(page))
^here is where it does the put_page() call, is that what
you were looking for?
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-23 19:10 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523182458.GA3571@redhat.com>
On Thu, May 23, 2019 at 02:24:58PM -0400, Jerome Glisse wrote:
> I can not take mmap_sem in range_register, the READ_ONCE is fine and
> they are no race as we do take a reference on the hmm struct thus
Of course there are use after free races with a READ_ONCE scheme, I
shouldn't have to explain this.
If you cannot take the read mmap sem (why not?), then please use my
version and push the update to the driver through -mm..
Jason
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: Ira Weiny @ 2019-05-23 19:04 UTC (permalink / raw)
To: John Hubbard
Cc: Jason Gunthorpe, john.hubbard@gmail.com, Andrew Morton,
linux-mm@kvack.org, LKML, linux-rdma@vger.kernel.org,
linux-fsdevel@vger.kernel.org, Doug Ledford, Mike Marciniszyn,
Dennis Dalessandro, Christian Benvenuti, Jan Kara
In-Reply-To: <fa6d7d7c-13a3-0586-6384-768ebb7f0561@nvidia.com>
On Thu, May 23, 2019 at 10:46:38AM -0700, John Hubbard wrote:
> On 5/23/19 10:32 AM, Jason Gunthorpe wrote:
> > On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
> > > > @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> > > > * ib_umem_odp_map_dma_single_page().
> > > > */
> > > > if (npages - (j + 1) > 0)
> > > > - release_pages(&local_page_list[j+1],
> > > > - npages - (j + 1));
> > > > + put_user_pages(&local_page_list[j+1],
> > > > + npages - (j + 1));
> > >
> > > I don't know if we discussed this before but it looks like the use of
> > > release_pages() was not entirely correct (or at least not necessary) here. So
> > > I think this is ok.
> >
> > Oh? John switched it from a put_pages loop to release_pages() here:
> >
> > commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
> > Author: John Hubbard <jhubbard@nvidia.com>
> > Date: Mon Mar 4 11:46:45 2019 -0800
> >
> > RDMA/umem: minor bug fix in error handling path
> > 1. Bug fix: fix an off by one error in the code that cleans up if it fails
> > to dma-map a page, after having done a get_user_pages_remote() on a
> > range of pages.
> > 2. Refinement: for that same cleanup code, release_pages() is better than
> > put_page() in a loop.
> >
> > And now we are going to back something called put_pages() that
> > implements the same for loop the above removed?
> >
> > Seems like we are going in circles?? John?
> >
>
> put_user_pages() is meant to be a drop-in replacement for release_pages(),
> so I made the above change as an interim step in moving the callsite from
> a loop, to a single call.
>
> And at some point, it may be possible to find a way to optimize put_user_pages()
> in a similar way to the batching that release_pages() does, that was part
> of the plan for this.
>
> But I do see what you mean: in the interim, maybe put_user_pages() should
> just be calling release_pages(), how does that change sound?
I'm certainly not the expert here but FWICT release_pages() was originally
designed to work with the page cache.
aabfb57296e3 mm: memcontrol: do not kill uncharge batching in free_pages_and_swap_cache
But at some point it was changed to be more general?
ea1754a08476 mm, fs: remove remaining PAGE_CACHE_* and page_cache_{get,release} usage
... and it is exported and used outside of the swapping code... and used at
lease 1 place to directly "put" pages gotten from get_user_pages_fast()
[arch/x86/kvm/svm.c]
>From that it seems like it is safe.
But I don't see where release_page() actually calls put_page() anywhere? What
am I missing?
Ira
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-23 18:24 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523175546.GE12159@ziepe.ca>
On Thu, May 23, 2019 at 02:55:46PM -0300, Jason Gunthorpe wrote:
> On Thu, May 23, 2019 at 01:33:03PM -0400, Jerome Glisse wrote:
> > On Thu, May 23, 2019 at 01:34:29PM -0300, Jason Gunthorpe wrote:
> > > On Thu, May 23, 2019 at 11:52:08AM -0400, Jerome Glisse wrote:
> > > > On Thu, May 23, 2019 at 12:41:49PM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, May 23, 2019 at 11:04:32AM -0400, Jerome Glisse wrote:
> > > > > > On Wed, May 22, 2019 at 08:57:37PM -0300, Jason Gunthorpe wrote:
> > > > > > > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> > > > > > >
> > > > > > > > > > So attached is a rebase on top of 5.2-rc1, i have tested with pingpong
> > > > > > > > > > (prefetch and not and different sizes). Seems to work ok.
> > > > > > > > >
> > > > > > > > > Urk, it already doesn't apply to the rdma tree :(
> > > > > > > > >
> > > > > > > > > The conflicts are a little more extensive than I'd prefer to handle..
> > > > > > > > > Can I ask you to rebase it on top of this branch please:
> > > > > > > > >
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/jgg-for-next
> > > > > > > > >
> > > > > > > > > Specifically it conflicts with this patch:
> > > > > > > > >
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-next&id=d2183c6f1958e6b6dfdde279f4cee04280710e34
> > > > > > >
> > > > > > > There is at least one more serious blocker here:
> > > > > > >
> > > > > > > config ARCH_HAS_HMM_MIRROR
> > > > > > > bool
> > > > > > > default y
> > > > > > > depends on (X86_64 || PPC64)
> > > > > > > depends on MMU && 64BIT
> > > > > > >
> > > > > > > I can't loose ARM64 support for ODP by merging this, that is too
> > > > > > > serious of a regression.
> > > > > > >
> > > > > > > Can you fix it?
> > > > > >
> > > > > > 5.2 already has patch to fix the Kconfig (ARCH_HAS_HMM_MIRROR and
> > > > > > ARCH_HAS_HMM_DEVICE replacing ARCH_HAS_HMM) I need to update nouveau
> > > > >
> > > > > Newer than 5.2-rc1? Is this why ARCH_HAS_HMM_MIRROR is not used anywhere?
> > > >
> > > > Yes this is multi-step update, first add the new Kconfig release n,
> > > > update driver in release n+1, update core Kconfig in release n+2
> > > >
> > > > So we are in release n (5.2), in 5.3 i will update nouveau and amdgpu
> > > > so that in 5.4 in ca remove the old ARCH_HAS_HMM
> > >
> > > Why don't you just send the patch for both parts to mm or to DRM?
> > >
> > > This is very normal - as long as the resulting conflicts would be
> > > small during there is no reason not to do this. Can you share the
> > > combined patch?
> >
> > This was tested in the past an resulted in failure. So for now i am
> > taking the simplest and easiest path with the least burden for every
> > maintainer. It only complexify my life.
>
> I don't know what you tried to do in the past, but it happens all the
> time, every merge cycle with success. Not everything can be done, but
> changing the signature of one function with one call site should
> really not be a problem.
>
> > Note that mm is not a git tree and thus i can not play any git trick
> > to help in this endeavor.
>
> I am aware..
>
> > > > > If mm takes the fixup patches so hmm mirror is as reliable as ODP's
> > > > > existing stuff, and patch from you to enable ARM64, then we can
> > > > > continue to merge into 5.3
> > > > >
> > > > > So, let us try to get acks on those other threads..
> > > >
> > > > I will be merging your patchset and Ralph and repost, they are only
> > > > minor change mostly that you can not update the driver API in just
> > > > one release.
> > >
> > > Of course you can, we do it all the time. It requires some
> > > co-ordination, but as long as the merge conflicts are not big it is
> > > fine.
> > >
> > > Merge the driver API change and the call site updates to -mm and
> > > refain from merging horrendously conflicting patches through DRM.
> > >
> > > In the case of the changes in my HMM RFC it is something like 2
> > > lines in DRM that need touching, no problem at all.
> > >
> > > If you want help I can volunteer make a hmm PR for Linus just for this
> > > during the merge window - but Andrew would need to agree and ack the
> > > patches.
> >
> > This was tested in the past and i do not want to go over this issue
> > again (or re-iterate the long emails discussion associated with that).
> > It failed and it put the burden on every maintainers. So it is easier
> > to do the multi-step thing.
> >
> > You can take a peak at Ralph patchset and yours into one with minor
> > changes here:
> >
> > https://cgit.freedesktop.org/~glisse/linux/log/?h=hmm-5.3
>
> Okay..
>
> This patch needs to use down_read(&mm->mmap_sem) not READ_ONCE:
>
> mm/hmm: do not try to create hmm struct from within hmm_range_register()
> Driver should never call hmm_range_register() without a valid and active
> registered hmm_mirror and thus without a valid and active hmm struct. So
> if that happens just return -EFAULT.
>
> Otherwise it is inconsisent with the locking scheme and has a use
> after free race.
I can not take mmap_sem in range_register, the READ_ONCE is fine and
they are no race as we do take a reference on the hmm struct thus
if we get are successful at taking that reference then hmm struct
will remain valid. Also this API has always been use with active
mirror and thus active hmm and thus mm->hmm will remain stable for
all call to hmm_range_register(), it would be a bug otherwise.
The READ_ONCE() is really just a safety net, it is not required and
once the convertion to hmm_mirror is done it will go away.
>
> I was not sure if the lock could be obtained safely here so I
> preferred to use the no-lock alternative of passing in mirror. I still
> think you should just change the single call site and sent to -mm.
It is too painful with unmerge driver like amdgpu that are targeting
5.3, those do target API that is in 5.2
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: John Hubbard @ 2019-05-23 17:56 UTC (permalink / raw)
To: Jerome Glisse
Cc: Andrew Morton, linux-mm, Jason Gunthorpe, LKML, linux-rdma,
linux-fsdevel, Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara, Jason Gunthorpe, Ira Weiny
In-Reply-To: <20190523153133.GB5104@redhat.com>
On 5/23/19 8:31 AM, Jerome Glisse wrote:
[...]
>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
>
Thanks for the review!
> Between i have a wishlist see below
[...]
>> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
>> index e7ea819fcb11..673f0d240b3e 100644
>> --- a/drivers/infiniband/core/umem.c
>> +++ b/drivers/infiniband/core/umem.c
>> @@ -54,9 +54,10 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
>>
>> for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
>> page = sg_page_iter_page(&sg_iter);
>> - if (!PageDirty(page) && umem->writable && dirty)
>> - set_page_dirty_lock(page);
>> - put_page(page);
>> + if (umem->writable && dirty)
>> + put_user_pages_dirty_lock(&page, 1);
>> + else
>> + put_user_page(page);
>
> Can we get a put_user_page_dirty(struct page 8*pages, bool dirty, npages) ?
>
> It is a common pattern that we might have to conditionaly dirty the pages
> and i feel it would look cleaner if we could move the branch within the
> put_user_page*() function.
>
This sounds reasonable to me, do others have a preference on this? Last time
we discussed it, I recall there was interest in trying to handle the sg lists,
which was where a lot of focus was. I'm not sure if there was a preference one
way or the other, on adding more of these helpers.
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jason Gunthorpe @ 2019-05-23 17:55 UTC (permalink / raw)
To: Jerome Glisse
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523173302.GD5104@redhat.com>
On Thu, May 23, 2019 at 01:33:03PM -0400, Jerome Glisse wrote:
> On Thu, May 23, 2019 at 01:34:29PM -0300, Jason Gunthorpe wrote:
> > On Thu, May 23, 2019 at 11:52:08AM -0400, Jerome Glisse wrote:
> > > On Thu, May 23, 2019 at 12:41:49PM -0300, Jason Gunthorpe wrote:
> > > > On Thu, May 23, 2019 at 11:04:32AM -0400, Jerome Glisse wrote:
> > > > > On Wed, May 22, 2019 at 08:57:37PM -0300, Jason Gunthorpe wrote:
> > > > > > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> > > > > >
> > > > > > > > > So attached is a rebase on top of 5.2-rc1, i have tested with pingpong
> > > > > > > > > (prefetch and not and different sizes). Seems to work ok.
> > > > > > > >
> > > > > > > > Urk, it already doesn't apply to the rdma tree :(
> > > > > > > >
> > > > > > > > The conflicts are a little more extensive than I'd prefer to handle..
> > > > > > > > Can I ask you to rebase it on top of this branch please:
> > > > > > > >
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/jgg-for-next
> > > > > > > >
> > > > > > > > Specifically it conflicts with this patch:
> > > > > > > >
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-next&id=d2183c6f1958e6b6dfdde279f4cee04280710e34
> > > > > >
> > > > > > There is at least one more serious blocker here:
> > > > > >
> > > > > > config ARCH_HAS_HMM_MIRROR
> > > > > > bool
> > > > > > default y
> > > > > > depends on (X86_64 || PPC64)
> > > > > > depends on MMU && 64BIT
> > > > > >
> > > > > > I can't loose ARM64 support for ODP by merging this, that is too
> > > > > > serious of a regression.
> > > > > >
> > > > > > Can you fix it?
> > > > >
> > > > > 5.2 already has patch to fix the Kconfig (ARCH_HAS_HMM_MIRROR and
> > > > > ARCH_HAS_HMM_DEVICE replacing ARCH_HAS_HMM) I need to update nouveau
> > > >
> > > > Newer than 5.2-rc1? Is this why ARCH_HAS_HMM_MIRROR is not used anywhere?
> > >
> > > Yes this is multi-step update, first add the new Kconfig release n,
> > > update driver in release n+1, update core Kconfig in release n+2
> > >
> > > So we are in release n (5.2), in 5.3 i will update nouveau and amdgpu
> > > so that in 5.4 in ca remove the old ARCH_HAS_HMM
> >
> > Why don't you just send the patch for both parts to mm or to DRM?
> >
> > This is very normal - as long as the resulting conflicts would be
> > small during there is no reason not to do this. Can you share the
> > combined patch?
>
> This was tested in the past an resulted in failure. So for now i am
> taking the simplest and easiest path with the least burden for every
> maintainer. It only complexify my life.
I don't know what you tried to do in the past, but it happens all the
time, every merge cycle with success. Not everything can be done, but
changing the signature of one function with one call site should
really not be a problem.
> Note that mm is not a git tree and thus i can not play any git trick
> to help in this endeavor.
I am aware..
> > > > If mm takes the fixup patches so hmm mirror is as reliable as ODP's
> > > > existing stuff, and patch from you to enable ARM64, then we can
> > > > continue to merge into 5.3
> > > >
> > > > So, let us try to get acks on those other threads..
> > >
> > > I will be merging your patchset and Ralph and repost, they are only
> > > minor change mostly that you can not update the driver API in just
> > > one release.
> >
> > Of course you can, we do it all the time. It requires some
> > co-ordination, but as long as the merge conflicts are not big it is
> > fine.
> >
> > Merge the driver API change and the call site updates to -mm and
> > refain from merging horrendously conflicting patches through DRM.
> >
> > In the case of the changes in my HMM RFC it is something like 2
> > lines in DRM that need touching, no problem at all.
> >
> > If you want help I can volunteer make a hmm PR for Linus just for this
> > during the merge window - but Andrew would need to agree and ack the
> > patches.
>
> This was tested in the past and i do not want to go over this issue
> again (or re-iterate the long emails discussion associated with that).
> It failed and it put the burden on every maintainers. So it is easier
> to do the multi-step thing.
>
> You can take a peak at Ralph patchset and yours into one with minor
> changes here:
>
> https://cgit.freedesktop.org/~glisse/linux/log/?h=hmm-5.3
Okay..
This patch needs to use down_read(&mm->mmap_sem) not READ_ONCE:
mm/hmm: do not try to create hmm struct from within hmm_range_register()
Driver should never call hmm_range_register() without a valid and active
registered hmm_mirror and thus without a valid and active hmm struct. So
if that happens just return -EFAULT.
Otherwise it is inconsisent with the locking scheme and has a use
after free race.
I was not sure if the lock could be obtained safely here so I
preferred to use the no-lock alternative of passing in mirror. I still
think you should just change the single call site and sent to -mm.
Thank you for all the other fixes, they look great.
Regards,
Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Khalid Aziz @ 2019-05-23 17:51 UTC (permalink / raw)
To: Kees Cook, Catalin Marinas
Cc: Mark Rutland, kvm, Szabolcs Nagy, Will Deacon, dri-devel,
Linux Memory Management List, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgenii Stepanov,
linux-media, Kevin Brodsky, Ruben Ayrapetyan, Andrey Konovalov,
Ramana Radhakrishnan, Alex Williamson, Yishai Hadas, Mauro
In-Reply-To: <201905211633.6C0BF0C2@keescook>
On 5/21/19 6:04 PM, Kees Cook wrote:
> As an aside: I think Sparc ADI support in Linux actually side-stepped
> this[1] (i.e. chose "solution 1"): "All addresses passed to kernel must
> be non-ADI tagged addresses." (And sadly, "Kernel does not enable ADI
> for kernel code.") I think this was a mistake we should not repeat for
> arm64 (we do seem to be at least in agreement about this, I think).
>
> [1] https://lore.kernel.org/patchwork/patch/654481/
That is a very early version of the sparc ADI patch. Support for tagged
addresses in syscalls was added in later versions and is in the patch
that is in the kernel.
That part "Kernel does not enable ADI for kernel code." is correct. It
is a possible enhancement for future.
--
Khalid
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: John Hubbard @ 2019-05-23 17:46 UTC (permalink / raw)
To: Jason Gunthorpe, Ira Weiny
Cc: john.hubbard@gmail.com, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <20190523173222.GH12145@mellanox.com>
On 5/23/19 10:32 AM, Jason Gunthorpe wrote:
> On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
>>>
>>> @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
>>> * ib_umem_odp_map_dma_single_page().
>>> */
>>> if (npages - (j + 1) > 0)
>>> - release_pages(&local_page_list[j+1],
>>> - npages - (j + 1));
>>> + put_user_pages(&local_page_list[j+1],
>>> + npages - (j + 1));
>>
>> I don't know if we discussed this before but it looks like the use of
>> release_pages() was not entirely correct (or at least not necessary) here. So
>> I think this is ok.
>
> Oh? John switched it from a put_pages loop to release_pages() here:
>
> commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
> Author: John Hubbard <jhubbard@nvidia.com>
> Date: Mon Mar 4 11:46:45 2019 -0800
>
> RDMA/umem: minor bug fix in error handling path
>
> 1. Bug fix: fix an off by one error in the code that cleans up if it fails
> to dma-map a page, after having done a get_user_pages_remote() on a
> range of pages.
>
> 2. Refinement: for that same cleanup code, release_pages() is better than
> put_page() in a loop.
>
>
> And now we are going to back something called put_pages() that
> implements the same for loop the above removed?
>
> Seems like we are going in circles?? John?
>
put_user_pages() is meant to be a drop-in replacement for release_pages(),
so I made the above change as an interim step in moving the callsite from
a loop, to a single call.
And at some point, it may be possible to find a way to optimize put_user_pages()
in a similar way to the batching that release_pages() does, that was part
of the plan for this.
But I do see what you mean: in the interim, maybe put_user_pages() should
just be calling release_pages(), how does that change sound?
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Catalin Marinas @ 2019-05-23 17:43 UTC (permalink / raw)
To: Kees Cook
Cc: enh, Evgenii Stepanov, Andrey Konovalov, Khalid Aziz, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <201905230917.DEE7A75EF0@keescook>
On Thu, May 23, 2019 at 09:38:19AM -0700, Kees Cook wrote:
> On Thu, May 23, 2019 at 03:44:49PM +0100, Catalin Marinas wrote:
> > There is also the obvious requirement which I didn't mention: new user
> > space continues to run on new/subsequent kernel versions. That's one of
> > the points of contention for this series (ignoring MTE) with the
> > maintainers having to guarantee this without much effort. IOW, do the
> > 500K+ new lines in a subsequent kernel version break any user space out
> > there? I'm only talking about the relaxed TBI ABI. Are the usual LTP,
> > syskaller sufficient? Better static analysis would definitely help.
>
> We can't have perfect coverage of people actively (or accidentally)
> working to trick static analyzers (and the compiler) into "forgetting"
> about a __user annotation. We can certainly improve analysis (I see
> the other sub-thread on that), but I'd like that work not to block
> this series.
I don't want to block this series either as it's a prerequisite for MTE
but at the moment I'm not confident we tracked down all the places where
things can break and what the future effort will be to analyse new
kernel changes.
We've made lots of progress since March last year (I think when the
first version was posted) by both identifying new places in the kernel
that need untagging and limiting the ranges that user space can tag
(e.g. an mmap() on a device should not allow tagged pointers). Can we
say we are done or more investigation is needed?
> What on this front would you be comfortable with? Given it's a new
> feature isn't it sufficient to have a CONFIG (and/or boot option)?
I'd rather avoid re-building kernels. A boot option would do, unless we
see value in a per-process (inherited) personality or prctl. The
per-process option has the slight advantage that I can boot my machine
normally while being able to run LTP with a relaxed ABI (and a new libc
which tags pointers). I admit I don't have a very strong argument on a
per-process opt-in.
Another option would be a sysctl control together with a cmdline option.
> > Or, if we ever want MTE to be turned on by default (i.e. tag checking),
> > even if everything is tagged with 0, we have to disallow TBI for user
> > and this includes hwasan. There were a small number of programs using
> > the TBI (I think some JavaScript compilers tried this). But now we are
> > bringing in the hwasan support and this can be a large user base. Shall
> > we add an ELF note for such binaries that use TBI/hwasan?
>
> Just to be clear, you say "disallow TBI for user" -- you mean a
> particular process, yes? i.e. there is no architectural limitation that
> says once we're using MTE nothing can switch to TBI. i.e. a process is
> either doing MTE or TBI (or nothing, but that's the same as TBI).
I may have answered this below. The architecture does not allow TBI
(i.e. faults) if MTE is enabled for a process and address range.
> > > So there needs to be some way to let the kernel know which of three
> > > things it should be doing:
> > > 1- leaving userspace addresses as-is (present)
> > > 2- wiping the top bits before using (this series)
> >
> > (I'd say tolerating rather than wiping since get_user still uses the tag
> > in the current series)
> >
> > The current series does not allow any choice between 1 and 2, the
> > default ABI basically becomes option 2.
>
> What about testing tools that intentionally insert high bits for syscalls
> and are _expecting_ them to fail? It seems the TBI series will break them?
> In that case, do we need to opt into TBI as well?
If there are such tools, then we may need a per-process control. It's
basically an ABI change.
> > > 3- wiping the top bits for most things, but retaining them for MTE as
> > > needed (the future)
> >
> > 2 and 3 are not entirely compatible as a tagged pointer may be checked
> > against the memory colour by the hardware. So you can't have hwasan
> > binary with MTE enabled.
>
> Right: a process must be either MTE or TBI, not both.
Indeed.
> > > I expect MTE to be the "default" in the future. Once a system's libc has
> > > grown support for it, everything will be trying to use MTE. TBI will be
> > > the special case (but TBI is effectively a prerequisite).
> >
> > The kernel handling of tagged pointers is indeed a prerequisite. The ABI
> > distinction between the above 2 and 3 needs to be solved.
>
> Does that need solving now or when the MTE series appears? As there is
> no reason to distinguish between "normal" and "TBI", that doesn't seem
> to need solving at this point?
We don't need to solve 2 vs 3 as long as option 3 is an explicit opt-in
(prctl) by the user. Otherwise the kernel cannot guess whether the user
is using TBI or not (and if it does and still opts in to MTE, we can say
it's a user problem ;)).
> > > To choose between "2" and "3", it seems we need a per-process flag to
> > > opt into TBI (and out of MTE).
> >
> > Or leave option 2 the default and get it to opt in to MTE.
>
> Given that MTE has to "start" at some point in the binary lifetime, I'm
> fine with opting into MTE. I do expect, though, this will feel redundant
> in a couple years as everything will immediately opt-in. But, okay, this
> is therefore an issue for the MTE series.
Unless Google phases out hwasan soon ;), they may live in parallel for a
while, so a choice of TBI vs MTE.
> Alright, the tl;dr appears to be:
> - you want more assurances that we can find __user stripping in the
> kernel more easily. (But this seems like a parallel problem.)
Yes, and that we found all (most) cases now. The reason I don't see it
as a parallel problem is that, as maintainer, I promise an ABI to user
and I'd rather stick to it. I don't want, for example, ncurses to stop
working because of some ioctl() rejecting tagged pointers.
If it's just the occasional one-off bug I'm fine to deal with it. But
no-one convinced me yet that this is the case.
As for the generic driver code (filesystems or other subsystems),
without some clear direction for developers, together with static
checking/sparse, on how user pointers are cast to longs (one example),
it would become my responsibility to identify and fix them up with any
kernel release. This series is not providing such guidance, just adding
untagged_addr() in some places that we think matter.
> - we might need to opt in to TBI with a prctl()
Yes, although still up for discussion.
> - all other concerns are for the future MTE series (though it sounds
> like HWCAP_MTE and a prctl() solve those issues too).
Indeed.
> Is this accurate? What do you see as the blockers for this series at
> this point?
Well, see my comment on your first bullet point above ;).
And thanks for summarising it.
--
Catalin
^ permalink raw reply
* Re: [RFC][PATCH] kernel.h: Add generic roundup_64() macro
From: Steven Rostedt @ 2019-05-23 17:36 UTC (permalink / raw)
To: Linus Torvalds
Cc: LKML, Ben Skeggs, David Airlie, Daniel Vetter, Leon Romanovsky,
Doug Ledford, Jason Gunthorpe, Darrick J. Wong, linux-xfs,
dri-devel, nouveau, linux-rdma, Andrew Morton
In-Reply-To: <CAHk-=whFJqTOk0mSxJGeh38ZxDksgRaMrNV8hqTngiuokyJzew@mail.gmail.com>
On Thu, 23 May 2019 09:51:29 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, May 23, 2019 at 8:27 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > I haven't yet tested this, but what about something like the following:
>
> So that at least handles the constant case that the normal "roundup()"
> case also handles.
>
> At the same time, in the case you are talking about, I really do
> suspect that we have a (non-constant) power of two, and that you
> should have just used "round_up()" which works fine regardless of
> size, and is always efficient.
I think you are correct in this.
act_size = roundup_64(attr->length, MLX5_SW_ICM_BLOCK_SIZE(dm_db->dev));
Where we have:
#define MLX5_SW_ICM_BLOCK_SIZE(dev) (1 << MLX5_LOG_SW_ICM_BLOCK_SIZE(dev))
Which pretty much guarantees that it is a power of two. Thus, the real
fix here is simply to s/roundup/round_up/ as you suggest.
>
> On a slight tangent.. Maybe we should have something like this:
>
> #define size_fn(x, prefix, ...) ({ \
> typeof(x) __ret; \
> switch (sizeof(x)) { \
> case 1: __ret = prefix##8(__VA_ARGS__); break; \
> case 2: __ret = prefix##16(__VA_ARGS__); break; \
> case 4: __ret = prefix##32(__VA_ARGS__); break; \
> case 8: __ret = prefix##64(__VA_ARGS__); break; \
> default: __ret = prefix##bad(__VA_ARGS__); \
> } __ret; })
>
> #define type_fn(x, prefix, ...) ({ \
> typeof(x) __ret; \
> if ((typeof(x))-1 > 1) \
> __ret = size_fn(x, prefix##_u, __VA_ARGS__); \
> else \
> __ret = size_fn(x, prefix##_s, __VA_ARGS__); \
> __ret; })
>
> which would allow typed integer functions like this. So you could do
> something like
>
> #define round_up(x, y) size_fn(x, round_up_size, x, y)
>
> and then you define functions for round_up_size8/16/32/64 (and you
You mean define functions for round_up_size_{u|s}8/16/32/64
> have toi declare - but not define - round_up_sizebad()).
>
> Of course, you probably want the usual "at least use 'int'" semantics,
> in which case the "type" should be "(x)+0":
>
> #define round_up(x, y) size_fn((x)+0, round_up_size, x, y)
>
> and the 8-bit and 16-bit cases will never be used.
I'm curious to what the advantage of that is?
>
> We have a lot of cases where we end up using "type overloading" by
> size. The most explicit case is perhaps "get_user()" and "put_user()",
> but this whole round_up thing is another example.
>
> Maybe we never really care about "char" and "short", and always want
> just the "int-vs-long-vs-longlong"? That would make the cases simpler
> (32 and 64). And maybe we never care about sign. But we could try to
> have some unified helper model like the above..
It may be simpler and perhaps more robust if we keep the char and short
cases.
I'm fine with adding something like this for round_up(), but do we want
to have a generic roundup_64() as well? I'm also thinking that we
perhaps should test for power of two on roundup():
#define roundup(x, y) ( \
{ \
typeof(y) __y = y; \
typeof(x) __x; \
\
if (__y & (__y - 1)) \
__x = round_up(x, __y); \
else \
__x = (((x) + (__y - 1)) / __y) * __y; \
__x; \
})
-- Steve
^ permalink raw reply
* Re: [PATCH v4 0/1] Use HMM for ODP v4
From: Jerome Glisse @ 2019-05-23 17:33 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-rdma, Leon Romanovsky, Doug Ledford,
Artemy Kovalyov, Moni Shoua, Mike Marciniszyn, Kaike Wan,
Dennis Dalessandro
In-Reply-To: <20190523163429.GC12159@ziepe.ca>
On Thu, May 23, 2019 at 01:34:29PM -0300, Jason Gunthorpe wrote:
> On Thu, May 23, 2019 at 11:52:08AM -0400, Jerome Glisse wrote:
> > On Thu, May 23, 2019 at 12:41:49PM -0300, Jason Gunthorpe wrote:
> > > On Thu, May 23, 2019 at 11:04:32AM -0400, Jerome Glisse wrote:
> > > > On Wed, May 22, 2019 at 08:57:37PM -0300, Jason Gunthorpe wrote:
> > > > > On Wed, May 22, 2019 at 01:48:52PM -0400, Jerome Glisse wrote:
> > > > >
> > > > > > > > So attached is a rebase on top of 5.2-rc1, i have tested with pingpong
> > > > > > > > (prefetch and not and different sizes). Seems to work ok.
> > > > > > >
> > > > > > > Urk, it already doesn't apply to the rdma tree :(
> > > > > > >
> > > > > > > The conflicts are a little more extensive than I'd prefer to handle..
> > > > > > > Can I ask you to rebase it on top of this branch please:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=wip/jgg-for-next
> > > > > > >
> > > > > > > Specifically it conflicts with this patch:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/commit/?h=wip/jgg-for-next&id=d2183c6f1958e6b6dfdde279f4cee04280710e34
> > > > >
> > > > > There is at least one more serious blocker here:
> > > > >
> > > > > config ARCH_HAS_HMM_MIRROR
> > > > > bool
> > > > > default y
> > > > > depends on (X86_64 || PPC64)
> > > > > depends on MMU && 64BIT
> > > > >
> > > > > I can't loose ARM64 support for ODP by merging this, that is too
> > > > > serious of a regression.
> > > > >
> > > > > Can you fix it?
> > > >
> > > > 5.2 already has patch to fix the Kconfig (ARCH_HAS_HMM_MIRROR and
> > > > ARCH_HAS_HMM_DEVICE replacing ARCH_HAS_HMM) I need to update nouveau
> > >
> > > Newer than 5.2-rc1? Is this why ARCH_HAS_HMM_MIRROR is not used anywhere?
> >
> > Yes this is multi-step update, first add the new Kconfig release n,
> > update driver in release n+1, update core Kconfig in release n+2
> >
> > So we are in release n (5.2), in 5.3 i will update nouveau and amdgpu
> > so that in 5.4 in ca remove the old ARCH_HAS_HMM
>
> Why don't you just send the patch for both parts to mm or to DRM?
>
> This is very normal - as long as the resulting conflicts would be
> small during there is no reason not to do this. Can you share the
> combined patch?
This was tested in the past an resulted in failure. So for now i am
taking the simplest and easiest path with the least burden for every
maintainer. It only complexify my life.
Note that mm is not a git tree and thus i can not play any git trick
to help in this endeavor.
> > > If mm takes the fixup patches so hmm mirror is as reliable as ODP's
> > > existing stuff, and patch from you to enable ARM64, then we can
> > > continue to merge into 5.3
> > >
> > > So, let us try to get acks on those other threads..
> >
> > I will be merging your patchset and Ralph and repost, they are only
> > minor change mostly that you can not update the driver API in just
> > one release.
>
> Of course you can, we do it all the time. It requires some
> co-ordination, but as long as the merge conflicts are not big it is
> fine.
>
> Merge the driver API change and the call site updates to -mm and
> refain from merging horrendously conflicting patches through DRM.
>
> In the case of the changes in my HMM RFC it is something like 2
> lines in DRM that need touching, no problem at all.
>
> If you want help I can volunteer make a hmm PR for Linus just for this
> during the merge window - but Andrew would need to agree and ack the
> patches.
This was tested in the past and i do not want to go over this issue
again (or re-iterate the long emails discussion associated with that).
It failed and it put the burden on every maintainers. So it is easier
to do the multi-step thing.
You can take a peak at Ralph patchset and yours into one with minor
changes here:
https://cgit.freedesktop.org/~glisse/linux/log/?h=hmm-5.3
I am about to start testing it with nouveau, amdgpu and RDMA.
Cheers,
Jérôme
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: Jason Gunthorpe @ 2019-05-23 17:32 UTC (permalink / raw)
To: Ira Weiny
Cc: john.hubbard@gmail.com, Andrew Morton, linux-mm@kvack.org, LKML,
linux-rdma@vger.kernel.org, linux-fsdevel@vger.kernel.org,
John Hubbard, Doug Ledford, Mike Marciniszyn, Dennis Dalessandro,
Christian Benvenuti, Jan Kara
In-Reply-To: <20190523172852.GA27175@iweiny-DESK2.sc.intel.com>
On Thu, May 23, 2019 at 10:28:52AM -0700, Ira Weiny wrote:
> >
> > @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> > * ib_umem_odp_map_dma_single_page().
> > */
> > if (npages - (j + 1) > 0)
> > - release_pages(&local_page_list[j+1],
> > - npages - (j + 1));
> > + put_user_pages(&local_page_list[j+1],
> > + npages - (j + 1));
>
> I don't know if we discussed this before but it looks like the use of
> release_pages() was not entirely correct (or at least not necessary) here. So
> I think this is ok.
Oh? John switched it from a put_pages loop to release_pages() here:
commit 75a3e6a3c129cddcc683538d8702c6ef998ec589
Author: John Hubbard <jhubbard@nvidia.com>
Date: Mon Mar 4 11:46:45 2019 -0800
RDMA/umem: minor bug fix in error handling path
1. Bug fix: fix an off by one error in the code that cleans up if it fails
to dma-map a page, after having done a get_user_pages_remote() on a
range of pages.
2. Refinement: for that same cleanup code, release_pages() is better than
put_page() in a loop.
And now we are going to back something called put_pages() that
implements the same for loop the above removed?
Seems like we are going in circles?? John?
Jason
^ permalink raw reply
* Re: [PATCH 1/1] infiniband/mm: convert put_page() to put_user_page*()
From: Ira Weiny @ 2019-05-23 17:28 UTC (permalink / raw)
To: john.hubbard
Cc: Andrew Morton, linux-mm, Jason Gunthorpe, LKML, linux-rdma,
linux-fsdevel, John Hubbard, Doug Ledford, Mike Marciniszyn,
Dennis Dalessandro, Christian Benvenuti, Jan Kara,
Jason Gunthorpe
In-Reply-To: <20190523072537.31940-2-jhubbard@nvidia.com>
On Thu, May 23, 2019 at 12:25:37AM -0700, john.hubbard@gmail.com wrote:
> From: John Hubbard <jhubbard@nvidia.com>
>
> For infiniband code that retains pages via get_user_pages*(),
> release those pages via the new put_user_page(), or
> put_user_pages*(), instead of put_page()
>
> This is a tiny part of the second step of fixing the problem described
> in [1]. The steps are:
>
> 1) Provide put_user_page*() routines, intended to be used
> for releasing pages that were pinned via get_user_pages*().
>
> 2) Convert all of the call sites for get_user_pages*(), to
> invoke put_user_page*(), instead of put_page(). This involves dozens of
> call sites, and will take some time.
>
> 3) After (2) is complete, use get_user_pages*() and put_user_page*() to
> implement tracking of these pages. This tracking will be separate from
> the existing struct page refcounting.
>
> 4) Use the tracking and identification of these pages, to implement
> special handling (especially in writeback paths) when the pages are
> backed by a filesystem. Again, [1] provides details as to why that is
> desirable.
>
> [1] https://lwn.net/Articles/753027/ : "The Trouble with get_user_pages()"
>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Cc: Christian Benvenuti <benve@cisco.com>
>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Acked-by: Jason Gunthorpe <jgg@mellanox.com>
> Tested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> drivers/infiniband/core/umem.c | 7 ++++---
> drivers/infiniband/core/umem_odp.c | 10 +++++-----
> drivers/infiniband/hw/hfi1/user_pages.c | 11 ++++-------
> drivers/infiniband/hw/mthca/mthca_memfree.c | 6 +++---
> drivers/infiniband/hw/qib/qib_user_pages.c | 11 ++++-------
> drivers/infiniband/hw/qib/qib_user_sdma.c | 6 +++---
> drivers/infiniband/hw/usnic/usnic_uiom.c | 7 ++++---
> 7 files changed, 27 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index e7ea819fcb11..673f0d240b3e 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -54,9 +54,10 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
>
> for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> page = sg_page_iter_page(&sg_iter);
> - if (!PageDirty(page) && umem->writable && dirty)
> - set_page_dirty_lock(page);
> - put_page(page);
> + if (umem->writable && dirty)
> + put_user_pages_dirty_lock(&page, 1);
> + else
> + put_user_page(page);
> }
>
> sg_free_table(&umem->sg_head);
> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
> index f962b5bbfa40..17e46df3990a 100644
> --- a/drivers/infiniband/core/umem_odp.c
> +++ b/drivers/infiniband/core/umem_odp.c
> @@ -487,7 +487,7 @@ void ib_umem_odp_release(struct ib_umem_odp *umem_odp)
> * The function returns -EFAULT if the DMA mapping operation fails. It returns
> * -EAGAIN if a concurrent invalidation prevents us from updating the page.
> *
> - * The page is released via put_page even if the operation failed. For
> + * The page is released via put_user_page even if the operation failed. For
> * on-demand pinning, the page is released whenever it isn't stored in the
> * umem.
> */
> @@ -536,7 +536,7 @@ static int ib_umem_odp_map_dma_single_page(
> }
>
> out:
> - put_page(page);
> + put_user_page(page);
>
> if (remove_existing_mapping) {
> ib_umem_notifier_start_account(umem_odp);
> @@ -659,7 +659,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> ret = -EFAULT;
> break;
> }
> - put_page(local_page_list[j]);
> + put_user_page(local_page_list[j]);
> continue;
> }
>
> @@ -686,8 +686,8 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt,
> * ib_umem_odp_map_dma_single_page().
> */
> if (npages - (j + 1) > 0)
> - release_pages(&local_page_list[j+1],
> - npages - (j + 1));
> + put_user_pages(&local_page_list[j+1],
> + npages - (j + 1));
I don't know if we discussed this before but it looks like the use of
release_pages() was not entirely correct (or at least not necessary) here. So
I think this is ok.
As for testing, I have been running with this patch for a while but I don't
have ODP hardware so that testing would not cover this code path. So you can
add my:
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> break;
> }
> }
> diff --git a/drivers/infiniband/hw/hfi1/user_pages.c b/drivers/infiniband/hw/hfi1/user_pages.c
> index 02eee8eff1db..b89a9b9aef7a 100644
> --- a/drivers/infiniband/hw/hfi1/user_pages.c
> +++ b/drivers/infiniband/hw/hfi1/user_pages.c
> @@ -118,13 +118,10 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t np
> void hfi1_release_user_pages(struct mm_struct *mm, struct page **p,
> size_t npages, bool dirty)
> {
> - size_t i;
> -
> - for (i = 0; i < npages; i++) {
> - if (dirty)
> - set_page_dirty_lock(p[i]);
> - put_page(p[i]);
> - }
> + if (dirty)
> + put_user_pages_dirty_lock(p, npages);
> + else
> + put_user_pages(p, npages);
>
> if (mm) { /* during close after signal, mm can be NULL */
> atomic64_sub(npages, &mm->pinned_vm);
> diff --git a/drivers/infiniband/hw/mthca/mthca_memfree.c b/drivers/infiniband/hw/mthca/mthca_memfree.c
> index 8ff0e90d7564..edccfd6e178f 100644
> --- a/drivers/infiniband/hw/mthca/mthca_memfree.c
> +++ b/drivers/infiniband/hw/mthca/mthca_memfree.c
> @@ -482,7 +482,7 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar,
>
> ret = pci_map_sg(dev->pdev, &db_tab->page[i].mem, 1, PCI_DMA_TODEVICE);
> if (ret < 0) {
> - put_page(pages[0]);
> + put_user_page(pages[0]);
> goto out;
> }
>
> @@ -490,7 +490,7 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar,
> mthca_uarc_virt(dev, uar, i));
> if (ret) {
> pci_unmap_sg(dev->pdev, &db_tab->page[i].mem, 1, PCI_DMA_TODEVICE);
> - put_page(sg_page(&db_tab->page[i].mem));
> + put_user_page(sg_page(&db_tab->page[i].mem));
> goto out;
> }
>
> @@ -556,7 +556,7 @@ void mthca_cleanup_user_db_tab(struct mthca_dev *dev, struct mthca_uar *uar,
> if (db_tab->page[i].uvirt) {
> mthca_UNMAP_ICM(dev, mthca_uarc_virt(dev, uar, i), 1);
> pci_unmap_sg(dev->pdev, &db_tab->page[i].mem, 1, PCI_DMA_TODEVICE);
> - put_page(sg_page(&db_tab->page[i].mem));
> + put_user_page(sg_page(&db_tab->page[i].mem));
> }
> }
>
> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c
> index f712fb7fa82f..bfbfbb7e0ff4 100644
> --- a/drivers/infiniband/hw/qib/qib_user_pages.c
> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c
> @@ -40,13 +40,10 @@
> static void __qib_release_user_pages(struct page **p, size_t num_pages,
> int dirty)
> {
> - size_t i;
> -
> - for (i = 0; i < num_pages; i++) {
> - if (dirty)
> - set_page_dirty_lock(p[i]);
> - put_page(p[i]);
> - }
> + if (dirty)
> + put_user_pages_dirty_lock(p, num_pages);
> + else
> + put_user_pages(p, num_pages);
> }
>
> /**
> diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c
> index 0c204776263f..ac5bdb02144f 100644
> --- a/drivers/infiniband/hw/qib/qib_user_sdma.c
> +++ b/drivers/infiniband/hw/qib/qib_user_sdma.c
> @@ -317,7 +317,7 @@ static int qib_user_sdma_page_to_frags(const struct qib_devdata *dd,
> * the caller can ignore this page.
> */
> if (put) {
> - put_page(page);
> + put_user_page(page);
> } else {
> /* coalesce case */
> kunmap(page);
> @@ -631,7 +631,7 @@ static void qib_user_sdma_free_pkt_frag(struct device *dev,
> kunmap(pkt->addr[i].page);
>
> if (pkt->addr[i].put_page)
> - put_page(pkt->addr[i].page);
> + put_user_page(pkt->addr[i].page);
> else
> __free_page(pkt->addr[i].page);
> } else if (pkt->addr[i].kvaddr) {
> @@ -706,7 +706,7 @@ static int qib_user_sdma_pin_pages(const struct qib_devdata *dd,
> /* if error, return all pages not managed by pkt */
> free_pages:
> while (i < j)
> - put_page(pages[i++]);
> + put_user_page(pages[i++]);
>
> done:
> return ret;
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
> index e312f522a66d..0b0237d41613 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.c
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
> @@ -75,9 +75,10 @@ static void usnic_uiom_put_pages(struct list_head *chunk_list, int dirty)
> for_each_sg(chunk->page_list, sg, chunk->nents, i) {
> page = sg_page(sg);
> pa = sg_phys(sg);
> - if (!PageDirty(page) && dirty)
> - set_page_dirty_lock(page);
> - put_page(page);
> + if (dirty)
> + put_user_pages_dirty_lock(&page, 1);
> + else
> + put_user_page(page);
> usnic_dbg("pa: %pa\n", &pa);
> }
> kfree(chunk);
> --
> 2.21.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox