mips ptrace32.c and compat_ptr()

There's something odd going on:

1)
static inline void __user *compat_ptr(compat_uptr_t uptr)

{
        /* cast to a __user pointer via "unsigned long" makes sparse happy */
        return (void __user *)(unsigned long)(long)uptr;
}

Huh? The first impression is that it wants to sign-extend the argument, but... compat_uptr_t is and always had been unsigned. Initially it went
+typedef u32 compat_uptr_t;
+
+static inline void *compat_ptr(compat_uptr_t uptr) +{
+ return (void *)(long)uptr;
+}
so there never had been any sign-extension in that thing. So what is that cast to long in the current version about?

2)
long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
compat_ulong_t caddr, compat_ulong_t cdata)

{
        int addr = caddr;
        int data = cdata;
...
                ret = put_user(tmp, (u32 __user *) (unsigned long) data);
                break;

and quite a few similar in the same function. Here we _do_ get sign extension. Which is bloody odd, seeing that all other compat syscalls end up using compat_ptr(), either explicitly or in the syscall glue.

Shouldn't it be doing

                ret = put_user(tmp, (u32 __user *)compat_ptr(cdata));
                break;

instead?

Hi Al,

Good question. It looks like it's been there ever since it was added in the separate arch/mips64 [1] & then got carried forwards into arch/mips.

The only thing I can think up is that perhaps Ralf added the cast to long in order to avoid warnings from -Wint-to-pointer-cast, and ought to have used unsigned long but didn't & got away with it. Later sparse warned about the cast & we got the band-aid of the extra cast from long to unsigned long [2], which ought to have replaced rather than supplemented the cast to long. All speculative of course, the git history isn't really descriptive enough to say anything for sure...

This is weird, and I need to go wrap my head around it. Worrying & confusing me further is the comment above compat_ptr:

I initially thought it was trying to get at the fact that 32b user code ought to be providing sign extended addresses anyway since MIPS32 instructions generally sign extend when run on a MIPS64 CPU, but actually arch/mips/kernel/scall64-o32.S implicitly sign extends arguments (using sll for the first 4 args in a0-a3, and the sign-extending lw instruction for any further arguments on the stack).

So perhaps this is what that comment is referring to, and it's saying that syscall entry points should just declare the arguments as pointers since scall64-o32.S will have sign extended addresses already anyway which makes explicitly trying to sign extend using compat_ptr() a no-op (even if it did some sign extension).

That implicit sign extension is not done for n32, only o32. And this is despite the fact that n32 is where I'd expect us to be more likely to have non-sign-extended addresses since we'll have MIPS64 instructions in the mix that could more easily generate such addresses.

And of course that only accounts for pointers passed directly as arguments to syscalls - a pointer in a struct will still need sign extension & we seem to have various bits of code scattered around doing that sort of thing that probably ought to use compat_ptr() if it worked properly.

I suppose we're getting away with this purely because of the typical 2GB/2GB user/kernel split of the MIPS32 address space. If users never have pointers with bit 31 set then sign extending or zero extending is all the same thing, and compat_ptr() doing zero extension becomes harmless.

I need to give this some further thought though & it feels like something we can clean up for the sake of sanity if nothing else.

Thanks,
Paul

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/include/asm-mips64/compat.h?id=f19e2d9e9eb4
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/asm-mips/compat.h?id=01bebc66793f