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

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

Hi Cyrill!

On 2025-01-21 09:48:28+0300, Cyrill Gorcunov wrote:
> On Mon, Jan 20, 2025 at 11:41:26PM +0100, Thomas Weißschuh wrote:
> > > 
> > > > +#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().
> 
> Yeah, and the PTP_ENABLE_PPS/PTP_ENABLE_PPS2 consider `arg` as 0/1 flip-flop
> so compat_ptr won't screw it. So I personally would rather stick with a more
> simple code (taking into account that ptp is the only real underlied device
> so far sitting in code for so long).

It is valid to pass any value to these ioctls, not only booleans.
On s390 the value 0x80000000 aka BIT(31) would interpreted as "true" by
a native 32bit kernel and "false" by a 64bit kernel in compat mode.

It's indeed an edge case.
Personally I prefer the correct solution.

> > 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.
> 
> Sure, I see your point. Thanks for comments!

Let's see what the maintainers prefer :-)


Thomas