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@xxxxxxxxxxxxxxxxxx> 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