[PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c

[PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c

[Arnd Bergmann]

We no longer need the rtc compat handling to be in common code, now that
all drivers are either moved to the rtc-class framework, or (rarely)
exist in drivers/char for architectures without compat mode (m68k,
alpha and ia64, respectively).

I checked the list of ioctl commands in drivers, and the ones that are
not already handled are all compatible, again with the one exception of
m68k driver, which implements RTC_PLL_GET and RTC_PLL_SET, but has no
compat mode.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/rtc/rtc-dev.c | 50 +++++++++++++++++++++++++++++++++++++++
 fs/compat_ioctl.c     | 54 -------------------------------------------
 2 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 43d962a9c210..50b03e175263 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/compat.h>
 #include <linux/module.h>
 #include <linux/rtc.h>
 #include <linux/sched/signal.h>
@@ -405,6 +406,52 @@ static long rtc_dev_ioctl(struct file *file,
 	return err;
 }
 
+#ifdef CONFIG_COMPAT
+#define RTC_IRQP_READ32		_IOR('p', 0x0b, compat_ulong_t)
+#define RTC_IRQP_SET32		_IOW('p', 0x0c, compat_ulong_t)
+#define RTC_EPOCH_READ32	_IOR('p', 0x0d, compat_ulong_t)
+#define RTC_EPOCH_SET32		_IOW('p', 0x0e, compat_ulong_t)
+
+static long rtc_dev_compat_ioctl(struct file *file, unsigned cmd, unsigned long arg)
+{
+	unsigned long __user *valp;
+	compat_ulong_t __user *argp = compat_ptr(arg);
+	unsigned long val;
+	int ret;
+
+	switch (cmd) {
+	/* pointer to 'long' needs translation. */
+	case RTC_IRQP_READ32:
+	case RTC_EPOCH_READ32: {
+		valp = compat_alloc_user_space(sizeof(*valp));
+		if (valp == NULL)
+			return -EFAULT;
+		ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?
+					RTC_IRQP_READ : RTC_EPOCH_READ,
+					(unsigned long)valp);
+		if (ret)
+			return ret;
+		if (get_user(val, valp) || put_user(val, argp))
+			return -EFAULT;
+		return 0;
+	}
+
+	/* arguments are compatible, command number is not */
+	case RTC_IRQP_SET32:
+		return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);
+	case RTC_EPOCH_SET32:
+		return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);
+	}
+
+	/*
+	 * all other rtc ioctls are compatible, or only
+	 * exist on architectures without compat mode
+	 */
+
+	return rtc_dev_ioctl(file, cmd, (unsigned long)argp);
+}
+#endif
+
 static int rtc_dev_fasync(int fd, struct file *file, int on)
 {
 	struct rtc_device *rtc = file->private_data;
@@ -439,6 +486,9 @@ static const struct file_operations rtc_dev_fops = {
 	.read		= rtc_dev_read,
 	.poll		= rtc_dev_poll,
 	.unlocked_ioctl	= rtc_dev_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= rtc_dev_compat_ioctl,
+#endif
 	.open		= rtc_dev_open,
 	.release	= rtc_dev_release,
 	.fasync		= rtc_dev_fasync,
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index a9b00942e87d..a5c86a2fa8e6 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -46,7 +46,6 @@
 #include <linux/raw.h>
 #include <linux/blkdev.h>
 #include <linux/elevator.h>
-#include <linux/rtc.h>
 #include <linux/pci.h>
 #include <linux/serial.h>
 #include <linux/if_tun.h>
@@ -623,37 +622,6 @@ static int serial_struct_ioctl(struct file *file,
         return err;
 }
 
-#define RTC_IRQP_READ32		_IOR('p', 0x0b, compat_ulong_t)
-#define RTC_IRQP_SET32		_IOW('p', 0x0c, compat_ulong_t)
-#define RTC_EPOCH_READ32	_IOR('p', 0x0d, compat_ulong_t)
-#define RTC_EPOCH_SET32		_IOW('p', 0x0e, compat_ulong_t)
-
-static int rtc_ioctl(struct file *file,
-		unsigned cmd, void __user *argp)
-{
-	unsigned long __user *valp = compat_alloc_user_space(sizeof(*valp));
-	int ret;
-
-	if (valp == NULL)
-		return -EFAULT;
-	switch (cmd) {
-	case RTC_IRQP_READ32:
-	case RTC_EPOCH_READ32:
-		ret = do_ioctl(file, (cmd == RTC_IRQP_READ32) ?
-					RTC_IRQP_READ : RTC_EPOCH_READ,
-					(unsigned long)valp);
-		if (ret)
-			return ret;
-		return convert_in_user(valp, (unsigned int __user *)argp);
-	case RTC_IRQP_SET32:
-		return do_ioctl(file, RTC_IRQP_SET, (unsigned long)argp);
-	case RTC_EPOCH_SET32:
-		return do_ioctl(file, RTC_EPOCH_SET, (unsigned long)argp);
-	}
-
-	return -ENOIOCTLCMD;
-}
-
 /* on ia32 l_start is on a 32-bit boundary */
 #if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
 struct space_resv_32 {
@@ -806,21 +774,6 @@ COMPATIBLE_IOCTL(SCSI_IOCTL_GET_PCI)
 /* Big V (don't complain on serial console) */
 IGNORE_IOCTL(VT_OPENQRY)
 IGNORE_IOCTL(VT_GETMODE)
-/* Little p (/dev/rtc, /dev/envctrl, etc.) */
-COMPATIBLE_IOCTL(RTC_AIE_ON)
-COMPATIBLE_IOCTL(RTC_AIE_OFF)
-COMPATIBLE_IOCTL(RTC_UIE_ON)
-COMPATIBLE_IOCTL(RTC_UIE_OFF)
-COMPATIBLE_IOCTL(RTC_PIE_ON)
-COMPATIBLE_IOCTL(RTC_PIE_OFF)
-COMPATIBLE_IOCTL(RTC_WIE_ON)
-COMPATIBLE_IOCTL(RTC_WIE_OFF)
-COMPATIBLE_IOCTL(RTC_ALM_SET)
-COMPATIBLE_IOCTL(RTC_ALM_READ)
-COMPATIBLE_IOCTL(RTC_RD_TIME)
-COMPATIBLE_IOCTL(RTC_SET_TIME)
-COMPATIBLE_IOCTL(RTC_WKALM_SET)
-COMPATIBLE_IOCTL(RTC_WKALM_RD)
 /*
  * These two are only for the sbus rtc driver, but
  * hwclock tries them on every rtc device first when
@@ -1297,13 +1250,6 @@ static long do_ioctl_trans(unsigned int cmd,
 	case TIOCGSERIAL:
 	case TIOCSSERIAL:
 		return serial_struct_ioctl(file, cmd, argp);
-	/* Not implemented in the native kernel */
-	case RTC_IRQP_READ32:
-	case RTC_IRQP_SET32:
-	case RTC_EPOCH_READ32:
-	case RTC_EPOCH_SET32:
-		return rtc_ioctl(file, cmd, argp);
-
 	/* dvb */
 	case VIDEO_GET_EVENT:
 		return do_video_get_event(file, cmd, argp);
-- 
2.18.0

Re: [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c

[Al Viro]

On Mon, Aug 27, 2018 at 10:03:09PM +0200, Arnd Bergmann wrote:

> +#ifdef CONFIG_COMPAT
> +#define RTC_IRQP_READ32		_IOR('p', 0x0b, compat_ulong_t)
> +#define RTC_IRQP_SET32		_IOW('p', 0x0c, compat_ulong_t)
> +#define RTC_EPOCH_READ32	_IOR('p', 0x0d, compat_ulong_t)
> +#define RTC_EPOCH_SET32		_IOW('p', 0x0e, compat_ulong_t)
> +
> +static long rtc_dev_compat_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> +{
> +	unsigned long __user *valp;
> +	compat_ulong_t __user *argp = compat_ptr(arg);
> +	unsigned long val;
> +	int ret;
> +
> +	switch (cmd) {
> +	/* pointer to 'long' needs translation. */
> +	case RTC_IRQP_READ32:
> +	case RTC_EPOCH_READ32: {
> +		valp = compat_alloc_user_space(sizeof(*valp));
> +		if (valp == NULL)
> +			return -EFAULT;
> +		ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?
> +					RTC_IRQP_READ : RTC_EPOCH_READ,
> +					(unsigned long)valp);
> +		if (ret)
> +			return ret;
> +		if (get_user(val, valp) || put_user(val, argp))
> +			return -EFAULT;

	No.  With the side of Hell, No.  This is bloody ridiculous - take a look
at rtc_dev_ioctl().  Seriously.  Relevant bits:
        struct rtc_device *rtc = file->private_data;
        const struct rtc_class_ops *ops = rtc->ops;
        struct rtc_time tm;
        struct rtc_wkalrm alarm;
        void __user *uarg = (void __user *) arg;

        err = mutex_lock_interruptible(&rtc->ops_lock);
        if (err)
                return err;

                err = put_user(rtc->irq_freq, (unsigned long __user *)uarg);

        mutex_unlock(&rtc->ops_lock);
        return err;

That's RTC_IRQP_READ.  IOW, all that song and dance starting with
compat_alloc_user_space() is just a way to get to rtc->irq_freq value.


RTC_EPOCH_READ is in the bowels of individual implementations, but it's
otherwise very similar; add a method returning the damn thing (e.g.
rtc->ops->get_epoch(rtc)) and lift its call into rtc_dev_ioctl(), then
this nonsense will go away as well.

> +		return 0;
> +	}
> +
> +	/* arguments are compatible, command number is not */
> +	case RTC_IRQP_SET32:
> +		return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);
ITYM
		cmd = RTC_IRQP_SET;
		break;
> +	case RTC_EPOCH_SET32:
... and
		cmd = RTC_EPOCH_SET;
		break;
here.
> +		return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);
> +	}
> +
> +	/*
> +	 * all other rtc ioctls are compatible, or only
> +	 * exist on architectures without compat mode
> +	 */
> +
> +	return rtc_dev_ioctl(file, cmd, (unsigned long)argp);
> +}

compat_alloc_user_space() is usually papering over bogosities like
that; if you are moving something out to individual drivers, it's
always a red flag - most of the time it'll turn out to be pointless.

Re: [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c

[Arnd Bergmann]

On Mon, Aug 27, 2018 at 10:21 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Aug 27, 2018 at 10:03:09PM +0200, Arnd Bergmann wrote:
> > +     case RTC_IRQP_READ32:
> > +     case RTC_EPOCH_READ32: {
> > +             valp = compat_alloc_user_space(sizeof(*valp));
> > +             if (valp == NULL)
> > +                     return -EFAULT;
> > +             ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?
> > +                                     RTC_IRQP_READ : RTC_EPOCH_READ,
> > +                                     (unsigned long)valp);
> > +             if (ret)
> > +                     return ret;
> > +             if (get_user(val, valp) || put_user(val, argp))
> > +                     return -EFAULT;
>
>         No.  With the side of Hell, No.  This is bloody ridiculous - take a look
> at rtc_dev_ioctl().  Seriously.  Relevant bits:
>         struct rtc_device *rtc = file->private_data;
>         const struct rtc_class_ops *ops = rtc->ops;
>         struct rtc_time tm;
>         struct rtc_wkalrm alarm;
>         void __user *uarg = (void __user *) arg;
>
>         err = mutex_lock_interruptible(&rtc->ops_lock);
>         if (err)
>                 return err;
>
>                 err = put_user(rtc->irq_freq, (unsigned long __user *)uarg);
>
>         mutex_unlock(&rtc->ops_lock);
>         return err;
>
> That's RTC_IRQP_READ.  IOW, all that song and dance starting with
> compat_alloc_user_space() is just a way to get to rtc->irq_freq value.
>
> RTC_EPOCH_READ is in the bowels of individual implementations, but it's
> otherwise very similar; add a method returning the damn thing (e.g.
> rtc->ops->get_epoch(rtc)) and lift its call into rtc_dev_ioctl(), then
> this nonsense will go away as well.

Okay. I was basically trying to do one step of moving the code
in a more appropriate place without changing it much. The problem
I see with a get_epoch() callback is that we don't actually want more
drivers to implement it. At the moment there is only one (vr41xx),
so how about just moving the compat epoch handling there and
completely leaving it outside of the common rtc-dev implementation?

diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
index 70f013e692b0..05f981f7cd21 100644
--- a/drivers/rtc/rtc-vr41xx.c
+++ b/drivers/rtc/rtc-vr41xx.c
@@ -17,6 +17,7 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
+#include <linux/compat.h>
 #include <linux/err.h>
 #include <linux/fs.h>
 #include <linux/init.h>
@@ -195,6 +196,11 @@ static int vr41xx_rtc_ioctl(struct device *dev,
unsigned int cmd, unsigned long
        switch (cmd) {
        case RTC_EPOCH_READ:
                return put_user(epoch, (unsigned long __user *)arg);
+#ifdef CONFIG_COMPAT
+       case RTC_EPOCH_READ32:
+               return put_user(epoch, (unsigned int __user *)arg);
+       case RTC_EPOCH_SET32:
+#endif
        case RTC_EPOCH_SET:
                /* Doesn't support before 1900 */

> > +             return 0;
> > +     }
> > +
> > +     /* arguments are compatible, command number is not */
> > +     case RTC_IRQP_SET32:
> > +             return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);
> ITYM
>                 cmd = RTC_IRQP_SET;
>                 break;
> > +     case RTC_EPOCH_SET32:
> ... and
>                 cmd = RTC_EPOCH_SET;
>                 break;
> here.

Just to clarify: do you mean I copied a bug from the old code,
or are you objecting to the coding style?

> > +             return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);
> > +     }
> > +
> > +     /*
> > +      * all other rtc ioctls are compatible, or only
> > +      * exist on architectures without compat mode
> > +      */
> > +
> > +     return rtc_dev_ioctl(file, cmd, (unsigned long)argp);
> > +}
>
> compat_alloc_user_space() is usually papering over bogosities like
> that; if you are moving something out to individual drivers, it's
> always a red flag - most of the time it'll turn out to be pointless.

Absolutely agreed. Eliminating compat_alloc_user_space()
was a bit lower on my priority list than killing off do_ioctl_trans()
(which is already fairly low at the bottom), but I've adjusted that
and will send an update.

I have a couple more patches for do_ioctl_trans() that started
out as a side product of my y2038 patches, I'll make sure
to do it the same way for any others. SG_GET_REQUEST_TABLE
was the only other command for which I chose to keep
compat_alloc_user_space() after my attempt to kill it ended
up in way more complexity, but I can also revisit that, or maybe
drop that patch until I get motivated to also tackle SG_IO.

     Arnd