Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652 ("staging: android: ashmem: Fix a race condition in pin ioctls") causing deadlock.
No need to hold ashmem_mutex while copying from user
Stacks are:
ashmem_mmap+0x53/0x400 drivers/staging/android/ashmem.c:379 mmap_region+0x7dd/0xfd0 mm/mmap.c:1694 do_mmap+0x57b/0xbe0 mm/mmap.c:1473
and
lock_acquire+0x12e/0x410 kernel/locking/lockdep.c:3756 __might_fault+0x14a/0x1d0 mm/memory.c:4014 copy_from_user arch/x86/include/asm/uaccess.h:705 [inline] ashmem_pin_unpin drivers/staging/android/ashmem.c:719 [inline]
Signed-off-by: Paul Lawrence paullawrence@google.com Cc: stable@vger.kernel.org # 4.9.x Cc: stable@vger.kernel.org # 4.4.x Cc: stable@vger.kernel.org # 3.18.x: ce8a3a9e76d01 Cc: stable@vger.kernel.org # 3.18.x Cc: Ben Hutchings ben@decadent.org.uk --- drivers/staging/android/ashmem.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 6dbba5aff191..8c55706c2cfa 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -702,16 +702,14 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, size_t pgstart, pgend; int ret = -EINVAL;
+ if (unlikely(copy_from_user(&pin, p, sizeof(pin)))) + return -EFAULT; + mutex_lock(&ashmem_mutex);
if (unlikely(!asma->file)) goto out_unlock;
- if (unlikely(copy_from_user(&pin, p, sizeof(pin)))) { - ret = -EFAULT; - goto out_unlock; - } - /* per custom, you can pass zero for len to mean "everything onward" */ if (!pin.len) pin.len = PAGE_ALIGN(asma->size) - pin.offset;
On Wed, Mar 07, 2018 at 01:40:30PM -0800, Paul Lawrence wrote:
Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652 ("staging: android: ashmem: Fix a race condition in pin ioctls") causing deadlock.
No need to hold ashmem_mutex while copying from user
Stacks are:
ashmem_mmap+0x53/0x400 drivers/staging/android/ashmem.c:379 mmap_region+0x7dd/0xfd0 mm/mmap.c:1694 do_mmap+0x57b/0xbe0 mm/mmap.c:1473
and
lock_acquire+0x12e/0x410 kernel/locking/lockdep.c:3756 __might_fault+0x14a/0x1d0 mm/memory.c:4014 copy_from_user arch/x86/include/asm/uaccess.h:705 [inline] ashmem_pin_unpin drivers/staging/android/ashmem.c:719 [inline]
Signed-off-by: Paul Lawrence paullawrence@google.com Cc: stable@vger.kernel.org # 4.9.x Cc: stable@vger.kernel.org # 4.4.x Cc: stable@vger.kernel.org # 3.18.x: ce8a3a9e76d01 Cc: stable@vger.kernel.org # 3.18.x Cc: Ben Hutchings ben@decadent.org.uk
drivers/staging/android/ashmem.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c index 6dbba5aff191..8c55706c2cfa 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -702,16 +702,14 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd, size_t pgstart, pgend; int ret = -EINVAL;
- if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
return -EFAULT;
- mutex_lock(&ashmem_mutex);
if (unlikely(!asma->file)) goto out_unlock;
- if (unlikely(copy_from_user(&pin, p, sizeof(pin)))) {
ret = -EFAULT;
goto out_unlock;
- }
- /* per custom, you can pass zero for len to mean "everything onward" */ if (!pin.len) pin.len = PAGE_ALIGN(asma->size) - pin.offset;
-- 2.16.2.395.g2e18187dfd-goog
Hey Paul,
Looks like this same patch is already in Greg's tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?i...
Cheers! Nathan Chancellor
Thanks! Somebody already sent this.
On Wed, Mar 07, 2018 at 01:40:30PM -0800, Paul Lawrence wrote:
Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652 ("staging: android: ashmem: Fix a race condition in pin ioctls") causing deadlock.
Use the Fixes tag for this. The format is:
Fixes: ce8a3a9e76d0 ("staging: android: ashmem: Fix a race condition in pin ioctls")
Then you only need one stable CC line. Just the oldest kernel which needs to be patched.
Cc: stable@vger.kernel.org # 3.18.x
The extra git hash after the stable CC is for if a bug requires several patches to fix it. In this case it only requires just this patch so there's no need to have the hash.
Basically you did extra work which isn't required. Anyway, this fix was already merged so no need to resend or anything.
regards, dan carpenter
linux-stable-mirror@lists.linaro.org