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

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

[Cyrill Gorcunov]

The posix_clock_compat_ioctl() is just a clone of posix_clock_ioctl(),
drop the redundance.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/posix-clock.c |   24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

Index: linux-tip.git/kernel/time/posix-clock.c
===================================================================
--- linux-tip.git.orig/kernel/time/posix-clock.c
+++ linux-tip.git/kernel/time/posix-clock.c
@@ -90,26 +90,6 @@ static long posix_clock_ioctl(struct fil
 	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 posi
 	.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)

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

[Thomas Weißschuh]

Hi Cyrill,

On 2025-01-21 01:10:27+0300, Cyrill Gorcunov wrote:
> The posix_clock_compat_ioctl() is just a clone of posix_clock_ioctl(),
> drop the redundance.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/time/posix-clock.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> Index: linux-tip.git/kernel/time/posix-clock.c
> ===================================================================
> --- linux-tip.git.orig/kernel/time/posix-clock.c
> +++ linux-tip.git/kernel/time/posix-clock.c
> @@ -90,26 +90,6 @@ static long posix_clock_ioctl(struct fil
>  	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 posi
>  	.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,

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.

>  };
>  
>  int posix_clock_register(struct posix_clock *clk, struct device *dev)


From 9d59a26da95ad416710c152dc87ee41f08933c2a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <linux@weissschuh.net>
Date: Fri, 3 Jan 2025 18:04:56 +0100
Subject: [PATCH] posix-clock: Fix compat ioctls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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().

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>
---
 drivers/ptp/ptp_chardev.c   | 18 ++++++++++++++++++
 drivers/ptp/ptp_clock.c     |  1 +
 drivers/ptp/ptp_private.h   |  7 +++++++
 include/linux/posix-clock.h |  3 +++
 kernel/time/posix-clock.c   |  4 ++--
 5 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index ea96a14d72d1..704af620c117 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2010 OMICRON electronics GmbH
  */
+#include <linux/compat.h>
 #include <linux/module.h>
 #include <linux/posix-clock.h>
 #include <linux/poll.h>
@@ -507,6 +508,23 @@ long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	return err;
 }
 
+#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);
+	}
+
+	return ptp_ioctl(pccontext, cmd, arg);
+}
+#endif
+
 __poll_t ptp_poll(struct posix_clock_context *pccontext, struct file *fp,
 		  poll_table *wait)
 {
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 77a36e7bddd5..dec84b81cedf 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -180,6 +180,7 @@ static struct posix_clock_operations ptp_clock_ops = {
 	.clock_getres	= ptp_clock_getres,
 	.clock_settime	= ptp_clock_settime,
 	.ioctl		= ptp_ioctl,
+	.compat_ioctl	= ptp_compat_ioctl,
 	.open		= ptp_open,
 	.release	= ptp_release,
 	.poll		= ptp_poll,
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..13999a7af47a 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -133,6 +133,13 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 long ptp_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
 	       unsigned long arg);
 
+#ifdef CONFIG_COMPAT
+long ptp_compat_ioctl(struct posix_clock_context *pccontext, unsigned int cmd,
+		      unsigned long arg);
+#else
+#define ptp_compat_ioctl NULL
+#endif
+
 int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode);
 
 int ptp_release(struct posix_clock_context *pccontext);
diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index ef8619f48920..8bdc7aec5f79 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -54,6 +54,9 @@ struct posix_clock_operations {
 	long (*ioctl)(struct posix_clock_context *pccontext, unsigned int cmd,
 		      unsigned long arg);
 
+	long (*compat_ioctl)(struct posix_clock_context *pccontext, unsigned int cmd,
+			     unsigned long arg);
+
 	int (*open)(struct posix_clock_context *pccontext, fmode_t f_mode);
 
 	__poll_t (*poll)(struct posix_clock_context *pccontext, struct file *file,
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index 1af0bb2cc45c..63184d92ef13 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -101,8 +101,8 @@ static long posix_clock_compat_ioctl(struct file *fp,
 	if (!clk)
 		return -ENODEV;
 
-	if (clk->ops.ioctl)
-		err = clk->ops.ioctl(pccontext, cmd, arg);
+	if (clk->ops.compat_ioctl)
+		err = clk->ops.compat_ioctl(pccontext, cmd, arg);
 
 	put_posix_clock(clk);

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

[Cyrill Gorcunov]

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?

```
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^^^

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

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

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

[Cyrill Gorcunov]

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^^^

Hi Thomas!

> 
> 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).

> 
> 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!

	Cyrill

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

[Thomas Weißschuh]

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

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

[Cyrill Gorcunov]

On Tue, Jan 21, 2025 at 01:48:23PM +0100, Thomas Weißschuh wrote:
...
> > 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.

Heh, indeed, I managed to forget that s390 clears the top bit and been scratching
my head thinking of how losing the sign bit is possible)

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

The correct solution is better, without a doubt. Thanks for looking into the
patch, Thomas!