Re: arch/arm64/kernel/signal.c:1046:36: sparse: sparse: cast removes address space '__user' of expression

[Catalin Marinas]

On Mon, Dec 09, 2024 at 04:11:00PM +0000, Mark Brown wrote:
> On Mon, Dec 09, 2024 at 03:37:23PM +0000, Catalin Marinas wrote:
> > On Mon, Dec 09, 2024 at 12:47:33PM +0800, kernel test robot wrote:
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1014       unsigned long __user *gcspr_el0;
> > 
> > I think we should keep this as u64 since it's a sysreg.
> 
> Do you mean pointer to u64 or plain u64? 

Plain u64.

> The value we get from the
> sysreg is a pointer so it makes the uses of the value clearer if we keep
> it as a pointer in C code, it seems to be defeating the point of doing
> static analysis to discard the pointerness to make it happier.

We have other cases where we treat a reg as u64 and convert it to
pointer as needed. While not a sysreg, the pt_regs::sp is u64 and we end
up treating it as a pointer eventually for writing the signal stack.
Another case is user_insn_read(). It's bit of bikeshedding around the
primary use in this function, do we need more conversions one way or the
other? In general I'd consider a sysreg read to be u64, especially as
the architecture has a habit of adding bits around the actual address
occasionally.

> > > eaf62ce1563b85 Mark Brown      2024-10-01  1051  	if (ret != 0)
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1052  		return -EFAULT;
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1053  
> > > eaf62ce1563b85 Mark Brown      2024-10-01  1054  	write_sysreg_s(gcspr_el0 + 1, SYS_GCSPR_EL0);
> 
> > And this would be +8 I guess.
> 
> The variable is a pointer so we're doing pointer arithmetic here not
> working directly with the value, unless we change the value to be purely
> a u64 with no pointer in which case we would need the case above.

That's what I meant, if we go for u64 we'll need a +8 here.

Anyway, I'd like to silence sparse on this. The u64 (non-pointer) has
some precedence in the arm64 code but, if you want, I'm happy to keep it
as a pointer (and maybe just rename it to shadow_stack or something that
does not imply a sysreg). I think for the actual warning, we can
probably fix it with a __force to silence sparse on conversion to u64.

-- 
Catalin