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

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

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