Re: [PATCH 10/11] scs: generic scs code updated to leverage hw assisted shadow stack

[Deepak Gupta]

On Mon, Jul 28, 2025 at 01:47:14PM +0100, Will Deacon wrote:
> On Fri, Jul 25, 2025 at 04:13:27PM +0000, Sami Tolvanen wrote:
> > On Thu, Jul 24, 2025 at 04:37:03PM -0700, Deepak Gupta wrote:
> > > diff --git a/include/linux/scs.h b/include/linux/scs.h
> > > index 4ab5bdc898cf..6ceee07c2d1a 100644
> > > --- a/include/linux/scs.h
> > > +++ b/include/linux/scs.h
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/poison.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/sizes.h>
> > > +#include <asm/scs.h>
> > >
> > >  #ifdef CONFIG_SHADOW_CALL_STACK
> > >
> > > @@ -37,22 +38,45 @@ static inline void scs_task_reset(struct task_struct *tsk)
> > >  	 * Reset the shadow stack to the base address in case the task
> > >  	 * is reused.
> > >  	 */
> > > +#ifdef CONFIG_ARCH_HAS_KERNEL_SHADOW_STACK
> > > +	task_scs_sp(tsk) = task_scs(tsk) + SCS_SIZE;
> > > +#else
> > >  	task_scs_sp(tsk) = task_scs(tsk);
> > > +#endif
> > >  }
> > >
> > >  static inline unsigned long *__scs_magic(void *s)
> > >  {
> > > +#ifdef CONFIG_ARCH_HAS_KERNEL_SHADOW_STACK
> > > +	return (unsigned long *)(s);
> > > +#else
> > >  	return (unsigned long *)(s + SCS_SIZE) - 1;
> > > +#endif
> > >  }
> > >
> > >  static inline bool task_scs_end_corrupted(struct task_struct *tsk)
> > >  {
> > >  	unsigned long *magic = __scs_magic(task_scs(tsk));
> > > -	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
> > > +	unsigned long sz;
> > > +
> > > +#ifdef CONFIG_ARCH_HAS_KERNEL_SHADOW_STACK
> > > +	sz = (task_scs(tsk) + SCS_SIZE) - task_scs_sp(tsk);
> > > +#else
> > > +	sz = task_scs_sp(tsk) - task_scs(tsk);
> > > +#endif
> > >
> > >  	return sz >= SCS_SIZE - 1 || READ_ONCE_NOCHECK(*magic) != SCS_END_MAGIC;
> > >  }
> > >
> > > +static inline void __scs_store_magic(unsigned long *s, unsigned long magic_val)
> > > +{
> > > +#ifdef CONFIG_ARCH_HAS_KERNEL_SHADOW_STACK
> > > +	arch_scs_store(s, magic_val);
> > > +#else
> > > +	*__scs_magic(s) = magic_val;
> > > +#endif
> > > +}
> > > +
> >
> > I'm not a huge fan of all the ifdefs. We could clean this up by
> > allowing architectures to simply override some these functions, or at
> > least use if (IS_ENABLED(CONFIG...)) instead. Will, any thoughts about
> > this?
>
> Yeah, I agree that allowing architectures to provide overrides makes
> sense, however I also suspect that some of this needs to be a runtime
> decision because not all CPUs will support the hardware-accelerated
> feature and will presumably want to fall back on the software
> implementation.

Hmm runtime fallback is an important point. Thanks. I'll munch on it a
bit.

> Will