On Wed, Nov 20, 2019 at 11:10 PM Ben Hutchings ben.hutchings@codethink.co.uk 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@codethink.co.uk 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@arndb.de 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@codethink.co.uk Signed-off-by: Arnd Bergmann arnd@arndb.de
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) {