On Wed, Jan 22, 2025, at 17:23, 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.
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?
The original review comment was about the complex argument transformation that is needed on architectures other than s390, which thankfully still works fine.
A lot of times we can ignore the s390 problem, and there are many drivers that still do, mainly because s390 has a very limited set of device drivers it actually uses, and also because 32-bit userspace is getting very rare on that architecture.
To do things correctly on all architectures, it's usually sufficient to just use compat_ptr_ioctl(), as in:
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 1af0bb2cc45c..e64d37f221b5 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -90,26 +90,6 @@ static long posix_clock_ioctl(struct file *fp, return err; }
-#ifdef CONFIG_COMPAT -static long posix_clock_compat_ioctl(struct file *fp, - unsigned int cmd, unsigned long arg) -{ - struct posix_clock_context *pccontext = fp->private_data; - struct posix_clock *clk = get_posix_clock(fp); - int err = -ENOTTY; - - if (!clk) - return -ENODEV; - - if (clk->ops.ioctl) - err = clk->ops.ioctl(pccontext, cmd, arg); - - put_posix_clock(clk); - - return err; -} -#endif - static int posix_clock_open(struct inode *inode, struct file *fp) { int err; @@ -173,9 +153,7 @@ static const struct file_operations posix_clock_file_operations = { .unlocked_ioctl = posix_clock_ioctl, .open = posix_clock_open, .release = posix_clock_release, -#ifdef CONFIG_COMPAT - .compat_ioctl = posix_clock_compat_ioctl, -#endif + .compat_ioctl = compat_ptr_ioctl, };
int posix_clock_register(struct posix_clock *clk, struct device *dev)
but this was only added in 2018 and was not available in your original version. Unfortunately this only works if 'arg' is always a pointer or a nonnegative integer less than 2^31. If the argument can be a negative integer, it's actually broken on all architectures because the current code performs a zero-extension when it should be doing a sign-extension.
A simpler variant of the patch would move the switch/case logic into posix_clock_compat_ioctl() and avoid the extra function pointer, simply calling posix_clock_ioctl() with the modified argument.
Arnd