From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 10F91C2BA1A for ; Tue, 7 Apr 2020 17:28:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 98E2C206F7 for ; Tue, 7 Apr 2020 17:28:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=linux-foundation.org header.i=@linux-foundation.org header.b="Xdrw0xDw" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98E2C206F7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 142058E0005; Tue, 7 Apr 2020 13:28:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0F2938E0001; Tue, 7 Apr 2020 13:28:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 02E9A8E0005; Tue, 7 Apr 2020 13:28:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0006.hostedemail.com [216.40.44.6]) by kanga.kvack.org (Postfix) with ESMTP id E07248E0001 for ; Tue, 7 Apr 2020 13:28:54 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 9CE913D05 for ; Tue, 7 Apr 2020 17:28:54 +0000 (UTC) X-FDA: 76681744188.04.plane05_4dc0359ec6736 X-HE-Tag: plane05_4dc0359ec6736 X-Filterd-Recvd-Size: 7321 Received: from mail-lj1-f196.google.com (mail-lj1-f196.google.com [209.85.208.196]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Tue, 7 Apr 2020 17:28:54 +0000 (UTC) Received: by mail-lj1-f196.google.com with SMTP id b1so4682042ljp.3 for ; Tue, 07 Apr 2020 10:28:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4zBowDsk8SE9XIHjyrUakYlZAvAxnPCEwiNhH5fwjuY=; b=Xdrw0xDwBHfCEmGTfzNnWznVG+4AGONJjDQN1h/lv9hhV/dTjZ04MAALboJJD39wFV +RqBuvJ+THImYmVKF0P4MGtL+IkFO6kt3MfXs/+Zveld1YcErLWxARFrTvtTcKPOwNOp 9Br+gwL2oWeO6ACGXesYVoatyDyydJEGdNrLI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4zBowDsk8SE9XIHjyrUakYlZAvAxnPCEwiNhH5fwjuY=; b=Mge/9oCo8Mf7fmOLtrfKORvqnxfHA+/p/AGYsXcptdKwbuY9yFtW5Rfw/qfEFxq7wj r3ys51L8yj8pc++HT6vafN6zWshk4R750fAGTPhyIZrm+k1bdPHF/MZTzddSmIfsqshD YNd/wQlLTm8Vd3Nrw88C9Ll3TitT8+1X7O4uPXXNNFiZuP1YQKZoLYFbiCQ02FEvrX4e /N5A6modbiLISf+MDHlFFXcqYqQPqkiCQRX5ZKUWG8ToijahYjH0Px9WWf/ZVXpMy7TH b2VRLTJkLttW9BmJTfu9dalxCWp6t+8Cc8t9UQ2FeyrlP4pSGbddJa//Gv4S3uPrxdpe B1Mg== X-Gm-Message-State: AGi0PuYtZyE+nCU+0ibCkRNhe6nTQzNbjndssJakTh8t6iUqm4zNudO2 gfa5jzyu9VrvKYmzP19rJQYHi8Xere0= X-Google-Smtp-Source: APiQypIMkUFIuRT4ZEoXCX3cJug5xQVr7D0cWWiHjN3f6QzRq/bG5ZKl+A2Q0WoICit34CwUyxo6hA== X-Received: by 2002:a2e:8850:: with SMTP id z16mr2378353ljj.284.1586280532063; Tue, 07 Apr 2020 10:28:52 -0700 (PDT) Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com. [209.85.208.169]) by smtp.gmail.com with ESMTPSA id h20sm11720949lji.103.2020.04.07.10.28.50 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Apr 2020 10:28:51 -0700 (PDT) Received: by mail-lj1-f169.google.com with SMTP id v16so4671807ljg.5 for ; Tue, 07 Apr 2020 10:28:50 -0700 (PDT) X-Received: by 2002:a2e:7c1a:: with SMTP id x26mr2257750ljc.209.1586280530396; Tue, 07 Apr 2020 10:28:50 -0700 (PDT) MIME-Version: 1.0 References: <20200406200254.a69ebd9e08c4074e41ddebaf@linux-foundation.org> <20200407031042.8o-fYMox-%akpm@linux-foundation.org> <158627540139.8918.10102358634447361335@build.alporthouse.com> In-Reply-To: <158627540139.8918.10102358634447361335@build.alporthouse.com> From: Linus Torvalds Date: Tue, 7 Apr 2020 10:28:34 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [patch 125/166] lib/list: prevent compiler reloads inside 'safe' list iteration To: Chris Wilson Cc: Andrew Morton , David Laight , Marco Elver , Linux-MM , Mark Rutland , mm-commits@vger.kernel.org, "Paul E. McKenney" , Randy Dunlap , stable Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Apr 7, 2020 at 9:04 AM Chris Wilson wrote: > > It'll take some time to reconstruct the original report, but the case in > question was in removing the last element of the list of the last list, > switch to a global lock over all such lists to park the HW, which in > doing so added one more element to the original list. [If we happen to > be retiring along the kernel timeline in the first place.] Please point to the real code and the list. Honestly, what you describe sounds complex enough that I think your locking is simply just buggy. IOW, this patch seems to really just paper over a locking bug, and the KASAN report tied to it. Because the fundamental issue is that READ_ONCE() can not fix a bug here. Reading the next pointer once fundamentally cannot matter if it can change concurrently: the code is buggy, and the READ_ONCE() just means that it gets one or the other value randomly, and that the list walking is fundamentally racy. One the other hand, if the next pointer _cannot_ change concurrently, then READ_ONCE() cannot make a difference. So as fat as I can tell, we have two possibilities, and in both cases changing the code to use READ_ONCE() is not the right thing to do. In one case it hides a bug, and in another case it's just pointless. > list->next changed from pointing to list_head, to point to the new > element instead. However, we don't have to check the next element yet > and want to terminate the list iteration. I'd really like to see the actual code that has that list walking. You say: > For reference, > drivers/gpu/drm/i915/gt/intel_engine_pm.c::__engine_park() .. but that function doesn't have any locking or list-walking. Is it the "call_idle_barriers()" loop? What is it? I'd really like to see the KASAN report and the discussion about this change. And if there was no discussion, then the patch just seems like "I changed code randomly and the KASAN report went away, so it's all good". > Global activity is serialised by engine->wakeref.mutex; every active > timeline is required to hold an engine wakeref, but retiring is local to > timelines and serialised by their own timeline->mutex. > > lock(&timeline->lock) > list_for_each_safe(&timeline->requests) > \-> i915_request_retire [list_del(&timeline->requests)] > \-> intel_timeline_exit > \-> lock(&engine->wakeref.mutex) > engine_park [list_add_tail(&engine->kernel_context->timeline->requests)] in that particular list_for_each_safe() thing, there's no possibility that the 'next' field would be reloaded, since the list_del() in the above will be somethign the compiler is aware of. So yes, the beginning list_for_each_safe() might load it twice (or a hundred times), but by the time that list_del() in i915_request_retire() has been called, if the compiler then reloads it afterwards, that would be a major compiler bug, since it's after that value could have been written in the local thread. So this doesn't explain it to me. What it *sounds* like is that the "engine" lock that you do *not* hold initially, is not protecting some accessor to that list, so you have a race on the list at the time of that list_del(). And that race may be what KASAN is reporting, and what that patch is _hiding_ from KASAN - but not fixing. See what I am saying and why I find this patch questionable? There may be something really subtle going on, but it really smells like "two threads are modifying the same list at the same time". And there's no way that the READ_ONCE() will fix that bug, it will only make KASAN shut up about it. Linus