From patchwork Tue May 12 04:42:16 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alistair Popple X-Patchwork-Id: 471124 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 55217140187 for ; Tue, 12 May 2015 14:43:24 +1000 (AEST) Received: from ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 39F8E1A0145 for ; Tue, 12 May 2015 14:43:24 +1000 (AEST) X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Received: from e23smtp07.au.ibm.com (e23smtp07.au.ibm.com [202.81.31.140]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4131C1A0008 for ; Tue, 12 May 2015 14:43:21 +1000 (AEST) Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 12 May 2015 14:43:19 +1000 Received: from d23dlp03.au.ibm.com (202.81.31.214) by e23smtp07.au.ibm.com (202.81.31.204) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 12 May 2015 14:43:16 +1000 Received: from d23relay09.au.ibm.com (d23relay09.au.ibm.com [9.185.63.181]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 07AF93578048 for ; Tue, 12 May 2015 14:43:15 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay09.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t4C4h6cv40239228 for ; Tue, 12 May 2015 14:43:15 +1000 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t4C4ggcO008578 for ; Tue, 12 May 2015 14:42:42 +1000 Received: from ozlabs.au.ibm.com (ozlabs.au.ibm.com [9.192.253.14]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id t4C4gfLX008157; Tue, 12 May 2015 14:42:42 +1000 Received: from localhost (unknown [9.192.254.114]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) by ozlabs.au.ibm.com (Postfix) with ESMTPSA id 99C3EA027C; Tue, 12 May 2015 14:42:17 +1000 (AEST) From: Alistair Popple To: skiboot@lists.ozlabs.org Date: Tue, 12 May 2015 14:42:16 +1000 Message-Id: <1431405736-26412-1-git-send-email-alistair@popple.id.au> X-Mailer: git-send-email 1.8.3.2 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15051204-0025-0000-0000-00000176E243 Subject: [Skiboot] [PATCH] Fix race in flash resource preloading X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" From: Stewart Smith (13:31:46) benh: stewart: flash_load_resources() (13:31:53) benh: stewart: you hit the unlock at the bottom of the loop (13:31:59) benh: stewart: at that point the list may be empty (13:32:09) benh: stewart: and so another concurrent load can restart the thread (13:32:15) benh: stewart: you end up with duplicate threads (13:32:26) benh: stewart: in which case you can hit the assert (13:34:27) benh: ie, never drop the lock with the queue empty (13:34:29) benh: unless you know you will exit the job (13:34:32) benh: otherwise you can have a duplicate job (13:34:41) benh: -> kaboom (13:36:29) benh: yeah the decision to exit the loop must be atomic with the popping of the last element in the list (13:36:43) benh: to match the decision to start the thread which is atomic with the queuing of the first element Reported-by: Benjamin Herrenschmidt Signed-off-by: Stewart Smith --- core/flash.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/flash.c b/core/flash.c index 092f8f2..c08bb49 100644 --- a/core/flash.c +++ b/core/flash.c @@ -680,15 +680,16 @@ static void flash_load_resources(void *data __unused) struct flash_load_resource_item *r; int result; + lock(&flash_load_resource_lock); do { - lock(&flash_load_resource_lock); if (list_empty(&flash_load_resource_queue)) { - unlock(&flash_load_resource_lock); break; } r = list_top(&flash_load_resource_queue, struct flash_load_resource_item, link); - assert(r->result == OPAL_EMPTY); + if (r->result != OPAL_EMPTY) + prerror("flash_load_resources() list_top unexpected " + " result %d\n", r->result); r->result = OPAL_BUSY; unlock(&flash_load_resource_lock); @@ -699,8 +700,8 @@ static void flash_load_resources(void *data __unused) struct flash_load_resource_item, link); r->result = result; list_add_tail(&flash_loaded_resources, &r->link); - unlock(&flash_load_resource_lock); } while(true); + unlock(&flash_load_resource_lock); } static void start_flash_load_resource_job(void)