Re: [PATCH] posix-clock: Explicitly handle compat ioctls

["Thomas Weißschuh" <linux@weissschuh.net>]

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.localdomain6/
> 
> 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