| Message ID | 1516326648-22775-28-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc, mm: Memory Protection Keys | expand |
Ram Pai <linuxram@us.ibm.com> writes: > Currently the architecture specific code is expected to > display the protection keys in smap for a given vma. > This can lead to redundant code and possibly to divergent > formats in which the key gets displayed. > > This patch changes the implementation. It displays the > pkey only if the architecture support pkeys. > > x86 arch_show_smap() function is not needed anymore. > Delete it. > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > --- > arch/x86/kernel/setup.c | 8 -------- > fs/proc/task_mmu.c | 11 ++++++----- > 2 files changed, 6 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 8af2e8d..ddf945a 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -1326,11 +1326,3 @@ static int __init register_kernel_offset_dumper(void) > return 0; > } > __initcall(register_kernel_offset_dumper); > - > -void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) > -{ > - if (!boot_cpu_has(X86_FEATURE_OSPKE)) > - return; > - > - seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > -} > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index 0edd4da..4b39a94 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -18,6 +18,7 @@ > #include <linux/page_idle.h> > #include <linux/shmem_fs.h> > #include <linux/uaccess.h> > +#include <linux/pkeys.h> > > #include <asm/elf.h> > #include <asm/tlb.h> > @@ -728,10 +729,6 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, > } > #endif /* HUGETLB_PAGE */ > > -void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) > -{ > -} > - > static int show_smap(struct seq_file *m, void *v, int is_pid) > { > struct proc_maps_private *priv = m->private; > @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) > (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); > > if (!rollup_mode) { > - arch_show_smap(m, vma); > +#ifdef CONFIG_ARCH_HAS_PKEYS > + if (arch_pkeys_enabled()) > + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > +#endif Would it be worth it making vma_pkey a noop on architectures that don't support protection keys so that we don't need the #ifdef here? Eric > show_smap_vma_flags(m, vma); > } > + > m_cache_vma(m, vma); > return ret; > }
On Fri, Jan 19, 2018 at 10:09:41AM -0600, Eric W. Biederman wrote: > Ram Pai <linuxram@us.ibm.com> writes: > > > Currently the architecture specific code is expected to > > display the protection keys in smap for a given vma. > > This can lead to redundant code and possibly to divergent > > formats in which the key gets displayed. > > > > This patch changes the implementation. It displays the > > pkey only if the architecture support pkeys. > > > > x86 arch_show_smap() function is not needed anymore. > > Delete it. > > > > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > > --- > > arch/x86/kernel/setup.c | 8 -------- > > fs/proc/task_mmu.c | 11 ++++++----- > > 2 files changed, 6 insertions(+), 13 deletions(-) > > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > > index 8af2e8d..ddf945a 100644 > > --- a/arch/x86/kernel/setup.c > > +++ b/arch/x86/kernel/setup.c > > @@ -1326,11 +1326,3 @@ static int __init register_kernel_offset_dumper(void) > > return 0; > > } > > __initcall(register_kernel_offset_dumper); > > - > > -void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) > > -{ > > - if (!boot_cpu_has(X86_FEATURE_OSPKE)) > > - return; > > - > > - seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > > -} > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > > index 0edd4da..4b39a94 100644 > > --- a/fs/proc/task_mmu.c > > +++ b/fs/proc/task_mmu.c > > @@ -18,6 +18,7 @@ > > #include <linux/page_idle.h> > > #include <linux/shmem_fs.h> > > #include <linux/uaccess.h> > > +#include <linux/pkeys.h> > > > > #include <asm/elf.h> > > #include <asm/tlb.h> > > @@ -728,10 +729,6 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, > > } > > #endif /* HUGETLB_PAGE */ > > > > -void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) > > -{ > > -} > > - > > static int show_smap(struct seq_file *m, void *v, int is_pid) > > { > > struct proc_maps_private *priv = m->private; > > @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) > > (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); > > > > if (!rollup_mode) { > > - arch_show_smap(m, vma); > > +#ifdef CONFIG_ARCH_HAS_PKEYS > > + if (arch_pkeys_enabled()) > > + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > > +#endif > > Would it be worth it making vma_pkey a noop on architectures that don't > support protection keys so that we don't need the #ifdef here? You mean something like this? #define vma_pkey(vma) It will lead to compilation error. I can make it #define vma_pkey(vma) 0 and that will work and get rid of the #ifdef RP
Ram Pai <linuxram@us.ibm.com> writes: > On Fri, Jan 19, 2018 at 10:09:41AM -0600, Eric W. Biederman wrote: >> Ram Pai <linuxram@us.ibm.com> writes: >> >> > Currently the architecture specific code is expected to >> > display the protection keys in smap for a given vma. >> > This can lead to redundant code and possibly to divergent >> > formats in which the key gets displayed. >> > >> > This patch changes the implementation. It displays the >> > pkey only if the architecture support pkeys. >> > >> > x86 arch_show_smap() function is not needed anymore. >> > Delete it. >> > >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> >> > --- >> > arch/x86/kernel/setup.c | 8 -------- >> > fs/proc/task_mmu.c | 11 ++++++----- >> > 2 files changed, 6 insertions(+), 13 deletions(-) >> > >> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c >> > index 8af2e8d..ddf945a 100644 >> > --- a/arch/x86/kernel/setup.c >> > +++ b/arch/x86/kernel/setup.c >> > @@ -1326,11 +1326,3 @@ static int __init register_kernel_offset_dumper(void) >> > return 0; >> > } >> > __initcall(register_kernel_offset_dumper); >> > - >> > -void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) >> > -{ >> > - if (!boot_cpu_has(X86_FEATURE_OSPKE)) >> > - return; >> > - >> > - seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); >> > -} >> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c >> > index 0edd4da..4b39a94 100644 >> > --- a/fs/proc/task_mmu.c >> > +++ b/fs/proc/task_mmu.c >> > @@ -18,6 +18,7 @@ >> > #include <linux/page_idle.h> >> > #include <linux/shmem_fs.h> >> > #include <linux/uaccess.h> >> > +#include <linux/pkeys.h> >> > >> > #include <asm/elf.h> >> > #include <asm/tlb.h> >> > @@ -728,10 +729,6 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, >> > } >> > #endif /* HUGETLB_PAGE */ >> > >> > -void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) >> > -{ >> > -} >> > - >> > static int show_smap(struct seq_file *m, void *v, int is_pid) >> > { >> > struct proc_maps_private *priv = m->private; >> > @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) >> > (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); >> > >> > if (!rollup_mode) { >> > - arch_show_smap(m, vma); >> > +#ifdef CONFIG_ARCH_HAS_PKEYS >> > + if (arch_pkeys_enabled()) >> > + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); >> > +#endif >> >> Would it be worth it making vma_pkey a noop on architectures that don't >> support protection keys so that we don't need the #ifdef here? > > You mean something like this? > #define vma_pkey(vma) > It will lead to compilation error. > > > I can make it > #define vma_pkey(vma) 0 > > and that will work and get rid of the #ifdef Yes the second is what I was thinking. I don't know if it is worth it but #ifdefs can be problematic as the result in code not being compile tested. Eric
On Thu 18-01-18 17:50:48, Ram Pai wrote: [...] > @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) > (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); > > if (!rollup_mode) { > - arch_show_smap(m, vma); > +#ifdef CONFIG_ARCH_HAS_PKEYS > + if (arch_pkeys_enabled()) > + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > +#endif > show_smap_vma_flags(m, vma); > } > + Why do you need to add ifdef here? The previous patch should make arch_pkeys_enabled == F when CONFIG_ARCH_HAS_PKEYS=n. Btw. could you merge those two patches into one. It is usually much easier to review a new helper function if it is added along with a user. > m_cache_vma(m, vma); > return ret; > } > -- > 1.7.1
On Tue, Jan 30, 2018 at 01:16:11PM +0100, Michal Hocko wrote: > On Thu 18-01-18 17:50:48, Ram Pai wrote: > [...] > > @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) > > (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); > > > > if (!rollup_mode) { > > - arch_show_smap(m, vma); > > +#ifdef CONFIG_ARCH_HAS_PKEYS > > + if (arch_pkeys_enabled()) > > + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); > > +#endif > > show_smap_vma_flags(m, vma); > > } > > + > > Why do you need to add ifdef here? The previous patch should make > arch_pkeys_enabled == F when CONFIG_ARCH_HAS_PKEYS=n. You are right. it need not be wrapped in CONFIG_ARCH_HAS_PKEYS. I had to do it because vma_pkey(vma) is not defined in some architectures. I will provide a generic vma_pkey() definition for architectures that do not support PKEYS. > Btw. could you > merge those two patches into one. It is usually much easier to review a > new helper function if it is added along with a user. ok. Thanks, RP
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 8af2e8d..ddf945a 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -1326,11 +1326,3 @@ static int __init register_kernel_offset_dumper(void) return 0; } __initcall(register_kernel_offset_dumper); - -void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) -{ - if (!boot_cpu_has(X86_FEATURE_OSPKE)) - return; - - seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); -} diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 0edd4da..4b39a94 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -18,6 +18,7 @@ #include <linux/page_idle.h> #include <linux/shmem_fs.h> #include <linux/uaccess.h> +#include <linux/pkeys.h> #include <asm/elf.h> #include <asm/tlb.h> @@ -728,10 +729,6 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask, } #endif /* HUGETLB_PAGE */ -void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma) -{ -} - static int show_smap(struct seq_file *m, void *v, int is_pid) { struct proc_maps_private *priv = m->private; @@ -851,9 +848,13 @@ static int show_smap(struct seq_file *m, void *v, int is_pid) (unsigned long)(mss->pss >> (10 + PSS_SHIFT))); if (!rollup_mode) { - arch_show_smap(m, vma); +#ifdef CONFIG_ARCH_HAS_PKEYS + if (arch_pkeys_enabled()) + seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma)); +#endif show_smap_vma_flags(m, vma); } + m_cache_vma(m, vma); return ret; }
Currently the architecture specific code is expected to display the protection keys in smap for a given vma. This can lead to redundant code and possibly to divergent formats in which the key gets displayed. This patch changes the implementation. It displays the pkey only if the architecture support pkeys. x86 arch_show_smap() function is not needed anymore. Delete it. Signed-off-by: Ram Pai <linuxram@us.ibm.com> --- arch/x86/kernel/setup.c | 8 -------- fs/proc/task_mmu.c | 11 ++++++----- 2 files changed, 6 insertions(+), 13 deletions(-)