Re: [PATCH] posix-clock: drop code duplication using compat_ptr_ioctl

[Thomas Weißschuh]

Hi Cyrill,

On 2025-01-21 01:30:11+0300, Cyrill Gorcunov wrote:
> On Mon, Jan 20, 2025 at 11:22:52PM +0100, Thomas Weißschuh wrote:
> ...
> > > -#ifdef CONFIG_COMPAT
> > > -	.compat_ioctl	= posix_clock_compat_ioctl,
> > > -#endif
> > > +	.compat_ioctl	= compat_ptr_ioctl,
> > 
> > This is not correct on s390. (It wasn't before either, though)
> > The improved patch below is in my personal queue, but I didn't get
> > around to actually testing and submitting it yet.
> 
> Hi, Thomas! Thanks for looking into the patch! I somehow miss why it won't
> work 'cause compat_ptr_ioctl already does the same conversion as in your
> code below, no? I miss something obvious?

See below.

> 
> ```
> long compat_ptr_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> 	if (!file->f_op->unlocked_ioctl)
> 		return -ENOIOCTLCMD;
> 
> 	return file->f_op->unlocked_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> }
> EXPORT_SYMBOL(compat_ptr_ioctl);
> ```
> 
> > +#ifdef CONFIG_COMPAT
> > +long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
> > +		      unsigned long arg)
> > +{
> > +	switch (cmd) {
> > +	case PTP_ENABLE_PPS:
> > +	case PTP_ENABLE_PPS2:
> > +		/* These take in scalar arg, do not convert */
> > +		break;
> > +	default:
> > +		arg = (unsigned long)compat_ptr(arg);
> 
> Here^^^

The key is to only call compat_ptr() on *pointers*.
Scalars have to be passed through unmodified.
For ptp_ioctl(), PTP_ENABLE_PPS and PTP_ENABLE_PPS2 take such scalars,
which is why those two *can not* use compat_ptr().
compat_ptr_ioctl() however passes all arguments through compat_ptr().

Admittedly it's quite unlikely anybody would pass a value where it would
make a difference in practice. But if we fix this now, it might as well
be correct.

> 
> > +	}
> > +
> > +	return ptp_ioctl(pccontext, cmd, arg);
> > +}
> > +#endif