Re: [PATCH v5 21/23] powerpc/book3s64/hash/kuep: Enable KUEP on hash
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/mm/book3s64/pkeys.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 20/23] powerpc/book3s64/hash/kuap: Enable kuap on hash
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/mm/book3s64/pkeys.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 19/23] powerpc/book3s64/kuep: Use Key 3 to implement KUEP with hash translation.
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Radix use IAMR Key 0 and hash translation use IAMR key 3. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/kup.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 18/23] powerpc/book3s64/kuap: Use Key 3 to implement KUAP with hash translation.
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Radix use AMR Key 0 and hash translation use AMR key 3. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/kup.h | 9 - > 1 file changed, 4 insertions(+), 5 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 16/23] powerpc/book3s64/kuap: Restrict access to userspace based on userspace AMR
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > If an application has configured address protection such that read/write is > denied using pkey even the kernel should receive a FAULT on accessing the > same. > > This patch use user AMR value stored in pt_regs.kuap to achieve the same. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/kup.h | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 15/23] powerpc/book3s64/pkeys: Don't update SPRN_AMR when in kernel mode.
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Now that kernel correctly store/restore userspace AMR/IAMR values, avoid > manipulating AMR and IAMR from the kernel on behalf of userspace. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/kup.h | 18 > arch/powerpc/include/asm/processor.h | 4 -- > arch/powerpc/kernel/process.c| 4 -- > arch/powerpc/kernel/traps.c | 6 --- > arch/powerpc/mm/book3s64/pkeys.c | 57 +--- > 5 files changed, 28 insertions(+), 61 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 12/23] powerpc/book3s64/pkeys: Inherit correctly on fork.
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > Child thread.kuap value is inherited from the parent in copy_thread_tls. We > still > need to make sure when the child returns from a fork in the kernel we start > with the kernel > default AMR value. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/kernel/process.c | 9 + > 1 file changed, 9 insertions(+) > Reviewed-by: Sandipan Das
Re: [PATCH v5 13/23] powerpc/book3s64/pkeys: Reset userspace AMR correctly on exec
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > On fork, we inherit from the parent and on exec, we should switch to > default_amr values. > > Also, avoid changing the AMR register value within the kernel. The kernel now > runs with > different AMR values. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/pkeys.h | 2 ++ > arch/powerpc/kernel/process.c | 6 +- > arch/powerpc/mm/book3s64/pkeys.c | 16 ++-- > 3 files changed, 9 insertions(+), 15 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 11/23] powerpc/book3s64/pkeys: Store/restore userspace AMR/IAMR correctly on entry and exit from kernel
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > This prepare kernel to operate with a different value than userspace AMR/IAMR. > For this, AMR/IAMR need to be saved and restored on entry and return from the > kernel. > > With KUAP we modify kernel AMR when accessing user address from the kernel > via copy_to/from_user interfaces. We don't need to modify IAMR value in > similar fashion. > > If MMU_FTR_PKEY is enabled we need to save AMR/IAMR in pt_regs on entering > kernel from userspace. If not we can assume that AMR/IAMR is not modified > from userspace. > > We need to save AMR if we have MMU_FTR_KUAP feature enabled and we are > interrupted within kernel. This is required so that if we get interrupted > within copy_to/from_user we continue with the right AMR value. > > If we hae MMU_FTR_KUEP enabled we need to restore IAMR on return to userspace > beause kernel will be running with a different IAMR value. > > Signed-off-by: Aneesh Kumar K.V > --- > arch/powerpc/include/asm/book3s/64/kup.h | 177 --- > arch/powerpc/include/asm/ptrace.h| 4 +- > arch/powerpc/kernel/asm-offsets.c| 2 + > arch/powerpc/kernel/entry_64.S | 6 +- > arch/powerpc/kernel/exceptions-64s.S | 4 +- > arch/powerpc/kernel/syscall_64.c | 30 +++- > 6 files changed, 192 insertions(+), 31 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH v5 09/23] powerpc/book3s64/kuap: Use Key 3 for kernel mapping with hash translation
On 27/08/20 9:39 am, Aneesh Kumar K.V wrote: > This patch updates kernel hash page table entries to use storage key 3 > for its mapping. This implies all kernel access will now use key 3 to > control READ/WRITE. The patch also prevents the allocation of key 3 from > userspace and UAMOR value is updated such that userspace cannot modify key 3. > > Signed-off-by: Aneesh Kumar K.V > --- > .../powerpc/include/asm/book3s/64/hash-pkey.h | 24 ++- > arch/powerpc/include/asm/book3s/64/hash.h | 2 +- > arch/powerpc/include/asm/book3s/64/mmu-hash.h | 1 + > arch/powerpc/include/asm/mmu_context.h| 2 +- > arch/powerpc/mm/book3s64/hash_4k.c| 2 +- > arch/powerpc/mm/book3s64/hash_64k.c | 4 ++-- > arch/powerpc/mm/book3s64/hash_hugepage.c | 2 +- > arch/powerpc/mm/book3s64/hash_hugetlbpage.c | 2 +- > arch/powerpc/mm/book3s64/hash_pgtable.c | 2 +- > arch/powerpc/mm/book3s64/hash_utils.c | 10 > arch/powerpc/mm/book3s64/pkeys.c | 4 > 11 files changed, 37 insertions(+), 18 deletions(-) > Reviewed-by: Sandipan Das
Re: [PATCH] powerpc/powernv/elog: Reduce elog message severity
On 10/9/20 10:34 AM, Michael Ellerman wrote:
Vasant Hegde writes:
OPAL interrupts kernel whenever it has new error log. Kernel calls
interrupt handler (elog_event()) to retrieve event. elog_event makes
OPAL API call (opal_get_elog_size()) to retrieve elog info.
In some case before kernel makes opal_get_elog_size() call, it gets interrupt
again. So second time when elog_event() calls opal_get_elog_size API OPAL
returns error.
Can you give more detail there? Do you have a stack trace?
I observe below log in dmesg
[ 615.708709] ELOG: OPAL log info read failed
That means elog_event called twice for same error log event.
I don't have stack trace.
We use IRQF_ONESHOT for elog_event(), which (I thought) meant it
shouldn't be called again until it has completed.
Yes. I thought so.. But once in a while we get above message (mostly when we
have multiple event bits are set).
-Vasant
So I'm unclear how you're seeing the behaviour you describe.
cheers
Its safe to ignore this error. Hence reduce the severity
of log message.
CC: Mahesh Salgaonkar
Signed-off-by: Vasant Hegde
---
arch/powerpc/platforms/powernv/opal-elog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal-elog.c
b/arch/powerpc/platforms/powernv/opal-elog.c
index 62ef7ad995da..67f435bb1ec4 100644
--- a/arch/powerpc/platforms/powernv/opal-elog.c
+++ b/arch/powerpc/platforms/powernv/opal-elog.c
@@ -247,7 +247,7 @@ static irqreturn_t elog_event(int irq, void *data)
rc = opal_get_elog_size(, , );
if (rc != OPAL_SUCCESS) {
- pr_err("ELOG: OPAL log info read failed\n");
+ pr_debug("ELOG: OPAL log info read failed\n");
return IRQ_HANDLED;
}
--
2.26.2
[PATCH] powerpc/powernv/dump: Handle multiple writes to ack attribute
Even though we use self removing sysfs helper, we still need
to make sure we do the final kobject delete conditionally.
sysfs_remove_file_self() will handle parallel calls to remove
the sysfs attribute file and returns true only in the caller
that removed the attribute file. The other parallel callers
are returned false. Do the final kobject delete checking
the return value of sysfs_remove_file_self().
Cc: Aneesh Kumar K.V
Cc: Mahesh Salgaonkar
Signed-off-by: Vasant Hegde
---
arch/powerpc/platforms/powernv/opal-dump.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-dump.c
b/arch/powerpc/platforms/powernv/opal-dump.c
index 0e6693bacb7e..00c5a59d82d9 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -88,9 +88,14 @@ static ssize_t dump_ack_store(struct dump_obj *dump_obj,
const char *buf,
size_t count)
{
- dump_send_ack(dump_obj->id);
- sysfs_remove_file_self(_obj->kobj, >attr);
- kobject_put(_obj->kobj);
+ /*
+* Try to self remove this attribute. If we are successful,
+* delete the kobject itself.
+*/
+ if (sysfs_remove_file_self(_obj->kobj, >attr)) {
+ dump_send_ack(dump_obj->id);
+ kobject_put(_obj->kobj);
+ }
return count;
}
--
2.26.2
[PATCH v2] powerpc/powernv/dump: Fix race while processing OPAL dump
Every dump reported by OPAL is exported to userspace through a sysfs
interface and notified using kobject_uevent(). The userspace daemon
(opal_errd) then reads the dump and acknowledges that the dump is
saved safely to disk. Once acknowledged the kernel removes the
respective sysfs file entry causing respective resources to be
released including kobject.
However it's possible the userspace daemon may already be scanning
dump entries when a new sysfs dump entry is created by the kernel.
User daemon may read this new entry and ack it even before kernel can
notify userspace about it through kobject_uevent() call. If that
happens then we have a potential race between
dump_ack_store->kobject_put() and kobject_uevent which can lead to
use-after-free of a kernfs object resulting in a kernel crash.
This patch fixes this race by protecting the sysfs file
creation/notification by holding a reference count on kobject until we
safely send kobject_uevent().
The function create_dump_obj() returns the dump object which if used
by caller function will end up in use-after-free problem again.
However, the return value of create_dump_obj() function isn't being
used today and there is no need as well. Hence change it to return
void to make this fix complete.
Fixes: c7e64b9c ("powerpc/powernv Platform dump interface")
CC: Mahesh Salgaonkar
Signed-off-by: Vasant Hegde
---
arch/powerpc/platforms/powernv/opal-dump.c | 41 +++---
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-dump.c
b/arch/powerpc/platforms/powernv/opal-dump.c
index 543c816fa99e..0e6693bacb7e 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -318,15 +318,14 @@ static ssize_t dump_attr_read(struct file *filep, struct
kobject *kobj,
return count;
}
-static struct dump_obj *create_dump_obj(uint32_t id, size_t size,
- uint32_t type)
+static void create_dump_obj(uint32_t id, size_t size, uint32_t type)
{
struct dump_obj *dump;
int rc;
dump = kzalloc(sizeof(*dump), GFP_KERNEL);
if (!dump)
- return NULL;
+ return;
dump->kobj.kset = dump_kset;
@@ -346,21 +345,39 @@ static struct dump_obj *create_dump_obj(uint32_t id,
size_t size,
rc = kobject_add(>kobj, NULL, "0x%x-0x%x", type, id);
if (rc) {
kobject_put(>kobj);
- return NULL;
+ return;
}
+ /*
+* As soon as the sysfs file for this dump is created/activated there is
+* a chance the opal_errd daemon (or any userspace) might read and
+* acknowledge the dump before kobject_uevent() is called. If that
+* happens then there is a potential race between
+* dump_ack_store->kobject_put() and kobject_uevent() which leads to a
+* use-after-free of a kernfs object resulting in a kernel crash.
+*
+* To avoid that, we need to take a reference on behalf of the bin file,
+* so that our reference remains valid while we call kobject_uevent().
+* We then drop our reference before exiting the function, leaving the
+* bin file to drop the last reference (if it hasn't already).
+*/
+
+ /* Take a reference for the bin file */
+ kobject_get(>kobj);
rc = sysfs_create_bin_file(>kobj, >dump_attr);
- if (rc) {
+ if (rc == 0) {
+ kobject_uevent(>kobj, KOBJ_ADD);
+
+ pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
+ __func__, dump->id, dump->size);
+ } else {
+ /* Drop reference count taken for bin file */
kobject_put(>kobj);
- return NULL;
}
- pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
- __func__, dump->id, dump->size);
-
- kobject_uevent(>kobj, KOBJ_ADD);
-
- return dump;
+ /* Drop our reference */
+ kobject_put(>kobj);
+ return;
}
static irqreturn_t process_dump(int irq, void *data)
--
2.26.2
[PATCH] asm-generic: Force inlining of get_order() to work around gcc10 poor decision
When building mpc885_ads_defconfig with gcc 10.1,
the function get_order() appears 50 times in vmlinux:
[linux]# ppc-linux-objdump -x vmlinux | grep get_order | wc -l
50
[linux]# size vmlinux
textdata bss dec hex filename
3842620 675624 135160 4653404 47015c vmlinux
In the old days, marking a function 'static inline' was forcing
GCC to inline, but since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") GCC may decide to not inline
a function.
It looks like GCC 10 is taking poor decisions on this.
get_order() compiles into the following tiny function,
occupying 20 bytes of text.
007c :
7c: 38 63 ff ff addir3,r3,-1
80: 54 63 a3 3e rlwinm r3,r3,20,12,31
84: 7c 63 00 34 cntlzw r3,r3
88: 20 63 00 20 subfic r3,r3,32
8c: 4e 80 00 20 blr
By forcing get_order() to be __always_inline, the size of text is
reduced by 1940 bytes, that is almost twice the space occupied by
50 times get_order()
[linux-powerpc]# size vmlinux
textdata bss dec hex filename
3840680 675588 135176 4651444 46f9b4 vmlinux
Signed-off-by: Christophe Leroy
---
include/asm-generic/getorder.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/asm-generic/getorder.h b/include/asm-generic/getorder.h
index e9f20b813a69..f2979e3a96b6 100644
--- a/include/asm-generic/getorder.h
+++ b/include/asm-generic/getorder.h
@@ -26,7 +26,7 @@
*
* The result is undefined if the size is 0.
*/
-static inline __attribute_const__ int get_order(unsigned long size)
+static __always_inline __attribute_const__ int get_order(unsigned long size)
{
if (__builtin_constant_p(size)) {
if (!size)
--
2.25.0
Re: [PATCH] powerpc/powernv/dump: Fix race while processing OPAL dump
On 10/9/20 10:36 AM, Michael Ellerman wrote:
Vasant Hegde writes:
diff --git a/arch/powerpc/platforms/powernv/opal-dump.c
b/arch/powerpc/platforms/powernv/opal-dump.c
index 543c816fa99e..7e6eeedec32b 100644
--- a/arch/powerpc/platforms/powernv/opal-dump.c
+++ b/arch/powerpc/platforms/powernv/opal-dump.c
@@ -346,21 +345,39 @@ static struct dump_obj *create_dump_obj(uint32_t id,
size_t size,
rc = kobject_add(>kobj, NULL, "0x%x-0x%x", type, id);
if (rc) {
kobject_put(>kobj);
- return NULL;
+ return;
}
+ /*
+* As soon as the sysfs file for this dump is created/activated there is
+* a chance the opal_errd daemon (or any userspace) might read and
+* acknowledge the dump before kobject_uevent() is called. If that
+* happens then there is a potential race between
+* dump_ack_store->kobject_put() and kobject_uevent() which leads to a
+* use-after-free of a kernfs object resulting in a kernel crash.
+*
+* To avoid that, we need to take a reference on behalf of the bin file,
+* so that our reference remains valid while we call kobject_uevent().
+* We then drop our reference before exiting the function, leaving the
+* bin file to drop the last reference (if it hasn't already).
+*/
+
+ /* Take a reference for the bin file */
+ kobject_get(>kobj);
rc = sysfs_create_bin_file(>kobj, >dump_attr);
if (rc) {
kobject_put(>kobj);
- return NULL;
+ /* Drop reference count taken for bin file */
+ kobject_put(>kobj);
+ return;
}
pr_info("%s: New platform dump. ID = 0x%x Size %u\n",
__func__, dump->id, dump->size);
kobject_uevent(>kobj, KOBJ_ADD);
-
- return dump;
+ /* Drop reference count taken for bin file */
+ kobject_put(>kobj);
}
I think this would be better if it was reworked along the lines of:
aea948bb80b4 ("powerpc/powernv/elog: Fix race while processing OPAL error log
event.")
Sure. Will fix it in v2.
-Vasant
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.10-1 tag
Linus Torvalds writes: > On Thu, Oct 15, 2020 at 8:24 PM Michael Ellerman wrote: >> >> Just two minor conflicts I'm aware of. The only slight subtlety is the >> conflict >> in kasan_init() where "int ret" needs to move out of the for_each_mem_range() >> and up to the function scope. > > Well, there was also a conflict for the dependencies of OCXL. Yep, it was so trivial I didn't think it was worth calling out. > I resolved that by ruthlessly simplifying the dependency: > > - depends on PPC_POWERNV && PCI && EEH && HOTPLUG_PCI_POWERNV > - depends on PPC_POWERNV && PCI && EEH && PPC_XIVE_NATIVE > ++ depends on HOTPLUG_PCI_POWERNV > > because all the other dependencies seem to be pointless. > > HOTPLUG_PCI_POWERNV already has a > > depends on PPC_POWERNV && EEH > > so there's no point in repeating those. And PPC_POWERNV selects FORCE_PCI, which takes care of the dependency on PCI. > And PPC_XIVE_NATIVE is selected by PPC_POWERNV, so if PPC_POWERNV, we > know PPC_XIVE_NATIVE is set. > > Maybe I missed something strange, so I'm just letting you know so you > can blame me if I broke something. No, that looks good to me. Thanks. cheers
