Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Mar 2022 10:26:12 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pkeys: Make pkey unsigned in arch_set_user_pkey_access()
Message-ID: <YiZORNusRg4UTv45@iweiny-desk3>
References: <20220304210543.3490880-1-ira.weiny@intel.com>
 <878rtmtfgs.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <878rtmtfgs.fsf@linux.ibm.com>
Precedence: bulk

On Mon, Mar 07, 2022 at 12:30:03PM +0530, Aneesh Kumar K.V wrote:
> ira.weiny@intel.com writes:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > The WARN_ON check in arch_set_user_pkey_access() in the x86 architecture
> > fails to check for an invalid negative value.
> >
> > A simple check for less than 0 would fix this issue however, in the call
> > stack below arch_set_user_pkey_access() the pkey should never be
> > negative on any architecture.  It is always best to use correct types
> > when possible.  x86 only supports 16 keys while ppc supports 32, u8 is
> > therefore large enough for all current architectures and likely those in
> > the future.
> 
> Should we do that as a separate patch? ie, now convert the variable to
> unsigned int and later switch all the variables to u8?

Maybe.

> because what we
> now have is confusing.
> 
> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> 		unsigned long pkey)
> static inline u64 pkey_to_vmflag_bits(u16 pkey)
> 

This looks like a good cleanup as well.  Why not convert
arch_calc_vm_prot_bits() and pkey_to_vmflag_bits() to u8?  (In another patch.)

This is all a result of this PKS conversation:

https://lore.kernel.org/lkml/Yg8C6UkgfBmQlPSq@iweiny-desk3/

That started me down the path of trying to figure out why 'int' was used for
PKRU and I realized that negative values had meaning there which did not apply
to me with PKS.  So at some point a conversion needs to be made between a
'conceptual pkey' (int) and a real pkey (unsigned) IHMO.

It's no bit deal to split this patch into one which converts to unsigned and
then another to u8 (or u16 if there is some arch which may need it that big).

However, digging more:

Is there a reason u16 was used in pkey_to_vmflag_bits()?  How about in
__pkru_allows_read() in the x86 code?  If possible I think u8 should be
standardized but I'm ok with u16 if that is preferred.

Also, am I missing something in init_amr() and init_iamr()?  I think I could
have gone farther and changed init_amr() and init_iamr() right?

From what I can see the argument to use unsigned long vs u8 (or u16) is some
expectation that pkeys will grow beyond 256 in number.  From what I can see I
don't think that is going to happen.

So do we need to do this in two steps?

Ira

> 
> >
> > Change the type of the pkey passed to arch_set_user_pkey_access() to u8.
> >
> > To: Dave Hansen <dave.hansen@linux.intel.com>
> > To: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  arch/powerpc/include/asm/pkeys.h | 4 ++--
> >  arch/powerpc/mm/book3s64/pkeys.c | 2 +-
> >  arch/x86/include/asm/pkeys.h     | 4 ++--
> >  arch/x86/kernel/fpu/xstate.c     | 2 +-
> >  include/linux/pkeys.h            | 2 +-
> >  5 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> > index 59a2c7dbc78f..e70615a1da9b 100644
> > --- a/arch/powerpc/include/asm/pkeys.h
> > +++ b/arch/powerpc/include/asm/pkeys.h
> > @@ -143,9 +143,9 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
> >  	return __arch_override_mprotect_pkey(vma, prot, pkey);
> >  }
> >  
> > -extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +extern int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  				       unsigned long init_val);
> 
> 
> > -static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  					    unsigned long init_val)
> >  {
> >  	if (!mmu_has_feature(MMU_FTR_PKEY))
> > diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> > index 753e62ba67af..c048467669df 100644
> > --- a/arch/powerpc/mm/book3s64/pkeys.c
> > +++ b/arch/powerpc/mm/book3s64/pkeys.c
> > @@ -333,7 +333,7 @@ static inline void init_iamr(int pkey, u8 init_bits)
> >   * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
> >   * specified in @init_val.
> >   */
> > -int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  				unsigned long init_val)
> >  {
> >  	u64 new_amr_bits = 0x0ul;
> > diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> > index 5292e6dfe2a7..48efb81f6cc6 100644
> > --- a/arch/x86/include/asm/pkeys.h
> > +++ b/arch/x86/include/asm/pkeys.h
> > @@ -9,7 +9,7 @@
> >   */
> >  #define arch_max_pkey() (cpu_feature_enabled(X86_FEATURE_OSPKE) ? 16 : 1)
> >  
> > -extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +extern int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  		unsigned long init_val);
> >  
> >  static inline bool arch_pkeys_enabled(void)
> > @@ -115,7 +115,7 @@ int mm_pkey_free(struct mm_struct *mm, int pkey)
> >  	return 0;
> >  }
> >  
> > -extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +extern int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  		unsigned long init_val);
> >  
> >  static inline int vma_pkey(struct vm_area_struct *vma)
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index 7c7824ae7862..db511bec57e5 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -1068,7 +1068,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
> >   * This will go out and modify PKRU register to set the access
> >   * rights for @pkey to @init_val.
> >   */
> > -int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  			      unsigned long init_val)
> >  {
> >  	u32 old_pkru, new_pkru_bits = 0;
> > diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> > index 86be8bf27b41..aa40ed2fb0fc 100644
> > --- a/include/linux/pkeys.h
> > +++ b/include/linux/pkeys.h
> > @@ -35,7 +35,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> >  	return -EINVAL;
> >  }
> >  
> > -static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> > +static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
> >  			unsigned long init_val)
> >  {
> >  	return 0;
> > -- 
> > 2.35.1