* [PATCH V5 13/14] powerpc/vas: Display process stuck message
From: Haren Myneni @ 2020-01-22 8:25 UTC (permalink / raw)
To: mpe; +Cc: mikey, herbert, npiggin, hch, oohall, sukadev, linuxppc-dev
In-Reply-To: <1579679802.26081.6.camel@hbabu-laptop>
Process can not close send window until all requests are processed.
Means wait until window state is not busy and send credits are
returned. Display debug message in case taking longer to close the
window.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 1439a6f..88cecff 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1182,6 +1182,7 @@ static void poll_window_credits(struct vas_window *window)
{
u64 val;
int creds, mode;
+ int count = 0;
val = read_hvwc_reg(window, VREG(WINCTL));
if (window->tx_win)
@@ -1200,10 +1201,26 @@ static void poll_window_credits(struct vas_window *window)
creds = GET_FIELD(VAS_LRX_WCRED, val);
}
+ /*
+ * Takes around few microseconds to complete all pending requests
+ * and return credits.
+ * TODO: Scan fault FIFO and invalidate CRBs points to this window
+ * and issue CRB Kill to stop all pending requests. Need only
+ * if there is a bug in NX or fault handling in kernel.
+ */
if (creds < window->wcreds_max) {
val = 0;
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(msecs_to_jiffies(10));
+ count++;
+ /*
+ * Process can not close send window until all credits are
+ * returned.
+ */
+ if (!(count % 10000))
+ pr_debug("%s() pid %d stuck? retries %d\n", __func__,
+ vas_window_pid(window), count);
+
goto retry;
}
}
@@ -1217,6 +1234,7 @@ static void poll_window_busy_state(struct vas_window *window)
{
int busy;
u64 val;
+ int count = 0;
retry:
val = read_hvwc_reg(window, VREG(WIN_STATUS));
@@ -1225,6 +1243,15 @@ static void poll_window_busy_state(struct vas_window *window)
val = 0;
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(msecs_to_jiffies(5));
+ count++;
+ /*
+ * Takes around 5 microseconds to process all pending
+ * requests.
+ */
+ if (!(count % 10000))
+ pr_debug("%s() pid %d stuck? retries %d\n", __func__,
+ vas_window_pid(window), count);
+
goto retry;
}
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH V5 14/14] powerpc/vas: Free send window in VAS instance after credits returned
From: Haren Myneni @ 2020-01-22 8:26 UTC (permalink / raw)
To: mpe; +Cc: mikey, herbert, npiggin, hch, oohall, sukadev, linuxppc-dev
In-Reply-To: <1579679802.26081.6.camel@hbabu-laptop>
NX may be processing requests while trying to close window. Wait until
all credits are returned and then free send window from VAS instance.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
arch/powerpc/platforms/powernv/vas-window.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
index 88cecff..5f9c915 100644
--- a/arch/powerpc/platforms/powernv/vas-window.c
+++ b/arch/powerpc/platforms/powernv/vas-window.c
@@ -1316,14 +1316,14 @@ int vas_win_close(struct vas_window *window)
unmap_paste_region(window);
- clear_vinst_win(window);
-
poll_window_busy_state(window);
unpin_close_window(window);
poll_window_credits(window);
+ clear_vinst_win(window);
+
poll_window_castout(window);
/* if send window, drop reference to matching receive window */
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH RFC v1] mm: is_mem_section_removable() overhaul
From: David Hildenbrand @ 2020-01-22 10:39 UTC (permalink / raw)
To: Michal Hocko
Cc: Nathan Lynch, Stephen Rothwell, Thomas Gleixner,
Anshuman Khandual, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List, Linux MM, Paul Mackerras,
Nathan Fontenot, Leonardo Bras, Dan Williams, linuxppc-dev,
Andrew Morton, Allison Randal, lantianyu1986
In-Reply-To: <20200121120714.GJ29276@dhcp22.suse.cz>
>>> Really, the interface is flawed and should have never been merged in the
>>> first place. We cannot simply remove it altogether I am afraid so let's
>>> at least remove the bogus code and pretend that the world is a better
>>> place where everything is removable except the reality sucks...
>>
>> As I expressed already, the interface works as designed/documented and
>> has been used like that for years.
>
> It seems we do differ in the usefulness though. Using a crappy interface
> for years doesn't make it less crappy. I do realize we cannot remove the
> interface but we can remove issues with the implementation and I dare to
> say that most existing users wouldn't really notice.
Well, at least powerpc-utils (why this interface was introduced) will
notice a) performance wise and b) because more logging output will be
generated (obviously non-offlineable blocks will be tried to offline).
However, it should not break, because we could have had races
before/false positives.
>
>> I tend to agree that it never should have been merged like that.
>>
>> We have (at least) two places that are racy (with concurrent memory
>> hotplug):
>>
>> 1. /sys/.../memoryX/removable
>> - a) make it always return yes and make the interface useless
>> - b) add proper locking and keep it running as is (e.g., so David can
>> identify offlineable memory blocks :) ).
>>
>> 2. /sys/.../memoryX/valid_zones
>> - a) always return "none" if the memory is online
>> - b) add proper locking and keep it running as is
>> - c) cache the result ("zone") when a block is onlined (e.g., in
>> mem->zone. If it is NULL, either mixed zones or unknown)
>>
>> At least 2. already scream for a proper device_lock() locking as the
>> mem->state is not stable across the function call.
>>
>> 1a and 2a are the easiest solutions but remove all ways to identify if a
>> memory block could theoretically be offlined - without trying
>> (especially, also to identify the MOVABLE zone).
>>
>> I tend to prefer 1b) and 2c), paired with proper device_lock() locking.
>> We don't affect existing use cases but are able to simplify the code +
>> fix the races.
>>
>> What's your opinion? Any alternatives?
>
> 1a) and 2c) if you ask me.
>
I'll look into that all, just might take a little (busy with a lot of
stuff). But after all, it does not seem to be urgent.
1a) will be easy, I'll post a patch soon that we can let rest in -next
for a bit to see if people start to scream out loud.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH RFC v1] mm: is_mem_section_removable() overhaul
From: Michal Hocko @ 2020-01-22 10:42 UTC (permalink / raw)
To: David Hildenbrand
Cc: Nathan Lynch, Stephen Rothwell, Thomas Gleixner,
Anshuman Khandual, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List, Linux MM, Paul Mackerras,
Nathan Fontenot, Leonardo Bras, Dan Williams, linuxppc-dev,
Andrew Morton, Allison Randal, lantianyu1986
In-Reply-To: <a29b49b9-28ad-44fa-6c0b-90cd43902f29@redhat.com>
On Wed 22-01-20 11:39:08, David Hildenbrand wrote:
> >>> Really, the interface is flawed and should have never been merged in the
> >>> first place. We cannot simply remove it altogether I am afraid so let's
> >>> at least remove the bogus code and pretend that the world is a better
> >>> place where everything is removable except the reality sucks...
> >>
> >> As I expressed already, the interface works as designed/documented and
> >> has been used like that for years.
> >
> > It seems we do differ in the usefulness though. Using a crappy interface
> > for years doesn't make it less crappy. I do realize we cannot remove the
> > interface but we can remove issues with the implementation and I dare to
> > say that most existing users wouldn't really notice.
>
> Well, at least powerpc-utils (why this interface was introduced) will
> notice a) performance wise and b) because more logging output will be
> generated (obviously non-offlineable blocks will be tried to offline).
I would really appreciate some specific example for a real usecase. I am
not familiar with powerpc-utils worklflows myself.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-01-22 10:45 UTC (permalink / raw)
To: Alexei Starovoitov, Stephen Smalley
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Stephane Eranian, james.bottomley@hansenpartnership.com,
Paul Mackerras, Jiri Olsa, Andi Kleen, Igor Lubashev,
James Morris, Alexander Shishkin, Ingo Molnar, oprofile-list,
Serge Hallyn, Robert Richter, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, jani.nikula@linux.intel.com,
Arnaldo Carvalho de Melo, rodrigo.vivi@intel.com, Namhyung Kim,
Thomas Gleixner, linux-arm-kernel, linux-parisc@vger.kernel.org,
linux-kernel, Lionel Landwerlin, Andy Lutomirski,
linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <537bdb28-c9e4-f44f-d665-25250065a6bb@linux.intel.com>
On 21.01.2020 21:27, Alexey Budankov wrote:
>
> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>> <alexey.budankov@linux.intel.com> wrote:
>>>
>>>
>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>
>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>> monitoring and observability operations so that CAP_PERFMON would assist
>>>>> CAP_SYS_ADMIN capability in its governing role for perf_events, i915_perf
>>>>> and other performance monitoring and observability subsystems.
>>>>>
>>>>> CAP_PERFMON intends to harden system security and integrity during system
>>>>> performance monitoring and observability operations by decreasing attack
>>>>> surface that is available to a CAP_SYS_ADMIN privileged process [1].
>>>>> Providing access to system performance monitoring and observability
>>>>> operations under CAP_PERFMON capability singly, without the rest of
>>>>> CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials and
>>>>> makes operation more secure.
>>>>>
>>>>> CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
>>>>> system performance monitoring and observability operations and balance
>>>>> amount of CAP_SYS_ADMIN credentials following the recommendations in the
>>>>> capabilities man page [1] for CAP_SYS_ADMIN: "Note: this capability is
>>>>> overloaded; see Notes to kernel developers, below."
>>>>>
>>>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>>>> of related hardware issues, the software can still mitigate these issues
>>>>> following the official embargoed hardware issues mitigation procedure [2].
>>>>> The bugs in the software itself could be fixed following the standard
>>>>> kernel development process [3] to maintain and harden security of system
>>>>> performance monitoring and observability operations.
>>>>>
>>>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>>>>
>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>> ---
>>>>> include/linux/capability.h | 12 ++++++++++++
>>>>> include/uapi/linux/capability.h | 8 +++++++-
>>>>> security/selinux/include/classmap.h | 4 ++--
>>>>> 3 files changed, 21 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>>>>> index ecce0f43c73a..8784969d91e1 100644
>>>>> --- a/include/linux/capability.h
>>>>> +++ b/include/linux/capability.h
>>>>> @@ -251,6 +251,18 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
>>>>> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
>>>>> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
>>>>> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
>>>>> +static inline bool perfmon_capable(void)
>>>>> +{
>>>>> + struct user_namespace *ns = &init_user_ns;
>>>>> +
>>>>> + if (ns_capable_noaudit(ns, CAP_PERFMON))
>>>>> + return ns_capable(ns, CAP_PERFMON);
>>>>> +
>>>>> + if (ns_capable_noaudit(ns, CAP_SYS_ADMIN))
>>>>> + return ns_capable(ns, CAP_SYS_ADMIN);
>>>>> +
>>>>> + return false;
>>>>> +}
>>>>
>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
So far so good, I suggest using the simplest version for v6:
static inline bool perfmon_capable(void)
{
return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
}
It keeps the implementation simple and readable. The implementation is more
performant in the sense of calling the API - one capable() call for CAP_PERFMON
privileged process.
Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
but this bloating also advertises and leverages using more secure CAP_PERFMON
based approach to use perf_event_open system call.
~Alexey
>>>
>>> Some of ideas from v4 review.
>>
>> well, in the requested changes form v4 I wrote:
>> return capable(CAP_PERFMON);
>> instead of
>> return false;
>
> Aww, indeed. I was concerning exactly about it when updating the patch
> and simply put false, missing the fact that capable() also logs.
>
> I suppose the idea is originally from here [1].
> BTW, Has it already seen any _more optimal_ implementation?
> Anyway, original or optimized version could be reused for CAP_PERFMON.
>
> ~Alexey
>
> [1] https://patchwork.ozlabs.org/patch/1159243/
>
>>
>> That's what Andy suggested earlier for CAP_BPF.
>> I think that should resolve Stephen's concern.
>>
^ permalink raw reply
* Re: [PATCH RFC v1] mm: is_mem_section_removable() overhaul
From: David Hildenbrand @ 2020-01-22 10:54 UTC (permalink / raw)
To: Michal Hocko
Cc: Nathan Lynch, Stephen Rothwell, Thomas Gleixner,
Anshuman Khandual, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List, Linux MM, Paul Mackerras,
Nathan Fontenot, Leonardo Bras, Dan Williams, linuxppc-dev,
Andrew Morton, Allison Randal, lantianyu1986
In-Reply-To: <20200122104230.GU29276@dhcp22.suse.cz>
On 22.01.20 11:42, Michal Hocko wrote:
> On Wed 22-01-20 11:39:08, David Hildenbrand wrote:
>>>>> Really, the interface is flawed and should have never been merged in the
>>>>> first place. We cannot simply remove it altogether I am afraid so let's
>>>>> at least remove the bogus code and pretend that the world is a better
>>>>> place where everything is removable except the reality sucks...
>>>>
>>>> As I expressed already, the interface works as designed/documented and
>>>> has been used like that for years.
>>>
>>> It seems we do differ in the usefulness though. Using a crappy interface
>>> for years doesn't make it less crappy. I do realize we cannot remove the
>>> interface but we can remove issues with the implementation and I dare to
>>> say that most existing users wouldn't really notice.
>>
>> Well, at least powerpc-utils (why this interface was introduced) will
>> notice a) performance wise and b) because more logging output will be
>> generated (obviously non-offlineable blocks will be tried to offline).
>
> I would really appreciate some specific example for a real usecase. I am
> not familiar with powerpc-utils worklflows myself.
>
Not an expert myself:
https://github.com/ibm-power-utilities/powerpc-utils
-> src/drmgr/drslot_chrp_mem.c
On request to remove some memory it will
a) Read "->removable" of all memory blocks ("lmb")
b) Check if the request can be fulfilled using the removable blocks
c) Try to offline the memory blocks by trying to offline it. If that
succeeded, trigger removeal of it using some hypervisor hooks.
Interestingly, with "AMS ballooning", it will already consider the
"removable" information useless (most probably, because of
non-migratable balloon pages that can be offlined - I assume the powerpc
code that I converted to proper balloon compaction just recently). a)
and b) is skipped.
Returning "yes" on all blocks will make them handle it just like if "AMS
ballooning" is active. So any memory block will be tried. Should work
but will be slower if no ballooning is active.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v5 07/10] powerpc/perf: open access for CAP_PERFMON privileged process
From: Anju T Sudhakar @ 2020-01-22 11:02 UTC (permalink / raw)
To: Alexey Budankov
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Stephane Eranian, james.bottomley@hansenpartnership.com,
Paul Mackerras, Jiri Olsa, Andi Kleen, Igor Lubashev,
James Morris, Alexander Shishkin, Ingo Molnar, oprofile-list,
Serge Hallyn, Robert Richter, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, jani.nikula@linux.intel.com,
Arnaldo Carvalho de Melo, rodrigo.vivi@intel.com, Namhyung Kim,
Thomas Gleixner, linux-arm-kernel, linux-parisc@vger.kernel.org,
linux-kernel, Lionel Landwerlin, linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <b74a3983-8e41-aba7-c18d-b16eff6fd5e5@linux.intel.com>
On 1/20/20 5:00 PM, Alexey Budankov wrote:
> Open access to monitoring for CAP_PERFMON privileged processes.
> For backward compatibility reasons access to the monitoring remains
> open for CAP_SYS_ADMIN privileged processes but CAP_SYS_ADMIN usage
> for secure monitoring is discouraged with respect to CAP_PERFMON
> capability. Providing the access under CAP_PERFMON capability singly,
> without the rest of CAP_SYS_ADMIN credentials, excludes chances to
> misuse the credentials and makes the operations more secure.
>
> Signed-off-by: Alexey Budankov<alexey.budankov@linux.intel.com>
> ---
Acked-by: Anju T Sudhakar<anju@linux.vnet.ibm.com>
^ permalink raw reply
* Re: [PATCH RFC v1] mm: is_mem_section_removable() overhaul
From: David Hildenbrand @ 2020-01-22 11:58 UTC (permalink / raw)
To: Michal Hocko
Cc: Nathan Lynch, Stephen Rothwell, Thomas Gleixner,
Anshuman Khandual, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List, Linux MM, Paul Mackerras,
Nathan Fontenot, Leonardo Bras, Dan Williams, linuxppc-dev,
Andrew Morton, Allison Randal, lantianyu1986
In-Reply-To: <98b6c208-b4dd-9052-43f6-543068c649cc@redhat.com>
On 22.01.20 11:54, David Hildenbrand wrote:
> On 22.01.20 11:42, Michal Hocko wrote:
>> On Wed 22-01-20 11:39:08, David Hildenbrand wrote:
>>>>>> Really, the interface is flawed and should have never been merged in the
>>>>>> first place. We cannot simply remove it altogether I am afraid so let's
>>>>>> at least remove the bogus code and pretend that the world is a better
>>>>>> place where everything is removable except the reality sucks...
>>>>>
>>>>> As I expressed already, the interface works as designed/documented and
>>>>> has been used like that for years.
>>>>
>>>> It seems we do differ in the usefulness though. Using a crappy interface
>>>> for years doesn't make it less crappy. I do realize we cannot remove the
>>>> interface but we can remove issues with the implementation and I dare to
>>>> say that most existing users wouldn't really notice.
>>>
>>> Well, at least powerpc-utils (why this interface was introduced) will
>>> notice a) performance wise and b) because more logging output will be
>>> generated (obviously non-offlineable blocks will be tried to offline).
>>
>> I would really appreciate some specific example for a real usecase. I am
>> not familiar with powerpc-utils worklflows myself.
>>
>
> Not an expert myself:
>
> https://github.com/ibm-power-utilities/powerpc-utils
>
> -> src/drmgr/drslot_chrp_mem.c
>
> On request to remove some memory it will
>
> a) Read "->removable" of all memory blocks ("lmb")
> b) Check if the request can be fulfilled using the removable blocks
> c) Try to offline the memory blocks by trying to offline it. If that
> succeeded, trigger removeal of it using some hypervisor hooks.
>
> Interestingly, with "AMS ballooning", it will already consider the
> "removable" information useless (most probably, because of
> non-migratable balloon pages that can be offlined - I assume the powerpc
> code that I converted to proper balloon compaction just recently). a)
> and b) is skipped.
>
> Returning "yes" on all blocks will make them handle it just like if "AMS
> ballooning" is active. So any memory block will be tried. Should work
> but will be slower if no ballooning is active.
>
On lsmem:
https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lgdd/lgdd_r_lsmem_cmd.html
"
Removable
yes if the memory range can be set offline, no if it cannot be set
offline. A dash (-) means that the range is already offline. The kernel
method that identifies removable memory ranges is heuristic and not
exact. Occasionally, memory ranges are falsely reported as removable or
falsely reported as not removable.
"
Usage of lsmem paird with chmem:
https://access.redhat.com/solutions/3937181
Especially interesting for IBM z Systems, whereby memory
onlining/offlining will trigger the actual population of memory in the
hypervisor. So if an admin wants to offline some memory (to give it back
to the hypervisor), it would use lsmem to identify such blocks first,
instead of trying random blocks until one offlining request succeeds.
E.g., documented in
https://books.google.de/books?id=1UEhDQAAQBAJ&pg=PA117&lpg=PA117&dq=lsmem+removable&source=bl&ots=OzMfU6Gbzu&sig=ACfU3U2IfH0eTVJs0qu50FdkysA3iC0elw&hl=de&sa=X&ved=2ahUKEwjQpdXQkpfnAhVOzqQKHTN4BsoQ6AEwBXoECAoQAQ#v=onepage&q=lsmem%20removable&f=false
So I still think that the interface is useful and is getting used in
real life. Users tolerate false positives/negatives.
--
Thanks,
David / dhildenb
^ permalink raw reply
* [PATCH v1 4/6] powerpc/32s: Drop NULL addr verification
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
NULL addr is a user address. Don't waste time checking it. If
someone tries to access it, it will SIGFAULT the same way as for
address 1, so no need to make it special.
The special case is when not doing a write, in that case we want
to drop the entire function. This is now handled by 'dir' param
and not by the nulity of 'to' anymore.
Also make beginning of prevent_user_access() similar
to beginning of allow_user_access(), and tell the compiler
that writing in kernel space or with a 0 length is unlikely
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/32/kup.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index d765515bd1c1..3c1798e56b55 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -113,7 +113,7 @@ static __always_inline void allow_user_access(void __user *to, const void __user
addr = (__force u32)to;
- if (!addr || addr >= TASK_SIZE || !size)
+ if (unlikely(addr >= TASK_SIZE || !size))
return;
end = min(addr + size, TASK_SIZE);
@@ -124,16 +124,18 @@ static __always_inline void allow_user_access(void __user *to, const void __user
static __always_inline void prevent_user_access(void __user *to, const void __user *from,
u32 size, unsigned long dir)
{
- u32 addr = (__force u32)to;
- u32 end = min(addr + size, TASK_SIZE);
+ u32 addr, end;
BUILD_BUG_ON(!__builtin_constant_p(dir));
if (!(dir & KUAP_W))
return;
- if (!addr || addr >= TASK_SIZE || !size)
+ addr = (__force u32)to;
+
+ if (unlikely(addr >= TASK_SIZE || !size))
return;
+ end = min(addr + size, TASK_SIZE);
current->thread.kuap = 0;
kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
}
--
2.25.0
^ permalink raw reply related
* [PATCH v1 2/6] powerpc/32s: Fix bad_kuap_fault()
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
At the moment, bad_kuap_fault() reports a fault only if a bad access
to userspace occurred while access to userspace was not granted.
But if a fault occurs for a write outside the allowed userspace
segment(s) that have been unlocked, bad_kuap_fault() fails to
detect it and the kernel loops forever in do_page_fault().
Fix it by checking that the accessed address is within the allowed
range.
Fixes: a68c31fc01ef ("powerpc/32s: Implement Kernel Userspace Access Protection")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/32/kup.h | 9 +++++++--
arch/powerpc/include/asm/book3s/64/kup-radix.h | 3 ++-
arch/powerpc/include/asm/nohash/32/kup-8xx.h | 3 ++-
arch/powerpc/mm/fault.c | 2 +-
4 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index f9dc597b0b86..d88008c8eb85 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -131,12 +131,17 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
+ unsigned long begin = regs->kuap & 0xf0000000;
+ unsigned long end = regs->kuap << 28;
+
if (!is_write)
return false;
- return WARN(!regs->kuap, "Bug: write fault blocked by segment registers !");
+ return WARN(address < begin || address >= end,
+ "Bug: write fault blocked by segment registers !");
}
#endif /* CONFIG_PPC_KUAP */
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index f254de956d6a..dbbd22cb80f5 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -95,7 +95,8 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
set_kuap(AMR_KUAP_BLOCKED);
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index 1006a427e99c..f2fea603b929 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -46,7 +46,8 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
mtspr(SPRN_MD_AP, MD_APG_KUAP);
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
return WARN(!((regs->kuap ^ MD_APG_KUAP) & 0xf0000000),
"Bug: fault blocked by AP register !");
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index b5047f9b5dec..1baeb045f7f4 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -233,7 +233,7 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
// Read/write fault in a valid region (the exception table search passed
// above), but blocked by KUAP is bad, it can never succeed.
- if (bad_kuap_fault(regs, is_write))
+ if (bad_kuap_fault(regs, address, is_write))
return true;
// What's left? Kernel fault on user in well defined regions (extable
--
2.25.0
^ permalink raw reply related
* [PATCH v1 3/6] powerpc/kuap: Fix set direction in allow/prevent_user_access()
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
__builtin_constant_p() always return 0 for pointers, so on RADIX
we always end up opening both direction (by writing 0 in SPR29):
0000000000000170 <._copy_to_user>:
...
1b0: 4c 00 01 2c isync
1b4: 39 20 00 00 li r9,0
1b8: 7d 3d 03 a6 mtspr 29,r9
1bc: 4c 00 01 2c isync
1c0: 48 00 00 01 bl 1c0 <._copy_to_user+0x50>
1c0: R_PPC64_REL24 .__copy_tofrom_user
...
0000000000000220 <._copy_from_user>:
...
2ac: 4c 00 01 2c isync
2b0: 39 20 00 00 li r9,0
2b4: 7d 3d 03 a6 mtspr 29,r9
2b8: 4c 00 01 2c isync
2bc: 7f c5 f3 78 mr r5,r30
2c0: 7f 83 e3 78 mr r3,r28
2c4: 48 00 00 01 bl 2c4 <._copy_from_user+0xa4>
2c4: R_PPC64_REL24 .__copy_tofrom_user
...
Use an explicit parameter for direction selection, so that GCC
is able to see it is a constant:
00000000000001b0 <._copy_to_user>:
...
1f0: 4c 00 01 2c isync
1f4: 3d 20 40 00 lis r9,16384
1f8: 79 29 07 c6 rldicr r9,r9,32,31
1fc: 7d 3d 03 a6 mtspr 29,r9
200: 4c 00 01 2c isync
204: 48 00 00 01 bl 204 <._copy_to_user+0x54>
204: R_PPC64_REL24 .__copy_tofrom_user
...
0000000000000260 <._copy_from_user>:
...
2ec: 4c 00 01 2c isync
2f0: 39 20 ff ff li r9,-1
2f4: 79 29 00 04 rldicr r9,r9,0,0
2f8: 7d 3d 03 a6 mtspr 29,r9
2fc: 4c 00 01 2c isync
300: 7f c5 f3 78 mr r5,r30
304: 7f 83 e3 78 mr r3,r28
308: 48 00 00 01 bl 308 <._copy_from_user+0xa8>
308: R_PPC64_REL24 .__copy_tofrom_user
...
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/32/kup.h | 13 ++++++--
.../powerpc/include/asm/book3s/64/kup-radix.h | 11 +++----
arch/powerpc/include/asm/kup.h | 30 ++++++++++++++-----
arch/powerpc/include/asm/nohash/32/kup-8xx.h | 4 +--
arch/powerpc/include/asm/uaccess.h | 4 +--
5 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index d88008c8eb85..d765515bd1c1 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -102,11 +102,13 @@ static inline void kuap_update_sr(u32 sr, u32 addr, u32 end)
isync(); /* Context sync required after mtsrin() */
}
-static inline void allow_user_access(void __user *to, const void __user *from, u32 size)
+static __always_inline void allow_user_access(void __user *to, const void __user *from,
+ u32 size, unsigned long dir)
{
u32 addr, end;
- if (__builtin_constant_p(to) && to == NULL)
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (!(dir & KUAP_W))
return;
addr = (__force u32)to;
@@ -119,11 +121,16 @@ static inline void allow_user_access(void __user *to, const void __user *from, u
kuap_update_sr(mfsrin(addr) & ~SR_KS, addr, end); /* Clear Ks */
}
-static inline void prevent_user_access(void __user *to, const void __user *from, u32 size)
+static __always_inline void prevent_user_access(void __user *to, const void __user *from,
+ u32 size, unsigned long dir)
{
u32 addr = (__force u32)to;
u32 end = min(addr + size, TASK_SIZE);
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (!(dir & KUAP_W))
+ return;
+
if (!addr || addr >= TASK_SIZE || !size)
return;
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index dbbd22cb80f5..f11315306d41 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -77,20 +77,21 @@ static inline void set_kuap(unsigned long value)
isync();
}
-static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size)
+static __always_inline void allow_user_access(void __user *to, const void __user *from,
+ unsigned long size, unsigned long dir)
{
// This is written so we can resolve to a single case at build time
- if (__builtin_constant_p(to) && to == NULL)
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (dir == KUAP_R)
set_kuap(AMR_KUAP_BLOCK_WRITE);
- else if (__builtin_constant_p(from) && from == NULL)
+ else if (dir == KUAP_W)
set_kuap(AMR_KUAP_BLOCK_READ);
else
set_kuap(0);
}
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
set_kuap(AMR_KUAP_BLOCKED);
}
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 5b5e39643a27..49fd977a498e 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -2,6 +2,10 @@
#ifndef _ASM_POWERPC_KUP_H_
#define _ASM_POWERPC_KUP_H_
+#define KUAP_R 1
+#define KUAP_W 2
+#define KUAP_RW (KUAP_R | KUAP_W)
+
#ifdef CONFIG_PPC64
#include <asm/book3s/64/kup-radix.h>
#endif
@@ -42,32 +46,44 @@ void setup_kuap(bool disabled);
#else
static inline void setup_kuap(bool disabled) { }
static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size) { }
+ unsigned long size, unsigned long dir) { }
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size) { }
+ unsigned long size, unsigned long dir) { }
static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
#endif /* CONFIG_PPC_KUAP */
static inline void allow_read_from_user(const void __user *from, unsigned long size)
{
- allow_user_access(NULL, from, size);
+ allow_user_access(NULL, from, size, KUAP_R);
}
static inline void allow_write_to_user(void __user *to, unsigned long size)
{
- allow_user_access(to, NULL, size);
+ allow_user_access(to, NULL, size, KUAP_W);
+}
+
+static inline void allow_read_write_user(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ allow_user_access(to, from, size, KUAP_RW);
}
static inline void prevent_read_from_user(const void __user *from, unsigned long size)
{
- prevent_user_access(NULL, from, size);
+ prevent_user_access(NULL, from, size, KUAP_R);
}
static inline void prevent_write_to_user(void __user *to, unsigned long size)
{
- prevent_user_access(to, NULL, size);
+ prevent_user_access(to, NULL, size, KUAP_W);
+}
+
+static inline void prevent_read_write_user(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ prevent_user_access(to, from, size, KUAP_RW);
}
#endif /* !__ASSEMBLY__ */
-#endif /* _ASM_POWERPC_KUP_H_ */
+#endif /* _ASM_POWERPC_KUAP_H_ */
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index f2fea603b929..1d70c80366fd 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -35,13 +35,13 @@
#include <asm/reg.h>
static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
mtspr(SPRN_MD_AP, MD_APG_INIT);
}
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
mtspr(SPRN_MD_AP, MD_APG_KUAP);
}
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index c92fe7fe9692..cafad1960e76 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -313,9 +313,9 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
unsigned long ret;
barrier_nospec();
- allow_user_access(to, from, n);
+ allow_read_write_user(to, from, n);
ret = __copy_tofrom_user(to, from, n);
- prevent_user_access(to, from, n);
+ prevent_read_write_user(to, from, n);
return ret;
}
#endif /* __powerpc64__ */
--
2.25.0
^ permalink raw reply related
* [PATCH v1 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linus Torvalds, Alexander Viro, Andrew Morton
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
Some architectures grand full access to userspace regardless of the
address/len passed to user_access_begin(), but other architectures
only grand access to the requested area.
For exemple, on 32 bits powerpc (book3s/32), access is granted by
segments of 256 Mbytes.
Modify filldir() and filldir64() to request the real area they need
to get access to.
Fixes: 9f79b78ef744 ("Convert filldir[64]() from __put_user() to unsafe_put_user()")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
fs/readdir.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/fs/readdir.c b/fs/readdir.c
index d26d5ea4de7b..ef04e5e76c59 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -236,15 +236,11 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
if (dirent && signal_pending(current))
return -EINTR;
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
+ if (dirent && unlikely(put_user(offset, &dirent->d_off)))
goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ if (!user_access_begin(dirent, reclen))
+ goto efault;
unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
@@ -323,15 +319,11 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
if (dirent && signal_pending(current))
return -EINTR;
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
+ if (dirent && unlikely(put_user(offset, &dirent->d_off)))
goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ if (!user_access_begin(dirent, reclen))
+ goto efault;
unsafe_put_user(ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, &dirent->d_type, efault_end);
--
2.25.0
^ permalink raw reply related
* [PATCH v1 5/6] powerpc/32s: prepare prevent_user_access() for user_access_end()
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
In preparation of implementing user_access_begin and friends
on powerpc, the book3s/32 version of prevent_user_access() need
to be prepared for user_access_end().
user_access_end() doesn't provide the address and size which
were passed to user_access_begin(), required by prevent_user_access()
to know which segment to modify.
The list of segments which where unprotected by allow_user_access()
are available in current->kuap. But we don't want prevent_user_access()
to read this all the time, especially everytime it is 0 (for instance
because the access was not a write access).
Implement a special direction case named KUAP_SELF. In this case only,
the addr and end are retrieved from current->kuap.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/book3s/32/kup.h | 25 ++++++++++++++++++------
arch/powerpc/include/asm/kup.h | 1 +
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 3c1798e56b55..a99fc3428ac9 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -117,6 +117,7 @@ static __always_inline void allow_user_access(void __user *to, const void __user
return;
end = min(addr + size, TASK_SIZE);
+
current->thread.kuap = (addr & 0xf0000000) | ((((end - 1) >> 28) + 1) & 0xf);
kuap_update_sr(mfsrin(addr) & ~SR_KS, addr, end); /* Clear Ks */
}
@@ -127,15 +128,27 @@ static __always_inline void prevent_user_access(void __user *to, const void __us
u32 addr, end;
BUILD_BUG_ON(!__builtin_constant_p(dir));
- if (!(dir & KUAP_W))
- return;
- addr = (__force u32)to;
+ if (dir == KUAP_SELF) {
+ u32 kuap = current->thread.kuap;
- if (unlikely(addr >= TASK_SIZE || !size))
- return;
+ if (unlikely(!kuap))
+ return;
+
+ addr = kuap & 0xf0000000;
+ end = kuap << 28;
+ } else {
+ if (!(dir & KUAP_W))
+ return;
+
+ addr = (__force u32)to;
+
+ if (unlikely(addr >= TASK_SIZE || !size))
+ return;
+
+ end = min(addr + size, TASK_SIZE);
+ }
- end = min(addr + size, TASK_SIZE);
current->thread.kuap = 0;
kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
}
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 49fd977a498e..0676df6f3f86 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -5,6 +5,7 @@
#define KUAP_R 1
#define KUAP_W 2
#define KUAP_RW (KUAP_R | KUAP_W)
+#define KUAP_SELF 4
#ifdef CONFIG_PPC64
#include <asm/book3s/64/kup-radix.h>
--
2.25.0
^ permalink raw reply related
* [PATCH v1 6/6] powerpc: Implement user_access_begin and friends
From: Christophe Leroy @ 2020-01-22 13:00 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
Today, when a function like strncpy_from_user() is called,
the userspace access protection is de-activated and re-activated
for every word read.
By implementing user_access_begin and friends, the protection
is de-activated at the beginning of the copy and re-activated at the
end.
Implement user_access_begin(), user_access_end() and
unsafe_get_user(), unsafe_put_user() and unsafe_copy_to_user()
For the time being, we keep user_access_save() and
user_access_restore() as nops.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/uaccess.h | 92 ++++++++++++++++++++++++++----
1 file changed, 82 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index cafad1960e76..ea67bbd56bd4 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -91,9 +91,14 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
__put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
#define __get_user(x, ptr) \
- __get_user_nocheck((x), (ptr), sizeof(*(ptr)))
+ __get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
#define __put_user(x, ptr) \
- __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
+ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), true)
+
+#define __get_user_allowed(x, ptr) \
+ __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
+#define __put_user_allowed(x, ptr) \
+ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), false)
#define __get_user_inatomic(x, ptr) \
__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
@@ -138,10 +143,9 @@ extern long __put_user_bad(void);
: "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
-#define __put_user_size(x, ptr, size, retval) \
+#define __put_user_size_allowed(x, ptr, size, retval) \
do { \
retval = 0; \
- allow_write_to_user(ptr, size); \
switch (size) { \
case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
@@ -149,17 +153,26 @@ do { \
case 8: __put_user_asm2(x, ptr, retval); break; \
default: __put_user_bad(); \
} \
+} while (0)
+
+#define __put_user_size(x, ptr, size, retval) \
+do { \
+ allow_write_to_user(ptr, size); \
+ __put_user_size_allowed(x, ptr, size, retval); \
prevent_write_to_user(ptr, size); \
} while (0)
-#define __put_user_nocheck(x, ptr, size) \
+#define __put_user_nocheck(x, ptr, size, allow) \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
if (!is_kernel_addr((unsigned long)__pu_addr)) \
might_fault(); \
__chk_user_ptr(ptr); \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ if (allow) \
+ __put_user_size((x), __pu_addr, (size), __pu_err); \
+ else \
+ __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
__pu_err; \
})
@@ -236,13 +249,12 @@ extern long __get_user_bad(void);
: "b" (addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
-#define __get_user_size(x, ptr, size, retval) \
+#define __get_user_size_allowed(x, ptr, size, retval) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
- allow_read_from_user(ptr, size); \
switch (size) { \
case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
@@ -250,6 +262,12 @@ do { \
case 8: __get_user_asm2(x, ptr, retval); break; \
default: (x) = __get_user_bad(); \
} \
+} while (0)
+
+#define __get_user_size(x, ptr, size, retval) \
+do { \
+ allow_read_from_user(ptr, size); \
+ __get_user_size_allowed(x, ptr, size, retval); \
prevent_read_from_user(ptr, size); \
} while (0)
@@ -260,7 +278,7 @@ do { \
#define __long_type(x) \
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
-#define __get_user_nocheck(x, ptr, size) \
+#define __get_user_nocheck(x, ptr, size, allow) \
({ \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
@@ -269,7 +287,10 @@ do { \
if (!is_kernel_addr((unsigned long)__gu_addr)) \
might_fault(); \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ if (allow) \
+ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ else \
+ __get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
__gu_err; \
})
@@ -387,6 +408,34 @@ static inline unsigned long raw_copy_to_user(void __user *to,
return ret;
}
+static inline unsigned long
+raw_copy_to_user_allowed(void __user *to, const void *from, unsigned long n)
+{
+ unsigned long ret;
+ if (__builtin_constant_p(n) && (n) <= 8) {
+ ret = 1;
+
+ switch (n) {
+ case 1:
+ __put_user_size_allowed(*(u8 *)from, (u8 __user *)to, 1, ret);
+ break;
+ case 2:
+ __put_user_size_allowed(*(u16 *)from, (u16 __user *)to, 2, ret);
+ break;
+ case 4:
+ __put_user_size_allowed(*(u32 *)from, (u32 __user *)to, 4, ret);
+ break;
+ case 8:
+ __put_user_size_allowed(*(u64 *)from, (u64 __user *)to, 8, ret);
+ break;
+ }
+ if (ret == 0)
+ return 0;
+ }
+
+ return __copy_tofrom_user(to, (__force const void __user *)from, n);
+}
+
static __always_inline unsigned long __must_check
copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
{
@@ -428,4 +477,27 @@ extern long __copy_from_user_flushcache(void *dst, const void __user *src,
extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
size_t len);
+static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
+{
+ if (unlikely(!access_ok(ptr, len)))
+ return false;
+ allow_read_write_user((void __user *)ptr, ptr, len);
+ return true;
+}
+#define user_access_begin user_access_begin
+
+static inline void user_access_end(void)
+{
+ prevent_user_access(NULL, NULL, ~0UL, KUAP_SELF);
+}
+#define user_access_end user_access_end
+
+static inline unsigned long user_access_save(void) { return 0UL; }
+static inline void user_access_restore(unsigned long flags) { }
+
+#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
+#define unsafe_get_user(x,p,e) unsafe_op_wrap(__get_user_allowed(x,p),e)
+#define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user_allowed(x,p),e)
+#define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(raw_copy_to_user_allowed(d,s,l),e)
+
#endif /* _ARCH_POWERPC_UACCESS_H */
--
2.25.0
^ permalink raw reply related
* Re: GCC bug ? Re: [PATCH v2 10/10] powerpc/32s: Implement Kernel Userspace Access Protection
From: Segher Boessenkool @ 2020-01-22 13:18 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <8501a33e-6c76-b6bd-9d8e-985313f94579@c-s.fr>
Hi!
On Wed, Jan 22, 2020 at 07:57:15AM +0100, Christophe Leroy wrote:
> GCC doc also says:
>
> "if you use it in an inlined function and pass an argument of the
> function as the argument to the built-in, GCC never returns 1 when you
> call the inline function with a string constant"
>
> Does GCC considers (void*)0 as a string constant ?
No, because it isn't (it's a pointer, not an array of characters).
Segher
^ permalink raw reply
* Re: GCC bug ? Re: [PATCH v2 10/10] powerpc/32s: Implement Kernel Userspace Access Protection
From: Segher Boessenkool @ 2020-01-22 13:36 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <af9ad296-401c-cb5c-868a-7a6f91d1e8bc@c-s.fr>
On Wed, Jan 22, 2020 at 07:52:02AM +0100, Christophe Leroy wrote:
> Le 21/01/2020 à 20:55, Segher Boessenkool a écrit :
> >On Tue, Jan 21, 2020 at 05:22:32PM +0000, Christophe Leroy wrote:
> >>g1() should return 3, not 5.
> >
> >What makes you say that?
>
> What makes me say that is that NULL is obviously a constant pointer and
> I think we are all expecting gcc to see it as a constant during kernel
> build, ie at -O2
But apparently at the point where the builtin was checked it did not
yet know it is passed a null pointer.
Please make a self-contained test case if we need further investigation?
> >"A return of 0 does not indicate that the
> > value is _not_ a constant, but merely that GCC cannot prove it is a
> > constant with the specified value of the '-O' option."
> >
> >(And the rules it uses for this are *not* the same as C "constant
> >expressions" or C "integer constant expression" or C "arithmetic
> >constant expression" or anything like that -- which should be already
> >obvious from that it changes with different -Ox).
> >
> >You can use builtin_constant_p to have the compiler do something better
> >if the compiler feels like it, but not anything more. Often people
> >want stronger guarantees, but when they see how much less often it then
> >returns "true", they do not want that either.
> If GCC doesn't see NULL as a constant, then the above doesn't work as
> expected.
That's not the question. Of course GCC sees it as a null pointer
constant, because it is one. But this builtin does its work very
early, during preprocessing already. Its concept of "constant" is
very different.
Does it work if you write just "0" instead of "NULL", btw? "0" is
also a null pointer constant eventually (here, that is).
The question is why (and if, it still needs verification after all)
builtin_constant_p didn't return true.
Segher
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-01-22 14:07 UTC (permalink / raw)
To: Alexey Budankov, Alexei Starovoitov
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Stephane Eranian, james.bottomley@hansenpartnership.com,
Paul Mackerras, Jiri Olsa, Andi Kleen, Igor Lubashev,
James Morris, Alexander Shishkin, Ingo Molnar, oprofile-list,
Serge Hallyn, Robert Richter, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, jani.nikula@linux.intel.com,
Arnaldo Carvalho de Melo, rodrigo.vivi@intel.com, Namhyung Kim,
Thomas Gleixner, linux-arm-kernel, linux-parisc@vger.kernel.org,
linux-kernel, Lionel Landwerlin, Andy Lutomirski,
linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <63d9700f-231d-7973-5307-3e56a48c54cb@linux.intel.com>
On 1/22/20 5:45 AM, Alexey Budankov wrote:
>
> On 21.01.2020 21:27, Alexey Budankov wrote:
>>
>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>> <alexey.budankov@linux.intel.com> wrote:
>>>>
>>>>
>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>
>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>> monitoring and observability operations so that CAP_PERFMON would assist
>>>>>> CAP_SYS_ADMIN capability in its governing role for perf_events, i915_perf
>>>>>> and other performance monitoring and observability subsystems.
>>>>>>
>>>>>> CAP_PERFMON intends to harden system security and integrity during system
>>>>>> performance monitoring and observability operations by decreasing attack
>>>>>> surface that is available to a CAP_SYS_ADMIN privileged process [1].
>>>>>> Providing access to system performance monitoring and observability
>>>>>> operations under CAP_PERFMON capability singly, without the rest of
>>>>>> CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials and
>>>>>> makes operation more secure.
>>>>>>
>>>>>> CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
>>>>>> system performance monitoring and observability operations and balance
>>>>>> amount of CAP_SYS_ADMIN credentials following the recommendations in the
>>>>>> capabilities man page [1] for CAP_SYS_ADMIN: "Note: this capability is
>>>>>> overloaded; see Notes to kernel developers, below."
>>>>>>
>>>>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>>>>> of related hardware issues, the software can still mitigate these issues
>>>>>> following the official embargoed hardware issues mitigation procedure [2].
>>>>>> The bugs in the software itself could be fixed following the standard
>>>>>> kernel development process [3] to maintain and harden security of system
>>>>>> performance monitoring and observability operations.
>>>>>>
>>>>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>>>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>>>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>>>>>
>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>> ---
>>>>>> include/linux/capability.h | 12 ++++++++++++
>>>>>> include/uapi/linux/capability.h | 8 +++++++-
>>>>>> security/selinux/include/classmap.h | 4 ++--
>>>>>> 3 files changed, 21 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>>>>>> index ecce0f43c73a..8784969d91e1 100644
>>>>>> --- a/include/linux/capability.h
>>>>>> +++ b/include/linux/capability.h
>>>>>> @@ -251,6 +251,18 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
>>>>>> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
>>>>>> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
>>>>>> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
>>>>>> +static inline bool perfmon_capable(void)
>>>>>> +{
>>>>>> + struct user_namespace *ns = &init_user_ns;
>>>>>> +
>>>>>> + if (ns_capable_noaudit(ns, CAP_PERFMON))
>>>>>> + return ns_capable(ns, CAP_PERFMON);
>>>>>> +
>>>>>> + if (ns_capable_noaudit(ns, CAP_SYS_ADMIN))
>>>>>> + return ns_capable(ns, CAP_SYS_ADMIN);
>>>>>> +
>>>>>> + return false;
>>>>>> +}
>>>>>
>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>
> So far so good, I suggest using the simplest version for v6:
>
> static inline bool perfmon_capable(void)
> {
> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
> }
>
> It keeps the implementation simple and readable. The implementation is more
> performant in the sense of calling the API - one capable() call for CAP_PERFMON
> privileged process.
>
> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
> but this bloating also advertises and leverages using more secure CAP_PERFMON
> based approach to use perf_event_open system call.
I can live with that. We just need to document that when you see both a
CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only
allowing CAP_PERFMON first and see if that resolves the issue. We have
a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-01-22 14:25 UTC (permalink / raw)
To: Stephen Smalley, Alexei Starovoitov
Cc: Mark Rutland, Song Liu, Peter Zijlstra,
joonas.lahtinen@linux.intel.com, Will Deacon, Alexei Starovoitov,
Stephane Eranian, james.bottomley@hansenpartnership.com,
Paul Mackerras, Jiri Olsa, Andi Kleen, Igor Lubashev,
James Morris, Alexander Shishkin, Ingo Molnar, oprofile-list,
Serge Hallyn, Robert Richter, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, jani.nikula@linux.intel.com,
Arnaldo Carvalho de Melo, rodrigo.vivi@intel.com, Namhyung Kim,
Thomas Gleixner, linux-arm-kernel, linux-parisc@vger.kernel.org,
linux-kernel, Lionel Landwerlin, Andy Lutomirski,
linux-perf-users@vger.kernel.org,
linux-security-module@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <d7213569-9578-7201-6106-f5ebc95bd6be@tycho.nsa.gov>
On 22.01.2020 17:07, Stephen Smalley wrote:
> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>
>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>
>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>
>>>>>
>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>
>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>> monitoring and observability operations so that CAP_PERFMON would assist
>>>>>>> CAP_SYS_ADMIN capability in its governing role for perf_events, i915_perf
>>>>>>> and other performance monitoring and observability subsystems.
>>>>>>>
>>>>>>> CAP_PERFMON intends to harden system security and integrity during system
>>>>>>> performance monitoring and observability operations by decreasing attack
>>>>>>> surface that is available to a CAP_SYS_ADMIN privileged process [1].
>>>>>>> Providing access to system performance monitoring and observability
>>>>>>> operations under CAP_PERFMON capability singly, without the rest of
>>>>>>> CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials and
>>>>>>> makes operation more secure.
>>>>>>>
>>>>>>> CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
>>>>>>> system performance monitoring and observability operations and balance
>>>>>>> amount of CAP_SYS_ADMIN credentials following the recommendations in the
>>>>>>> capabilities man page [1] for CAP_SYS_ADMIN: "Note: this capability is
>>>>>>> overloaded; see Notes to kernel developers, below."
>>>>>>>
>>>>>>> Although the software running under CAP_PERFMON can not ensure avoidance
>>>>>>> of related hardware issues, the software can still mitigate these issues
>>>>>>> following the official embargoed hardware issues mitigation procedure [2].
>>>>>>> The bugs in the software itself could be fixed following the standard
>>>>>>> kernel development process [3] to maintain and harden security of system
>>>>>>> performance monitoring and observability operations.
>>>>>>>
>>>>>>> [1] http://man7.org/linux/man-pages/man7/capabilities.7.html
>>>>>>> [2] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
>>>>>>> [3] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
>>>>>>>
>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>> ---
>>>>>>> include/linux/capability.h | 12 ++++++++++++
>>>>>>> include/uapi/linux/capability.h | 8 +++++++-
>>>>>>> security/selinux/include/classmap.h | 4 ++--
>>>>>>> 3 files changed, 21 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>>>>>>> index ecce0f43c73a..8784969d91e1 100644
>>>>>>> --- a/include/linux/capability.h
>>>>>>> +++ b/include/linux/capability.h
>>>>>>> @@ -251,6 +251,18 @@ extern bool privileged_wrt_inode_uidgid(struct user_namespace *ns, const struct
>>>>>>> extern bool capable_wrt_inode_uidgid(const struct inode *inode, int cap);
>>>>>>> extern bool file_ns_capable(const struct file *file, struct user_namespace *ns, int cap);
>>>>>>> extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
>>>>>>> +static inline bool perfmon_capable(void)
>>>>>>> +{
>>>>>>> + struct user_namespace *ns = &init_user_ns;
>>>>>>> +
>>>>>>> + if (ns_capable_noaudit(ns, CAP_PERFMON))
>>>>>>> + return ns_capable(ns, CAP_PERFMON);
>>>>>>> +
>>>>>>> + if (ns_capable_noaudit(ns, CAP_SYS_ADMIN))
>>>>>>> + return ns_capable(ns, CAP_SYS_ADMIN);
>>>>>>> +
>>>>>>> + return false;
>>>>>>> +}
>>>>>>
>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>
>> So far so good, I suggest using the simplest version for v6:
>>
>> static inline bool perfmon_capable(void)
>> {
>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>> }
>>
>> It keeps the implementation simple and readable. The implementation is more
>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>> privileged process.
>>
>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>> based approach to use perf_event_open system call.
>
> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
perf security [1] document can be updated, at least, to align and document
this audit logging specifics.
~Alexey
[1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
^ permalink raw reply
* Re: GCC bug ? Re: [PATCH v2 10/10] powerpc/32s: Implement Kernel Userspace Access Protection
From: Christophe Leroy @ 2020-01-22 14:45 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <20200122133626.GL3191@gate.crashing.org>
Le 22/01/2020 à 14:36, Segher Boessenkool a écrit :
> On Wed, Jan 22, 2020 at 07:52:02AM +0100, Christophe Leroy wrote:
>> Le 21/01/2020 à 20:55, Segher Boessenkool a écrit :
>>> On Tue, Jan 21, 2020 at 05:22:32PM +0000, Christophe Leroy wrote:
>>>> g1() should return 3, not 5.
>>>
>>> What makes you say that?
>>
>> What makes me say that is that NULL is obviously a constant pointer and
>> I think we are all expecting gcc to see it as a constant during kernel
>> build, ie at -O2
>
> But apparently at the point where the builtin was checked it did not
> yet know it is passed a null pointer.
>
> Please make a self-contained test case if we need further investigation?
The test in my original mail is self-contained:
#define NULL (void*)0
static inline int f1(void *to)
{
if (__builtin_constant_p(to) && to == NULL)
return 3;
return 5;
}
int g1(void)
{
return f1(NULL);
}
Build the above with -O2 then objdump:
00000000 <g1>:
0: 38 60 00 05 li r3,5
4: 4e 80 00 20 blr
It returns 5 so that shows __builtin_constant_p(to) was evaluated as false.
>
>>> "A return of 0 does not indicate that the
>>> value is _not_ a constant, but merely that GCC cannot prove it is a
>>> constant with the specified value of the '-O' option."
>>>
>>> (And the rules it uses for this are *not* the same as C "constant
>>> expressions" or C "integer constant expression" or C "arithmetic
>>> constant expression" or anything like that -- which should be already
>>> obvious from that it changes with different -Ox).
>>>
>>> You can use builtin_constant_p to have the compiler do something better
>>> if the compiler feels like it, but not anything more. Often people
>>> want stronger guarantees, but when they see how much less often it then
>>> returns "true", they do not want that either.
>
>> If GCC doesn't see NULL as a constant, then the above doesn't work as
>> expected.
>
> That's not the question. Of course GCC sees it as a null pointer
> constant, because it is one. But this builtin does its work very
> early, during preprocessing already. Its concept of "constant" is
> very different.
>
> Does it work if you write just "0" instead of "NULL", btw? "0" is
> also a null pointer constant eventually (here, that is).
No it doesn't.
It works if you change the 'void *to' to 'unsigned long to'
>
> The question is why (and if, it still needs verification after all)
> builtin_constant_p didn't return true.
I sent a patch to overcome the problem. See
https://patchwork.ozlabs.org/patch/1227249/
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc: drmem: avoid NULL pointer dereference when drmem is unavailable
From: Michal Suchánek @ 2020-01-22 15:21 UTC (permalink / raw)
To: Libor Pechacek
Cc: Nathan Lynch, David Hildenbrand, linux-kernel, stable,
Paul Mackerras, Leonardo Bras, Nathan Fontenot, Thomas Gleixner,
linuxppc-dev, Allison Randal
In-Reply-To: <20200116102758.GC25138@fm.suse.cz>
On Thu, Jan 16, 2020 at 11:27:58AM +0100, Libor Pechacek wrote:
> In KVM guests drmem structure is only zero initialized. Trying to
> manipulate DLPAR parameters results in a crash in this environment.
>
> $ echo "memory add count 1" > /sys/kernel/dlpar
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in: af_packet(E) rfkill(E) nvram(E) vmx_crypto(E)
> gf128mul(E) e1000(E) virtio_balloon(E) rtc_generic(E) crct10dif_vpmsum(E)
> btrfs(E) blake2b_generic(E) libcrc32c(E) xor(E) raid6_pq(E) virtio_rng(E)
> virtio_blk(E) ohci_pci(E) ehci_pci(E) ohci_hcd(E) ehci_hcd(E)
> crc32c_vpmsum(E) usbcore(E) virtio_pci(E) virtio_ring(E) virtio(E) sg(E)
> dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E)
> scsi_mod(E)
> CPU: 1 PID: 4114 Comm: bash Kdump: loaded Tainted: G E 5.5.0-rc6-2-default #1
> NIP: c0000000000ff294 LR: c0000000000ff248 CTR: 0000000000000000
> REGS: c0000000fb9d3880 TRAP: 0300 Tainted: G E (5.5.0-rc6-2-default)
> MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 28242428 XER: 20000000
> CFAR: c0000000009a6c10 DAR: 0000000000000010 DSISR: 40000000 IRQMASK: 0
> GPR00: c0000000000ff248 c0000000fb9d3b10 c000000001682e00 0000000000000033
> GPR04: c0000000ff30bf90 c0000000ff394800 0000000000005110 ffffffffffffffe8
> GPR08: 0000000000000000 0000000000000000 00000000fe1c0000 0000000000000000
> GPR12: 0000000000002200 c00000003fffee00 0000000000000000 000000011cbc37c0
> GPR16: 000000011cb27ed0 0000000000000000 000000011cb6dd10 0000000000000000
> GPR20: 000000011cb7db28 000001003ce035f0 000000011cbc7828 000000011cbc6c70
> GPR24: 000001003cf01210 0000000000000000 c0000000ffade4e0 c000000002d7216b
> GPR28: 0000000000000001 c000000002d78560 0000000000000000 c0000000015458d0
> NIP [c0000000000ff294] dlpar_memory+0x6e4/0xd00
> LR [c0000000000ff248] dlpar_memory+0x698/0xd00
> Call Trace:
> [c0000000fb9d3b10] [c0000000000ff248] dlpar_memory+0x698/0xd00 (unreliable)
> [c0000000fb9d3ba0] [c0000000000f5990] handle_dlpar_errorlog+0xc0/0x190
> [c0000000fb9d3c10] [c0000000000f5c58] dlpar_store+0x198/0x4a0
> [c0000000fb9d3cd0] [c000000000c4cb00] kobj_attr_store+0x30/0x50
> [c0000000fb9d3cf0] [c0000000005a37b4] sysfs_kf_write+0x64/0x90
> [c0000000fb9d3d10] [c0000000005a2c90] kernfs_fop_write+0x1b0/0x290
> [c0000000fb9d3d60] [c0000000004a2bec] __vfs_write+0x3c/0x70
> [c0000000fb9d3d80] [c0000000004a6560] vfs_write+0xd0/0x260
> [c0000000fb9d3dd0] [c0000000004a69ac] ksys_write+0xdc/0x130
> [c0000000fb9d3e20] [c00000000000b478] system_call+0x5c/0x68
> Instruction dump:
> ebc90000 1ce70018 38e7ffe8 7cfe3a14 7fbe3840 419dff14 fb610068 7fc9f378
> 39000000 4800000c 60000000 4195fef4 <81490010> 39290018 38c80001 7ea93840
> ---[ end trace cc2dd8152608c295 ]---
>
> Taking closer look at the code, I can see that for_each_drmem_lmb is a
> macro expanding into `for (lmb = &drmem_info->lmbs[0]; lmb <=
> &drmem_info->lmbs[drmem_info->n_lmbs - 1]; lmb++)`. When drmem_info->lmbs
> is NULL, the loop would iterate through the whole address range if it
> weren't stopped by the NULL pointer dereference on the next line.
>
> This patch aligns for_each_drmem_lmb and for_each_drmem_lmb_in_range macro
> behavior with the common C semantics, where the end marker does not belong
> to the scanned range, and alters get_lmb_range() semantics. As a side
> effect, the wraparound observed in the crash is prevented.
>
> Fixes: 6c6ea53725b3 ("powerpc/mm: Separate ibm, dynamic-memory data from DT format")
> Cc: Michal Suchanek <msuchanek@suse.cz>
> Cc: stable@vger.kernel.org
> Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Reviewed-by: Michal Suchanek <msuchanek@suse.de>
> ---
> arch/powerpc/include/asm/drmem.h | 4 ++--
> arch/powerpc/platforms/pseries/hotplug-memory.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index 3d76e1c388c2..28c3d936fdf3 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -27,12 +27,12 @@ struct drmem_lmb_info {
> extern struct drmem_lmb_info *drmem_info;
>
> #define for_each_drmem_lmb_in_range(lmb, start, end) \
> - for ((lmb) = (start); (lmb) <= (end); (lmb)++)
> + for ((lmb) = (start); (lmb) < (end); (lmb)++)
>
> #define for_each_drmem_lmb(lmb) \
> for_each_drmem_lmb_in_range((lmb), \
> &drmem_info->lmbs[0], \
> - &drmem_info->lmbs[drmem_info->n_lmbs - 1])
> + &drmem_info->lmbs[drmem_info->n_lmbs])
>
> /*
> * The of_drconf_cell_v1 struct defines the layout of the LMB data
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c126b94d1943..4ea6af002e27 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -236,9 +236,9 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
> if (!start)
> return -EINVAL;
>
> - end = &start[n_lmbs - 1];
> + end = &start[n_lmbs];
>
> - last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs - 1];
> + last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs];
> if (end > last_lmb)
> return -EINVAL;
>
> --
> 2.24.1
>
>
> --
> Libor Pechacek
> SUSE Labs Remember to have fun...
^ permalink raw reply
* [PATCH] powerpc/papr_scm: Fix leaking 'bus_desc.provider_name' in some paths
From: Vaibhav Jain @ 2020-01-22 15:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Vaibhav Jain, Oliver O'Halloran, Aneesh Kumar K . V
String 'bus_desc.provider_name' allocated inside
papr_scm_nvdimm_init() will leaks in case call to
nvdimm_bus_register() fails or when papr_scm_remove() is called.
This minor patch ensures that 'bus_desc.provider_name' is freed in
error path for nvdimm_bus_register() as well as in papr_scm_remove().
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/platforms/pseries/papr_scm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index c2ef320ba1bf..eb420655ed0b 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -322,6 +322,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
p->bus = nvdimm_bus_register(NULL, &p->bus_desc);
if (!p->bus) {
dev_err(dev, "Error creating nvdimm bus %pOF\n", p->dn);
+ kfree(p->bus_desc.provider_name);
return -ENXIO;
}
@@ -477,6 +478,7 @@ static int papr_scm_remove(struct platform_device *pdev)
nvdimm_bus_unregister(p->bus);
drc_pmem_unbind(p);
+ kfree(p->bus_desc.provider_name);
kfree(p);
return 0;
--
2.24.1
^ permalink raw reply related
* [PATCH] libnvdimm/bus: return the outvar 'cmd_rc' error code in __nd_ioctl()
From: Vaibhav Jain @ 2020-01-22 15:53 UTC (permalink / raw)
To: linux-nvdimm
Cc: Vaibhav Jain, dan.j.williams, linuxppc-dev, Aneesh Kumar K.V,
Vishal Verma
Presently the error code returned via out variable 'cmd_rc' from the
nvdimm-bus controller function is ignored when called from
__nd_ioctl() and never communicated back to user-space code that called
an ioctl on dimm/bus.
This minor patch updates __nd_ioctl() to propagate the value of out
variable 'cmd_rc' back to user-space in case it reports an error.
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
drivers/nvdimm/bus.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index a8b515968569..5b687a27fdf2 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -1153,6 +1153,11 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
if (rc < 0)
goto out_unlock;
+ if (cmd_rc < 0) {
+ rc = cmd_rc;
+ goto out_unlock;
+ }
+
if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
struct nd_cmd_clear_error *clear_err = buf;
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v1 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Linus Torvalds @ 2020-01-22 16:13 UTC (permalink / raw)
To: Christophe Leroy
Cc: Linux Kernel Mailing List, Linux-MM, Paul Mackerras,
Alexander Viro, linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <a02d3426f93f7eb04960a4d9140902d278cab0bb.1579697910.git.christophe.leroy@c-s.fr>
On Wed, Jan 22, 2020 at 5:00 AM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
> Modify filldir() and filldir64() to request the real area they need
> to get access to.
Not like this.
This makes the situation for architectures like x86 much worse, since
you now use "put_user()" for the previous dirent filling. Which does
that expensive user access setup/teardown twice again.
So either you need to cover both the dirent's with one call, or you
just need to cover the whole (original) user buffer passed in. But not
this unholy mixing of both unsafe_put_user() and regular put_user().
Linus
^ permalink raw reply
* Re: [PATCH RFC v1] mm: is_mem_section_removable() overhaul
From: Michal Hocko @ 2020-01-22 16:46 UTC (permalink / raw)
To: David Hildenbrand
Cc: Nathan Lynch, Stephen Rothwell, Thomas Gleixner,
Anshuman Khandual, Greg Kroah-Hartman, Rafael J. Wysocki,
Linux Kernel Mailing List, Linux MM, Paul Mackerras,
Nathan Fontenot, Leonardo Bras, Dan Williams, linuxppc-dev,
Andrew Morton, Allison Randal, lantianyu1986
In-Reply-To: <816ddd66-c90b-76f1-f4a0-72fe41263edd@redhat.com>
On Wed 22-01-20 12:58:16, David Hildenbrand wrote:
> On 22.01.20 11:54, David Hildenbrand wrote:
> > On 22.01.20 11:42, Michal Hocko wrote:
> >> On Wed 22-01-20 11:39:08, David Hildenbrand wrote:
> >>>>>> Really, the interface is flawed and should have never been merged in the
> >>>>>> first place. We cannot simply remove it altogether I am afraid so let's
> >>>>>> at least remove the bogus code and pretend that the world is a better
> >>>>>> place where everything is removable except the reality sucks...
> >>>>>
> >>>>> As I expressed already, the interface works as designed/documented and
> >>>>> has been used like that for years.
> >>>>
> >>>> It seems we do differ in the usefulness though. Using a crappy interface
> >>>> for years doesn't make it less crappy. I do realize we cannot remove the
> >>>> interface but we can remove issues with the implementation and I dare to
> >>>> say that most existing users wouldn't really notice.
> >>>
> >>> Well, at least powerpc-utils (why this interface was introduced) will
> >>> notice a) performance wise and b) because more logging output will be
> >>> generated (obviously non-offlineable blocks will be tried to offline).
> >>
> >> I would really appreciate some specific example for a real usecase. I am
> >> not familiar with powerpc-utils worklflows myself.
> >>
> >
> > Not an expert myself:
> >
> > https://github.com/ibm-power-utilities/powerpc-utils
> >
> > -> src/drmgr/drslot_chrp_mem.c
> >
> > On request to remove some memory it will
> >
> > a) Read "->removable" of all memory blocks ("lmb")
> > b) Check if the request can be fulfilled using the removable blocks
> > c) Try to offline the memory blocks by trying to offline it. If that
> > succeeded, trigger removeal of it using some hypervisor hooks.
> >
> > Interestingly, with "AMS ballooning", it will already consider the
> > "removable" information useless (most probably, because of
> > non-migratable balloon pages that can be offlined - I assume the powerpc
> > code that I converted to proper balloon compaction just recently). a)
> > and b) is skipped.
> >
> > Returning "yes" on all blocks will make them handle it just like if "AMS
> > ballooning" is active. So any memory block will be tried. Should work
> > but will be slower if no ballooning is active.
> >
>
> On lsmem:
>
> https://www.ibm.com/support/knowledgecenter/linuxonibm/com.ibm.linux.z.lgdd/lgdd_r_lsmem_cmd.html
>
> "
> Removable
> yes if the memory range can be set offline, no if it cannot be set
> offline. A dash (-) means that the range is already offline. The kernel
> method that identifies removable memory ranges is heuristic and not
> exact. Occasionally, memory ranges are falsely reported as removable or
> falsely reported as not removable.
> "
>
> Usage of lsmem paird with chmem:
>
> https://access.redhat.com/solutions/3937181
>
>
> Especially interesting for IBM z Systems, whereby memory
> onlining/offlining will trigger the actual population of memory in the
> hypervisor. So if an admin wants to offline some memory (to give it back
> to the hypervisor), it would use lsmem to identify such blocks first,
> instead of trying random blocks until one offlining request succeeds.
I am sorry for being dense here but I still do not understand why s390
and the way how it does the hotremove matters here. Afterall there are
no arch specific operations done until the memory is offlined. Also
randomly checking memory blocks and then hoping that the offline will
succeed is not way much different from just trying the offline the
block. Both have to crawl through the pfn range and bail out on the
unmovable memory.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] powerpc/papr_scm: Fix leaking 'bus_desc.provider_name' in some paths
From: Aneesh Kumar K.V @ 2020-01-22 16:55 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev; +Cc: Oliver O'Halloran
In-Reply-To: <20200122155140.120429-1-vaibhav@linux.ibm.com>
On 1/22/20 9:21 PM, Vaibhav Jain wrote:
> String 'bus_desc.provider_name' allocated inside
> papr_scm_nvdimm_init() will leaks in case call to
> nvdimm_bus_register() fails or when papr_scm_remove() is called.
>
> This minor patch ensures that 'bus_desc.provider_name' is freed in
> error path for nvdimm_bus_register() as well as in papr_scm_remove().
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index c2ef320ba1bf..eb420655ed0b 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -322,6 +322,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> p->bus = nvdimm_bus_register(NULL, &p->bus_desc);
> if (!p->bus) {
> dev_err(dev, "Error creating nvdimm bus %pOF\n", p->dn);
> + kfree(p->bus_desc.provider_name);
> return -ENXIO;
> }
>
> @@ -477,6 +478,7 @@ static int papr_scm_remove(struct platform_device *pdev)
>
> nvdimm_bus_unregister(p->bus);
> drc_pmem_unbind(p);
> + kfree(p->bus_desc.provider_name);
> kfree(p);
>
> return 0;
>
Add similar error handling to of-pmem driver too?
-aneesh
^ 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