On Wed, Nov 20, 2019 at 11:10 PM Ben Hutchings
<ben.hutchings@xxxxxxxxxxxxxxx> wrote:
> On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
> > On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote:
> > -
> > return lp_set_timeout(minor, karg[0], karg[1]);
> > }
> >
> > +static int lp_set_timeout(unsigned int minor, void __user *arg)
>
> That function name is already used! Maybe this should be
> lp_set_timeout_old()?
Yes, that's what I used after actually compile-testing and running
into a couple of issues with my draft.
> > @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
> > mutex_lock(&lp_mutex);
> > switch (cmd) {
> > case LPSETTIMEOUT_OLD:
> > - if (BITS_PER_LONG == 32) {
> > - ret = lp_set_timeout32(minor, (void __user *)arg);
> > - break;
> > - }
> > - /* fall through - for 64-bit */
> > + ret = lp_set_timeout(minor, (void __user *)arg);
> > + break;
> > case LPSETTIMEOUT_NEW:
> > ret = lp_set_timeout64(minor, (void __user *)arg);
> > break;
> >
> > Do you like that better?
>
> Yes. Aside from the duplicate function name, it looks correct and
> cleaner than the current version.
As Greg has already merged the original patch, and that version works
just as well, I'd probably just leave what I did at first. One benefit is
that in case we decide to kill off sparc64 support before drivers/char/lp.c,
the special case can be removed more easily.
I don't think either of them is going any time soon, but working on y2038
patches has made me think ahead longer term ;-)
If you still think we should change it I can send the below patch (now
actually build-tested) with your Ack.
Arnd
---
commit 93efbb1768a5071a0a98bf4627f0104075cf83a6 (HEAD -> y2038)
Author: Arnd Bergmann <arnd@xxxxxxxx>
Date: Thu Nov 21 14:45:14 2019 +0100
lp: clean up set_timeout handling
As Ben Hutchings noticed, we can avoid the special case for sparc64
by dealing with '__kernel_old_timeval' arguments separately from
the fixed-length 32-bit and 64-bit arguments.
Note that the behavior for LPSETTIMEOUT_NEW changes on sparc64 to
expect the same argument as other architectures, but this is ok
because sparc64 users would pass LPSETTIMEOUT_OLD anyway.
Suggested-by: Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx>
Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index bd95aba1f9fe..cc17d5a387c5 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -696,14 +696,14 @@ static int lp_set_timeout(unsigned int minor,
s64 tv_sec, long tv_usec)
return 0;
}
-static int lp_set_timeout32(unsigned int minor, void __user *arg)
+static int lp_set_timeout_old(unsigned int minor, void __user *arg)
{
- s32 karg[2];
+ struct __kernel_old_timeval tv;
- if (copy_from_user(karg, arg, sizeof(karg)))
+ if (copy_from_user(&tv, arg, sizeof(tv)))
return -EFAULT;
- return lp_set_timeout(minor, karg[0], karg[1]);
+ return lp_set_timeout(minor, tv.tv_sec, tv.tv_usec);
}
static int lp_set_timeout64(unsigned int minor, void __user *arg)
@@ -713,10 +713,6 @@ static int lp_set_timeout64(unsigned int minor,
void __user *arg)
if (copy_from_user(karg, arg, sizeof(karg)))
return -EFAULT;
- /* sparc64 suseconds_t is 32-bit only */
- if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
- karg[1] >>= 32;
-
return lp_set_timeout(minor, karg[0], karg[1]);
}
@@ -730,11 +726,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
mutex_lock(&lp_mutex);
switch (cmd) {
case LPSETTIMEOUT_OLD:
- if (BITS_PER_LONG == 32) {
- ret = lp_set_timeout32(minor, (void __user *)arg);
- break;
- }
- /* fall through - for 64-bit */
+ ret = lp_set_timeout_old(minor, (void __user *)arg);
+ break;
case LPSETTIMEOUT_NEW:
ret = lp_set_timeout64(minor, (void __user *)arg);
break;
@@ -748,6 +741,16 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
}
#ifdef CONFIG_COMPAT
+static int lp_set_timeout32(unsigned int minor, void __user *arg)
+{
+ s32 karg[2];
+
+ if (copy_from_user(karg, arg, sizeof(karg)))
+ return -EFAULT;
+
+ return lp_set_timeout(minor, karg[0], karg[1]);
+}
+
static long lp_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
