From patchwork Sat May 16 19:54:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnd Bergmann X-Patchwork-Id: 6769 Received: (qmail 58758 invoked by alias); 16 May 2015 19:54:37 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 58692 invoked by uid 89); 16 May 2015 19:54:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.9 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 X-HELO: mout.kundenserver.de From: Arnd Bergmann To: Thomas Gleixner Cc: y2038@lists.linaro.org, baolin.wang@linaro.org, albert.aribaud@3adev.fr, john.stultz@linaro.org, bamvor.zhangjian@linaro.org, ruchandani.tina@gmail.com, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, libc-alpha@sourceware.org Subject: Re: [PATCH 13/19] y2038: add compat handling for sys_semtimedop Date: Sat, 16 May 2015 21:54:25 +0200 Message-ID: <5686498.JnFHvQTH21@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: References: <1430929826-318934-1-git-send-email-arnd@arndb.de> <1430929826-318934-14-git-send-email-arnd@arndb.de> MIME-Version: 1.0 X-UI-Out-Filterresults: notjunk:1; On Saturday 16 May 2015 00:46:44 Thomas Gleixner wrote: > On Wed, 6 May 2015, Arnd Bergmann wrote: > > +SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, > > + unsigned, nsops, > > + const struct __kernel_timespec __user *, timeout) > > +{ > > + unsigned long jiffies_left = 0; > > + > > + if (timeout) { > > + struct timespec64 _timeout; > > + if (get_timespec64(&_timeout, timeout)) > > Moo. I had to look 3 times to get not confused by the extra > underscore. What's wrong with a proper variable name which is easy to > distinguish? > > > + return -EFAULT; > > > + if (_timeout.tv_sec < 0 || _timeout.tv_nsec < 0 || > > + _timeout.tv_nsec >= 1000000000L) > > + return -EINVAL; > > We have proper helper functions to validate time specs. I ended up fixing both issues you noticed in the same patch after all, and also simplified it slightly more. Finally, I also noticed that I had not done a timespec64_to_jiffies() call at the time when I wrote this patch, but it actually exists now, so I've reordered my series and am using it in the new version, as I should have done to start with. Arnd 8<---- From e04b14d49273c27d92f1799233b82bcdafb43d9a Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Mon, 27 Apr 2015 23:30:39 +0200 Subject: [UPDATED PATCH] y2038: add compat handling for sys_semtimedop This moves the compat_sys_semtimedop function to ipc/sem.c so it can be shared with 32-bit architectures efficiently. Instead of copying the timespec back to user space, we take a shortcut and pass the kernel timespec64 value to the low-level implementation directly. The native sys_semtimedop() function is modified to take a __kernel_timespec structure, which will be based on a 64-bit time_t in the future. There is a small API change here: if multiple errors are present, and the timespec argument is an invalid pointer, we now return -EFAULT before checking any of the other error conditions. This is what the compat version has always done, but if it is a problem, we need a more sophisticated approach. Signed-off-by: Arnd Bergmann Reviewed-by: Thomas Gleixner diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index f3fdc312627b..c2a70a8f907d 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -665,7 +665,7 @@ asmlinkage long sys_semop(int semid, struct sembuf __user *sops, asmlinkage long sys_semctl(int semid, int semnum, int cmd, unsigned long arg); asmlinkage long sys_semtimedop(int semid, struct sembuf __user *sops, unsigned nsops, - const struct timespec __user *timeout); + const struct __kernel_timespec __user *timeout); asmlinkage long sys_shmat(int shmid, char __user *shmaddr, int shmflg); asmlinkage long sys_shmget(key_t key, size_t size, int flag); asmlinkage long sys_shmdt(char __user *shmaddr); diff --git a/ipc/compat.c b/ipc/compat.c index 9b3c85f8a538..2bbdb093d1be 100644 --- a/ipc/compat.c +++ b/ipc/compat.c @@ -745,13 +745,3 @@ COMPAT_SYSCALL_DEFINE3(shmctl, int, first, int, second, void __user *, uptr) } return err; } - -COMPAT_SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsems, - unsigned, nsops, - const struct compat_timespec __user *, timeout) -{ - struct timespec __user *ts64; - if (compat_convert_timespec(&ts64, timeout)) - return -EFAULT; - return sys_semtimedop(semid, tsems, nsops, ts64); -} diff --git a/ipc/sem.c b/ipc/sem.c index d1a6edd17eba..84d354a34df3 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -72,6 +72,7 @@ * The worst-case behavior is nevertheless O(N^2) for N wakeups. */ +#include #include #include #include @@ -1779,8 +1780,8 @@ static int get_queue_result(struct sem_queue *q) return error; } -SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, - unsigned, nsops, const struct timespec __user *, timeout) +static long semtimedop(int semid, struct sembuf __user * tsops, + unsigned nsops, struct timespec64 *timeout) { int error = -EINVAL; struct sem_array *sma; @@ -1809,17 +1810,11 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, goto out_free; } if (timeout) { - struct timespec _timeout; - if (copy_from_user(&_timeout, timeout, sizeof(*timeout))) { - error = -EFAULT; - goto out_free; - } - if (_timeout.tv_sec < 0 || _timeout.tv_nsec < 0 || - _timeout.tv_nsec >= 1000000000L) { + if (!timespec64_valid(timeout)) { error = -EINVAL; goto out_free; } - jiffies_left = timespec_to_jiffies(&_timeout); + jiffies_left = timespec64_to_jiffies(timeout); } max = 0; for (sop = sops; sop < sops + nsops; sop++) { @@ -2014,10 +2009,36 @@ out_free: return error; } +SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, + unsigned, nsops, + const struct __kernel_timespec __user *, p) +{ + struct timespec64 timeout; + + if (p && get_timespec64(&timeout, p)) + return -EFAULT; + + return semtimedop(semid, tsops, nsops, p ? &timeout : NULL); +} + +#ifdef CONFIG_COMPAT_TIME +COMPAT_SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops, + unsigned, nsops, + const struct compat_timespec __user *, p) +{ + struct timespec64 timeout; + + if (p && compat_get_timespec64(&timeout, p)) + return -EFAULT; + + return semtimedop(semid, tsops, nsops, p ? &timeout : NULL); +} +#endif + SYSCALL_DEFINE3(semop, int, semid, struct sembuf __user *, tsops, unsigned, nsops) { - return sys_semtimedop(semid, tsops, nsops, NULL); + return semtimedop(semid, tsops, nsops, NULL); } /* If CLONE_SYSVSEM is set, establish sharing of SEM_UNDO state between diff --git a/ipc/syscall.c b/ipc/syscall.c index 52429489cde0..d7b17355d870 100644 --- a/ipc/syscall.c +++ b/ipc/syscall.c @@ -7,6 +7,7 @@ #include #ifdef __ARCH_WANT_SYS_IPC +#include #include #include #include @@ -26,9 +27,15 @@ SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, unsigned long, second, return sys_semtimedop(first, (struct sembuf __user *)ptr, second, NULL); case SEMTIMEDOP: +#if defined(CONFIG_ARCH_HAS_COMPAT_TIME) && !defined(CONFIG_64BIT) + return compat_sys_semtimedop(first, (struct sembuf __user *)ptr, + second, + (const struct compat_timespec __user *)fifth); +#else return sys_semtimedop(first, (struct sembuf __user *)ptr, second, (const struct timespec __user *)fifth); +#endif case SEMGET: return sys_semget(first, second, third);