* [PATCH 1/2] rtc: mips: default to rtc-cmos on mips @ 2018-08-27 20:03 Arnd Bergmann 2018-08-27 20:03 ` [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c Arnd Bergmann 0 siblings, 1 reply; 4+ messages in thread From: Arnd Bergmann @ 2018-08-27 20:03 UTC (permalink / raw) To: linux-rtc Cc: a.zummo, alexandre.belloni, keguang.zhang, y2038, Arnd Bergmann, Ralf Baechle, Paul Burton, James Hogan, Greg Kroah-Hartman, Masahiro Yamada, Matt Redfearn, David Daney, linux-mips, linux-kernel The old rtc driver is getting in the way of some compat_ioctl simplification. Looking up the loongson64 git history, it seems that everyone uses the more modern but compatible RTC_CMOS driver anyway, so let's remove the special case for loongson64. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/mips/Kconfig | 2 +- drivers/char/Kconfig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 35511999156a..c695825d9377 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -75,7 +75,7 @@ config MIPS select MODULES_USE_ELF_RELA if MODULES && 64BIT select MODULES_USE_ELF_REL if MODULES select PERF_USE_VMALLOC - select RTC_LIB if !MACH_LOONGSON64 + select RTC_LIB select SYSCTL_EXCEPTION_TRACE select VIRT_TO_BUS diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index ce277ee0a28a..131b4c300050 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -268,7 +268,7 @@ if RTC_LIB=n config RTC tristate "Enhanced Real Time Clock Support (legacy PC RTC driver)" - depends on ALPHA || (MIPS && MACH_LOONGSON64) + depends on ALPHA ---help--- If you say Y here and create a character special file /dev/rtc with major number 10 and minor number 135 using mknod ("man mknod"), you -- 2.18.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c 2018-08-27 20:03 [PATCH 1/2] rtc: mips: default to rtc-cmos on mips Arnd Bergmann @ 2018-08-27 20:03 ` Arnd Bergmann 2018-08-27 20:21 ` Al Viro 0 siblings, 1 reply; 4+ messages in thread From: Arnd Bergmann @ 2018-08-27 20:03 UTC (permalink / raw) To: linux-rtc Cc: a.zummo, alexandre.belloni, keguang.zhang, y2038, Arnd Bergmann, Alexander Viro, Kees Cook, Andrew Morton, Greg Kroah-Hartman, linux-kernel, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c 2018-08-27 20:03 ` [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c Arnd Bergmann @ 2018-08-27 20:21 ` Al Viro 2018-08-28 8:10 ` Arnd Bergmann 0 siblings, 1 reply; 4+ messages in thread From: Al Viro @ 2018-08-27 20:21 UTC (permalink / raw) To: Arnd Bergmann Cc: linux-rtc, a.zummo, alexandre.belloni, keguang.zhang, y2038, Kees Cook, Andrew Morton, Greg Kroah-Hartman, linux-kernel, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c 2018-08-27 20:21 ` Al Viro @ 2018-08-28 8:10 ` Arnd Bergmann 0 siblings, 0 replies; 4+ messages in thread From: Arnd Bergmann @ 2018-08-28 8:10 UTC (permalink / raw) To: Al Viro Cc: linux-rtc, Alessandro Zummo, Alexandre Belloni, keguang.zhang, y2038 Mailman List, Kees Cook, Andrew Morton, gregkh, Linux Kernel Mailing List, Linux FS-devel Mailing List 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-28 8:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-27 20:03 [PATCH 1/2] rtc: mips: default to rtc-cmos on mips Arnd Bergmann 2018-08-27 20:03 ` [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c Arnd Bergmann 2018-08-27 20:21 ` Al Viro 2018-08-28 8:10 ` Arnd Bergmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).