Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Mon, Dec 18, 2017 at 3:54 AM, Peter Zijlstra wrote: > On Fri, Dec 15, 2017 at 08:38:02AM -0800, Dan Williams wrote: > >> The motivation was that I noticed that get_user_pages_fast() was doing >> a full pud_access_permitted() check, but the get_user_pages() slow >> path

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Mon, Dec 18, 2017 at 3:54 AM, Peter Zijlstra wrote: > On Fri, Dec 15, 2017 at 08:38:02AM -0800, Dan Williams wrote: > >> The motivation was that I noticed that get_user_pages_fast() was doing >> a full pud_access_permitted() check, but the get_user_pages() slow >> path was only doing a

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 08:38:02AM -0800, Dan Williams wrote: > The motivation was that I noticed that get_user_pages_fast() was doing > a full pud_access_permitted() check, but the get_user_pages() slow > path was only doing a pud_write() check. That was inconsistent so I > went to go resolve

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 08:38:02AM -0800, Dan Williams wrote: > The motivation was that I noticed that get_user_pages_fast() was doing > a full pud_access_permitted() check, but the get_user_pages() slow > path was only doing a pud_write() check. That was inconsistent so I > went to go resolve

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/15/2017 06:52 PM, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 6:48 PM, Al Viro wrote: >> Treating protection key bits as "escalate to page fault and let that >> deal with the checks" should be fine > > Well, it's *semantically* fine and I think it's the right

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/15/2017 06:52 PM, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 6:48 PM, Al Viro wrote: >> Treating protection key bits as "escalate to page fault and let that >> deal with the checks" should be fine > > Well, it's *semantically* fine and I think it's the right model from > that

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 6:52 PM, Linus Torvalds wrote: > > However, since the main use case of protection keys is probably > databases (Dave?) and since those also might be performance-sensitive > about direct-IO doing page table lookups, it might not be great in >

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 6:52 PM, Linus Torvalds wrote: > > However, since the main use case of protection keys is probably > databases (Dave?) and since those also might be performance-sensitive > about direct-IO doing page table lookups, it might not be great in > practice. Anyway, I reverted

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 6:48 PM, Al Viro wrote: > > Treating protection key bits as "escalate to page fault and let that > deal with the checks" should be fine Well, it's *semantically* fine and I think it's the right model from that standpoint. However, since the main

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 6:48 PM, Al Viro wrote: > > Treating protection key bits as "escalate to page fault and let that > deal with the checks" should be fine Well, it's *semantically* fine and I think it's the right model from that standpoint. However, since the main use case of protection

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 06:28:36PM -0800, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 5:25 PM, Dave Hansen wrote: > > > > I think the reason we needed VMA and PTE checks was the > > get_user_pages_fast() path not having a VMA. > > That is indeed the point of

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 06:28:36PM -0800, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 5:25 PM, Dave Hansen wrote: > > > > I think the reason we needed VMA and PTE checks was the > > get_user_pages_fast() path not having a VMA. > > That is indeed the point of get_user_pages_fast(): no vma

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 5:25 PM, Dave Hansen wrote: > > I think the reason we needed VMA and PTE checks was the > get_user_pages_fast() path not having a VMA. That is indeed the point of get_user_pages_fast(): no vma lookup, no locking, just "do the default case as

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 5:25 PM, Dave Hansen wrote: > > I think the reason we needed VMA and PTE checks was the > get_user_pages_fast() path not having a VMA. That is indeed the point of get_user_pages_fast(): no vma lookup, no locking, just "do the default case as streamlined as possible". But

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 5:10 PM, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 4:29 PM, Dan Williams > wrote: >> So do you want to do a straight revert of these that went in for 4.15: > > I think that's the right thing to do, but would

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 5:10 PM, Linus Torvalds wrote: > On Fri, Dec 15, 2017 at 4:29 PM, Dan Williams > wrote: >> So do you want to do a straight revert of these that went in for 4.15: > > I think that's the right thing to do, but would want to verify that > there are no *other* issues than

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/15/2017 05:10 PM, Linus Torvalds wrote: > Because *if* we want to check protection keys, I think we should do > that at the vma layer, partly exactly because the exact implementation > of protection keys is so architecture-specific, and partly because I > don't think it makes sense to check

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/15/2017 05:10 PM, Linus Torvalds wrote: > Because *if* we want to check protection keys, I think we should do > that at the vma layer, partly exactly because the exact implementation > of protection keys is so architecture-specific, and partly because I > don't think it makes sense to check

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:29 PM, Dan Williams wrote: > So do you want to do a straight revert of these that went in for 4.15: I think that's the right thing to do, but would want to verify that there are no *other* issues than just the attempt at PKRU. The commit

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:29 PM, Dan Williams wrote: > So do you want to do a straight revert of these that went in for 4.15: I think that's the right thing to do, but would want to verify that there are no *other* issues than just the attempt at PKRU. The commit message does talk about

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:31 PM, Al Viro wrote: >> >> The fact is, if we have non-user mappings in the user part of the >> address space, we _need_ to teach access_ok() about them, because >> fundamentally any "get_user()/put_user()" will happily ignore the lack >> of

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:31 PM, Al Viro wrote: >> >> The fact is, if we have non-user mappings in the user part of the >> address space, we _need_ to teach access_ok() about them, because >> fundamentally any "get_user()/put_user()" will happily ignore the lack >> of PAGE_USER (since those

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 04:20:31PM -0800, Linus Torvalds wrote: > On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > > > So we actually need the pte_access_permitted() stuff if we want to > > ensure we're not stepping on !PAGE_USER things. > > We really don't. Not

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 04:20:31PM -0800, Linus Torvalds wrote: > On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > > > So we actually need the pte_access_permitted() stuff if we want to > > ensure we're not stepping on !PAGE_USER things. > > We really don't. Not in that complex and

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:20 PM, Linus Torvalds wrote: > > On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > > > So we actually need the pte_access_permitted() stuff if we want to > > ensure we're not stepping on !PAGE_USER things. >

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 4:20 PM, Linus Torvalds wrote: > > On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > > > So we actually need the pte_access_permitted() stuff if we want to > > ensure we're not stepping on !PAGE_USER things. > > We really don't. Not in that complex and broken

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > So we actually need the pte_access_permitted() stuff if we want to > ensure we're not stepping on !PAGE_USER things. We really don't. Not in that complex and broken format, and not for every level. Also, while I

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 11:51 PM, Peter Zijlstra wrote: > > So we actually need the pte_access_permitted() stuff if we want to > ensure we're not stepping on !PAGE_USER things. We really don't. Not in that complex and broken format, and not for every level. Also, while I think we *should* check

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 3:38 AM, Peter Zijlstra wrote: > On Fri, Dec 15, 2017 at 11:25:29AM +0100, Peter Zijlstra wrote: >> The memory one is also clearly wrong, not having access does not a write >> fault make. If we have pte_write() set we should not do_wp_page() just >>

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 3:38 AM, Peter Zijlstra wrote: > On Fri, Dec 15, 2017 at 11:25:29AM +0100, Peter Zijlstra wrote: >> The memory one is also clearly wrong, not having access does not a write >> fault make. If we have pte_write() set we should not do_wp_page() just >> because we don't have

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 03:37:30PM +0100, Peter Zijlstra wrote: > Kirill did point out that my patch(es) break FOLL_DUMP in that it would > now exclude pkey protected pages from core-dumps. > > My counter argument is that it will now properly exclude !_PAGE_USER > pages. > > If we change

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 03:37:30PM +0100, Peter Zijlstra wrote: > Kirill did point out that my patch(es) break FOLL_DUMP in that it would > now exclude pkey protected pages from core-dumps. > > My counter argument is that it will now properly exclude !_PAGE_USER > pages. > > If we change

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 11:25:29AM +0100, Peter Zijlstra wrote: > The memory one is also clearly wrong, not having access does not a write > fault make. If we have pte_write() set we should not do_wp_page() just > because we don't have access. This falls under the "doing anything other > than hard

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 11:25:29AM +0100, Peter Zijlstra wrote: > The memory one is also clearly wrong, not having access does not a write > fault make. If we have pte_write() set we should not do_wp_page() just > because we don't have access. This falls under the "doing anything other > than hard

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 09:00:41AM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 09:04:56PM -0800, Dave Hansen wrote: > > > > I've got some additions to the selftests and a fix where we pass FOLL_* > > flags around a bit more instead of just 'write'. I'll get those out as > > soon as I

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Fri, Dec 15, 2017 at 09:00:41AM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 09:04:56PM -0800, Dave Hansen wrote: > > > > I've got some additions to the selftests and a fix where we pass FOLL_* > > flags around a bit more instead of just 'write'. I'll get those out as > > soon as I

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 09:04:56PM -0800, Dave Hansen wrote: > > I've got some additions to the selftests and a fix where we pass FOLL_* > flags around a bit more instead of just 'write'. I'll get those out as > soon as I do a bit more testing. Try the below; I have more in the works, but this

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 09:04:56PM -0800, Dave Hansen wrote: > > I've got some additions to the selftests and a fix where we pass FOLL_* > flags around a bit more instead of just 'write'. I'll get those out as > soon as I do a bit more testing. Try the below; I have more in the works, but this

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 10:09:24PM -0800, Linus Torvalds wrote: > On Dec 14, 2017 21:04, "Dave Hansen" wrote: > Can we please just undo that broken crap instead of trying to "fix" it? > > It was wrong. We absolutely do not want to complicate the gup path. > > Let's fet

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 10:09:24PM -0800, Linus Torvalds wrote: > On Dec 14, 2017 21:04, "Dave Hansen" wrote: > Can we please just undo that broken crap instead of trying to "fix" it? > > It was wrong. We absolutely do not want to complicate the gup path. > > Let's fet rid of those broken

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/14/2017 12:54 PM, Peter Zijlstra wrote: >> That short-circuits the page fault pretty quickly. So, basically, the >> rule is: if the hardware says you tripped over pkey permissions, you >> die. We don't try to do anything to the underlying page *before* saying >> that you die. > That only

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/14/2017 12:54 PM, Peter Zijlstra wrote: >> That short-circuits the page fault pretty quickly. So, basically, the >> rule is: if the hardware says you tripped over pkey permissions, you >> die. We don't try to do anything to the underlying page *before* saying >> that you die. > That only

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 09:54:50PM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 12:44:58PM -0800, Dave Hansen wrote: > > On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > > > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > > > looks very dodgy to me. How does that

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 09:54:50PM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 12:44:58PM -0800, Dave Hansen wrote: > > On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > > > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > > > looks very dodgy to me. How does that

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 12:44:58PM -0800, Dave Hansen wrote: > On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > > looks very dodgy to me. How does that not result in endlessly CoW'ing > > the same page over and over when we

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 12:44:58PM -0800, Dave Hansen wrote: > On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > > looks very dodgy to me. How does that not result in endlessly CoW'ing > > the same page over and over when we

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > looks very dodgy to me. How does that not result in endlessly CoW'ing > the same page over and over when we have a PKEY disallowing write access > on that page? I'm not seeing

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On 12/14/2017 06:37 AM, Peter Zijlstra wrote: > I'm also looking at pte_access_permitted() in handle_pte_fault(); that > looks very dodgy to me. How does that not result in endlessly CoW'ing > the same page over and over when we have a PKEY disallowing write access > on that page? I'm not seeing

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 01:41:17PM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 12:27:27PM +0100, Peter Zijlstra wrote: > > The gup_*_range() functions which implement __get_user_pages_fast() do > > a p*_access_permitted() test to see if the memory is at all accessible > > (tests both

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 01:41:17PM +0100, Peter Zijlstra wrote: > On Thu, Dec 14, 2017 at 12:27:27PM +0100, Peter Zijlstra wrote: > > The gup_*_range() functions which implement __get_user_pages_fast() do > > a p*_access_permitted() test to see if the memory is at all accessible > > (tests both

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 12:27:27PM +0100, Peter Zijlstra wrote: > The gup_*_range() functions which implement __get_user_pages_fast() do > a p*_access_permitted() test to see if the memory is at all accessible > (tests both _PAGE_USER|_PAGE_RW as well as architectural things like > pkeys). > >

Re: [PATCH v2 01/17] mm/gup: Fixup p*_access_permitted()

On Thu, Dec 14, 2017 at 12:27:27PM +0100, Peter Zijlstra wrote: > The gup_*_range() functions which implement __get_user_pages_fast() do > a p*_access_permitted() test to see if the memory is at all accessible > (tests both _PAGE_USER|_PAGE_RW as well as architectural things like > pkeys). > >