From patchwork Mon Feb 24 14:06:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ioanna Alifieraki X-Patchwork-Id: 1243111 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48R3ln0mHCz9sRQ; Tue, 25 Feb 2020 01:06:59 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1j6ENp-0007LU-Vi; Mon, 24 Feb 2020 14:06:53 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1j6ENk-0007LO-E1 for kernel-team@lists.ubuntu.com; Mon, 24 Feb 2020 14:06:48 +0000 Received: from mail-wr1-f72.google.com ([209.85.221.72]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1j6ENk-00006U-61 for kernel-team@lists.ubuntu.com; Mon, 24 Feb 2020 14:06:48 +0000 Received: by mail-wr1-f72.google.com with SMTP id z1so1989409wrs.9 for ; Mon, 24 Feb 2020 06:06:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=Az8641fPr5eUqUI5G+3hE1dQynGrGcjXhEMix/JHn7U=; b=GJXPWpjmhdOf9raCkD+OE8GEWgc0ogZGIvrSM0cUaWZIr++3RJDtgeZAp9fPIQizkk stwlJUJmX0QadUoU6qOM+Fno6w7WvbrkzIg64UXlva0jG/h16BmkSXeYoU7MJnquPSrI ym1OhDRLyyGYLc5+uHwPXvEiEOvAhlsfH0b+f2HnQv3kADaC3IydvOj28Wo/fUBFdEJF GtGYrNPTg8BYaGtF2x9GYLnxxp1s5NT2/6B8xF9BL2FlqKVlnm5TfY2Lt8bcdE0y3cHk HnbGLaYiZ40Xk2Aog3wzdbp0xL3ZxdUdX2MU76foNU/EEG0lzgDB0WBcMHYCiFMVbuKA dSZg== X-Gm-Message-State: APjAAAUVgnPLcxOYExgGKSvxil2A285yvxfnQR5ybfc9WGlyH8NSr3dF 6Z5TyCoqALllsxEl4tKXmfpKvxrvnkWLvFBEX6M09cTwv60doZwE3h6WvyFIHArxUWFXvzGIL/r TdsR/kjMaG4yQpBwhcFVI/7pgwyjvylzNFbf9LowG7Q== X-Received: by 2002:a5d:5609:: with SMTP id l9mr4468579wrv.48.1582553207375; Mon, 24 Feb 2020 06:06:47 -0800 (PST) X-Google-Smtp-Source: APXvYqymk+hZmSqNmk3ejelao9CliYfjZ7KAwq7VAhUF7DyTzAvDyxg8A+XX53AuIB2HrB10to/Zzg== X-Received: by 2002:a5d:5609:: with SMTP id l9mr4468552wrv.48.1582553207072; Mon, 24 Feb 2020 06:06:47 -0800 (PST) Received: from localhost ([2a02:587:2808:4200:2471:e017:5e67:ac0f]) by smtp.gmail.com with ESMTPSA id a26sm18263948wmm.18.2020.02.24.06.06.45 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 24 Feb 2020 06:06:46 -0800 (PST) From: Ioanna Alifieraki To: kernel-team@lists.ubuntu.com Subject: [SRU][X/B/E/F][PATCH] Revert "ipc, sem: remove uneeded sem_undo_list lock usage in exit_sem()" Date: Mon, 24 Feb 2020 14:06:44 +0000 Message-Id: <20200224140644.27330-1-ioanna-maria.alifieraki@canonical.com> X-Mailer: git-send-email 2.17.1 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" BugLink: https://bugs.launchpad.net/bugs/1858834 This reverts commit a97955844807e327df11aa33869009d14d6b7de0. Commit a97955844807 ("ipc,sem: remove uneeded sem_undo_list lock usage in exit_sem()") removes a lock that is needed. This leads to a process looping infinitely in exit_sem() and can also lead to a crash. There is a reproducer available in [1] and with the commit reverted the issue does not reproduce anymore. Using the reproducer found in [1] is fairly easy to reach a point where one of the child processes is looping infinitely in exit_sem between for(;;) and if (semid == -1) block, while it's trying to free its last sem_undo structure which has already been freed by freeary(). Each sem_undo struct is on two lists: one per semaphore set (list_id) and one per process (list_proc). The list_id list tracks undos by semaphore set, and the list_proc by process. Undo structures are removed either by freeary() or by exit_sem(). The freeary function is invoked when the user invokes a syscall to remove a semaphore set. During this operation freeary() traverses the list_id associated with the semaphore set and removes the undo structures from both the list_id and list_proc lists. For this case, exit_sem() is called at process exit. Each process contains a struct sem_undo_list (referred to as "ulp") which contains the head for the list_proc list. When the process exits, exit_sem() traverses this list to remove each sem_undo struct. As in freeary(), whenever a sem_undo struct is removed from list_proc, it is also removed from the list_id list. Removing elements from list_id is safe for both exit_sem() and freeary() due to sem_lock(). Removing elements from list_proc is not safe; freeary() locks &un->ulp->lock when it performs list_del_rcu(&un->list_proc) but exit_sem() does not (locking was removed by commit a97955844807 ("ipc,sem: remove uneeded sem_undo_list lock usage in exit_sem()"). This can result in the following situation while executing the reproducer [1] : Consider a child process in exit_sem() and the parent in freeary() (because of semctl(sid[i], NSEM, IPC_RMID)). - The list_proc for the child contains the last two undo structs A and B (the rest have been removed either by exit_sem() or freeary()). - The semid for A is 1 and semid for B is 2. - exit_sem() removes A and at the same time freeary() removes B. - Since A and B have different semid sem_lock() will acquire different locks for each process and both can proceed. The bug is that they remove A and B from the same list_proc at the same time because only freeary() acquires the ulp lock. When exit_sem() removes A it makes ulp->list_proc.next to point at B and at the same time freeary() removes B setting B->semid=-1. At the next iteration of for(;;) loop exit_sem() will try to remove B. The only way to break from for(;;) is for (&un->list_proc == &ulp->list_proc) to be true which is not. Then exit_sem() will check if B->semid=-1 which is and will continue looping in for(;;) until the memory for B is reallocated and the value at B->semid is changed. At that point, exit_sem() will crash attempting to unlink B from the lists (this can be easily triggered by running the reproducer [1] a second time). To prove this scenario instrumentation was added to keep information about each sem_undo (un) struct that is removed per process and per semaphore set (sma). CPU0 CPU1 [caller holds sem_lock(sma for A)] ... freeary() exit_sem() ... ... ... sem_lock(sma for B) spin_lock(A->ulp->lock) ... list_del_rcu(un_A->list_proc) list_del_rcu(un_B->list_proc) Undo structures A and B have different semid and sem_lock() operations proceed. However they belong to the same list_proc list and they are removed at the same time. This results into ulp->list_proc.next pointing to the address of B which is already removed. After reverting commit a97955844807 ("ipc,sem: remove uneeded sem_undo_list lock usage in exit_sem()") the issue was no longer reproducible. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1694779 Link: http://lkml.kernel.org/r/20191211191318.11860-1-ioanna-maria.alifieraki@canonical.com Fixes: a97955844807 ("ipc,sem: remove uneeded sem_undo_list lock usage in exit_sem()") Signed-off-by: Ioanna Alifieraki Acked-by: Manfred Spraul Acked-by: Herton R. Krzesinski Cc: Arnd Bergmann Cc: Catalin Marinas Cc: Cc: Joel Fernandes (Google) Cc: Davidlohr Bueso Cc: Jay Vosburgh Cc: Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds (cherry picked from commit edf28f4061afe4c2d9eb1c3323d90e882c1d6800) Acked-by: Colin Ian King Acked-by: Khalid Elmously --- ipc/sem.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ipc/sem.c b/ipc/sem.c index ec97a7072413..fe12ea8dd2b3 100644 --- a/ipc/sem.c +++ b/ipc/sem.c @@ -2368,11 +2368,9 @@ void exit_sem(struct task_struct *tsk) ipc_assert_locked_object(&sma->sem_perm); list_del(&un->list_id); - /* we are the last process using this ulp, acquiring ulp->lock - * isn't required. Besides that, we are also protected against - * IPC_RMID as we hold sma->sem_perm lock now - */ + spin_lock(&ulp->lock); list_del_rcu(&un->list_proc); + spin_unlock(&ulp->lock); /* perform adjustments registered in un */ for (i = 0; i < sma->sem_nsems; i++) {