On 2025-01-22 08:23:35-0800, Richard Cochran wrote:
On Wed, Jan 22, 2025 at 08:30:51AM +0100, Arnd Bergmann wrote:
On Tue, Jan 21, 2025, at 23:41, Thomas Weißschuh wrote:
Pointer arguments passed to ioctls need to pass through compat_ptr() to work correctly on s390; as explained in Documentation/driver-api/ioctl.rst. Plumb the compat_ioctl callback through 'struct posix_clock_operations' and handle the different ioctls cmds in the new ptp_compat_ioctl().
Using compat_ptr_ioctl is not possible. For the commands PTP_ENABLE_PPS/PTP_ENABLE_PPS2 on s390 it would corrupt the argument 0x80000000, aka BIT(31) to zero.
Fixes: 0606f422b453 ("posix clocks: Introduce dynamic clocks") Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.") Cc: stable@vger.kernel.org Signed-off-by: Thomas Weißschuh linux@weissschuh.net
This looks correct to me,
I'm not familiar with s390, but I can remember that the compat ioctl was nixed during review.
From Documentation/driver-api/ioctl.rst:
compat_ptr() ------------
On the s390 architecture, 31-bit user space has ambiguous representations for data pointers, with the upper bit being ignored. When running such a process in compat mode, the compat_ptr() helper must be used to clear the upper bit of a compat_uptr_t and turn it into a valid 64-bit pointer. On other architectures, this macro only performs a cast to a ``void __user *`` pointer.
In an compat_ioctl() callback, the last argument is an unsigned long, which can be interpreted as either a pointer or a scalar depending on the command. If it is a scalar, then compat_ptr() must not be used, to ensure that the 64-bit kernel behaves the same way as a 32-bit kernel for arguments with the upper bit set.
The compat_ptr_ioctl() helper can be used in place of a custom compat_ioctl file operation for drivers that only take arguments that are pointers to compatible data structures.
TLDR:
If the argument is a pointer, pass it through compat_ptr(). If the argument is a scalar, *do not* pass it through compat_ptr().
If all ioctls handled by a struct file_operations::unlocked_ioctl are pointers, use .compat_ioctl = compat_ptr_ioctl. If all ioctls handled by a struct file_operations::unlocked_ioctl are scalars, use .compat_ioctl = .unlocked_ioctl. If it's mixed, add a custom handler that knows the details, like this patch does.
This all assumes the actual data structures are compatible between compat and non-compat, which is the case here. If they are not compatible those also need to be converted around.
https://lore.kernel.org/lkml/201012161716.42520.arnd@arndb.de/
https://lore.kernel.org/lkml/alpine.LFD.2.00.1012161939340.12146@localhost6....
Can you explain what changed or what was missed?
From your link:
* I would recommend starting without a compat_ioctl operation if you can. Just mandate that all ioctls for posix clocks are compatible and call fops->ioctl from the posix_clock_compat_ioctl() function. If you ever need compat_ioctl handling, it can still be added later.
The fact that compat_ptr() is necessary for pointer arguments was missed. And because there are two commands with scalar arguments, compat_ptr_ioctl() can't be used.
Thomas