# Re: \[PATCH\] Adds a new ioctl32 syscall for backwards compatibility layers - List: linux-api - 4 messages, 4 authors - Page: https://ratatoskr.run/linux-api/2021/01/197410/t ## Re: \[PATCH\] Adds a new ioctl32 syscall for backwards compatibility layers - From: Arnd Bergmann \ - Date: 2021-01-15 09:02:04 - Message: https://ratatoskr.run/linux-api/2021/01/197410.md ```text On Fri, Jan 15, 2021 at 3:06 AM Ryan Houdek [ref] wrote: > On Wed, Jan 6, 2021 at 12:49 AM Arnd Bergmann [ref] wrote: >> On Wed, Jan 6, 2021 at 7:48 AM [ref] wrote: >> > From: Ryan Houdek >> ... >> >> For x86, this has another complication, as some ioctls also need to >> check whether they are in an ia32 task (with packed u64 and 32-bit >> __kernel_old_time_t) or an x32 task (with aligned u64 and 64-bit >> __kernel_old_time_t). If the new syscall gets wired up on x86 as well, >> you'd need to decide which of the two behaviors you want. > > > I can have a follow-up patch that makes this do ni-syscall on x86_64 since > we can go through the int 0x80 handler, or x32 handler path and choose > whichever one there. I'd say for consistency >> > 3a) Userspace consumes all VA space above 32bit. Forcing allocations to >> > occur in lower 32bits >> > - This is the current implementation >> > 3b) Ensure any allocation in the ioctl handles ioctl entrypoint rather >> > than just allow generic memory allocations in full VA space >> > - This is hard to guarantee >> >> What kind of allocation do you mean here? Can you give an example of >> an ioctl that does this? >> > My concern here would be something like DRM allocating memory and > returning a pointer to userspace that ends up in 64bit space. > I can see something like `drm_get_unmapped_area` calls in to > `current->mm->get_unmapped_area` which I believe only ends up falling > down TASK_SIZE checks. I see. > Which could potentially return pointers in the 64bit address space range > in this case. Theoretically can be resolved either by thieving the full 64bit > VA range, or doing something like the Tango layer patches that on > syscall entry changes the syscall to a "compat" syscall. > compat syscall flag like Tango might be nicer here? Not sure how that flag is best encoded, but yes, it would have to be somewhere that arch_get_unmapped_area() and arch_get_mmap_end() can find. Clearly we want a solution that works for both tango and for your work, as well as being portable to any architecture. >> > } >> > + >> > +COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, >> > + compat_ulong_t, arg) >> > +{ >> > + return do_ioctl32(fd, cmd, arg); >> > +} >> > + >> > +SYSCALL_DEFINE3(ioctl32, unsigned int, fd, unsigned int, cmd, >> > + compat_ulong_t, arg) >> > +{ >> > + return do_ioctl32(fd, cmd, arg); >> > +} >> >> These two look identical to me, I don't think you need to add a wrapper >> here at all, but can just use the normal compat_sys_ioctl entry point >> unless you want to add a 'flags' argument to control the struct padding. > > > I tried having the dispatch table call directly in to the COMPAT one and > the way things were lining up weren't allowing me to do this. > Since this is a bit unique in how it operates, I'm not quite sure if there is > another example I could pull from for this. For the asm-generic/unistd.h, you should be able to write htis as #if __BITS_PER_LONG == 64 __SC_COMP(__NR_ioctl32, compat_sys_ioctl, sys_ni_syscall) #endif Which means that the native syscall in a 64-bit process always points to compat_sys_ioctl, while a 32-bit process always gets -ENOSYS. Similarly, the syscall_64.tbl file on x86 and the other 64-bit architectures would use 442 64 ioctl32 compat_sys_ioctl FWIW, I suppose you can rename compat_sys_ioctl to sys_ioctl32 treewide, if that name makes more sense. Arnd ``` ## Re: \[PATCH\] Adds a new ioctl32 syscall for backwards compatibility layers - From: Andy Lutomirski \ - Date: 2021-01-16 00:08:58 - Message: https://ratatoskr.run/linux-api/2021/01/197457.md ```text On Fri, Jan 15, 2021 at 1:03 AM Arnd Bergmann [ref] wrote: > > On Fri, Jan 15, 2021 at 3:06 AM Ryan Houdek [ref] wrote: > > On Wed, Jan 6, 2021 at 12:49 AM Arnd Bergmann [ref] wrote: > >> On Wed, Jan 6, 2021 at 7:48 AM [ref] wrote: > >> > From: Ryan Houdek > >> ... > >> > >> For x86, this has another complication, as some ioctls also need to > >> check whether they are in an ia32 task (with packed u64 and 32-bit > >> __kernel_old_time_t) or an x32 task (with aligned u64 and 64-bit > >> __kernel_old_time_t). If the new syscall gets wired up on x86 as well, > >> you'd need to decide which of the two behaviors you want. > > > > > > I can have a follow-up patch that makes this do ni-syscall on x86_64 since > > we can go through the int 0x80 handler, or x32 handler path and choose > > whichever one there. > > I'd say for consistency > We need to make it crystal clear on x86 what this ioctl does. We have a silly selection of options: - ioctl32() via SYSCALL, x32 bit clear -- presumably does an i386 ioctl? - ioctl32() via SYSCALL, x32 bit set -- this needs to do something clearly documented. - ioctl32() via int80 -- presumably you're not wiring this up In any case, the compat alloc thing should just go away. It's a hack and serves no real purpose. Finally, I'm not convinced that this patch works correctly. We have in_compat_syscall(), and code that uses it may well be reachable from ioctl. I personally would like to see in_compat_syscall() go away, but some other people (Hi, Christoph!) disagree, and usage seems to be increasing, not decreasing. ``` ## Re: \[PATCH\] Adds a new ioctl32 syscall for backwards compatibility layers - From: Christoph Hellwig \ - Date: 2021-01-16 09:08:23 - Message: https://ratatoskr.run/linux-api/2021/01/197462.md ```text On Fri, Jan 15, 2021 at 04:07:46PM -0800, Andy Lutomirski wrote: > Finally, I'm not convinced that this patch works correctly. We have > in_compat_syscall(), and code that uses it may well be reachable from > ioctl. ioctls are the prime user of in_compat_syscall(). > I personally would like to see in_compat_syscall() go away, > but some other people (Hi, Christoph!) disagree, and usage seems to be > increasing, not decreasing. I'm absolutely against it going away. in_compat_syscall helped to remove so much crap compared to the explicit compat syscalls. ``` ## RE: \[PATCH\] Adds a new ioctl32 syscall for backwards compatibility layers - From: David Laight \ - Date: 2021-01-17 18:33:37 - Message: https://ratatoskr.run/linux-api/2021/01/197487.md ```text From: Christoph Hellwig > Sent: 16 January 2021 09:07 ... > > I personally would like to see in_compat_syscall() go away, > > but some other people (Hi, Christoph!) disagree, and usage seems to be > > increasing, not decreasing. > > I'm absolutely against it going away. in_compat_syscall helped to > remove so much crap compared to the explicit compat syscalls. The only other real option is to pass the 'syscall type' explicitly through all the layers into every piece of code that might need it. So passing it as a 'parameter' that is (probably) current->syscall_type does make sense. It might even make sense have separate bits for the required emulations. So you'd have separate bits for '32bit pointers' and '64bit items 32bit aligned' (etc). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ```