From patchwork Tue Jul 14 15:41:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Bas Nieuwenhuizen X-Patchwork-Id: 11663125 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 04222138C for ; Tue, 14 Jul 2020 15:41:48 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D5E032083B for ; Tue, 14 Jul 2020 15:41:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=basnieuwenhuizen-nl.20150623.gappssmtp.com header.i=@basnieuwenhuizen-nl.20150623.gappssmtp.com header.b="At4gkRUI" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D5E032083B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=basnieuwenhuizen.nl Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AAAD46E8A7; Tue, 14 Jul 2020 15:41:44 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by gabe.freedesktop.org (Postfix) with ESMTPS id F29A16E8A7 for ; Tue, 14 Jul 2020 15:41:42 +0000 (UTC) Received: by mail-ed1-x541.google.com with SMTP id g20so17728497edm.4 for ; Tue, 14 Jul 2020 08:41:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=basnieuwenhuizen-nl.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=fPc2Ss/8lpvcY2mROIad4+yzm0HXN/Mn6E5P50MZh+g=; b=At4gkRUItDv2bTs0xNUQM6klkWzkjBtcD5o50Y2SJAnx+zKx8I5H//b36vyamjzR/A RZzwpBJDvYsTOZz/Ai/RIUmNFK/bDWapx2VMd67/9AC5ekcX5ckzxokHGZ6ahFWqa6ye ctoxBjYJVT26gEdpOdAovQJpd0IAG9rgZbuNQ3nj7b1Lyw0PihmVFTpagYxhP9OxM1Tv 9eMBXwsHA80WDrXTeDiTm12HJxzRet/p1RioqNg1ChFmKyP3MfQvWi2CQMD9J7hyHFqR /UpNGM5Fbv+sTgtTo9KLx9NzTjA+DMIAjGbmJfePiyTuSCBqSvjRLxH9aSq94Yi/25G6 uFQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=fPc2Ss/8lpvcY2mROIad4+yzm0HXN/Mn6E5P50MZh+g=; b=D97uOVOSpGQ5CIc/OxDcHgSEDSBDqv72ig8JfRKZ4v0XKQ2aFza45KvVmM8Kq4jV7v 5UEN8jfCi3EEgHEu9F0mbenQmxcTQ4Q/QiiL93n0PI4P69OFeRhaiWeFlphLdV3D5GR1 TefTyHnSD0E0wwzc/JQMx7TUYXdL+XImapu6WctAGvNP9R+HZPK5/SmAjsIWV0ufHp7n I6j6rIK7lwQKqESGipPM2Px06S1Tl8fpuRnAQeuxOuFXpXUc/2Vf5LDm70sYe7p7EQHZ a/5/T/yfCK/mCQVJqkRAucMg5/SxWGKkwtWjcTWgidoFFy/tI3590QCOQzNJi627RuA0 xfuQ== X-Gm-Message-State: AOAM530OwhRuEnjcR4yvWcFQkoZtWWr0EjYOa+lPkHwYgIrQzW71Xw8N 5VizyU+gJnj48t8QiIkJ975y9X1gkhETNg== X-Google-Smtp-Source: ABdhPJwZObsEu8F5INUsvjQ8cz1MKDyRzRFpaBjGfKgiWcqbuSEzB2QfIMzWZgTdyKXSf+zp7gasYw== X-Received: by 2002:a05:6402:559:: with SMTP id i25mr4994902edx.35.1594741301264; Tue, 14 Jul 2020 08:41:41 -0700 (PDT) Received: from localhost.localdomain (31-10-158-161.cgn.dynamic.upc.ch. [31.10.158.161]) by smtp.gmail.com with ESMTPSA id s7sm15200482edr.57.2020.07.14.08.41.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jul 2020 08:41:40 -0700 (PDT) From: Bas Nieuwenhuizen To: dri-devel@lists.freedesktop.org Subject: [PATCH] dma-buf/sw_sync: Avoid recursive lock during fence signal. Date: Tue, 14 Jul 2020 17:41:02 +0200 Message-Id: <20200714154102.450826-1-bas@basnieuwenhuizen.nl> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Gustavo Padovan , stable@vger.kernel.org, Chris Wilson , =?utf-8?q?Christian_K=C3=B6nig?= Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Calltree: timeline_fence_release drm_sched_entity_wakeup dma_fence_signal_locked sync_timeline_signal sw_sync_ioctl Releasing the reference to the fence in the fence signal callback seems reasonable to me, so this patch avoids the locking issue in sw_sync. d3862e44daa7 ("dma-buf/sw-sync: Fix locking around sync_timeline lists") fixed the recursive locking issue but caused an use-after-free. Later d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") fixed the use-after-free but reintroduced the recursive locking issue. In this attempt we avoid the use-after-free still because the release function still always locks, and outside of the locking region in the signal function we have properly refcounted references. We furthermore also avoid the recurive lock by making sure that either: 1) We have a properly refcounted reference, preventing the signal from triggering the release function inside the locked region. 2) The refcount was already zero, and hence nobody will be able to trigger the release function from the signal function. Fixes: d3c6dd1fb30d ("dma-buf/sw_sync: Synchronize signal vs syncpt free") Cc: Sumit Semwal Cc: Chris Wilson Cc: Gustavo Padovan Cc: Christian König Cc: Signed-off-by: Bas Nieuwenhuizen --- drivers/dma-buf/sw_sync.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 348b3a9170fa..30a482f75d56 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -192,9 +192,12 @@ static const struct dma_fence_ops timeline_fence_ops = { static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) { struct sync_pt *pt, *next; + struct list_head ref_list; trace_sync_timeline(obj); + INIT_LIST_HEAD(&ref_list); + spin_lock_irq(&obj->lock); obj->value += inc; @@ -206,18 +209,27 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) list_del_init(&pt->link); rb_erase(&pt->node, &obj->pt_tree); - /* - * A signal callback may release the last reference to this - * fence, causing it to be freed. That operation has to be - * last to avoid a use after free inside this loop, and must - * be after we remove the fence from the timeline in order to - * prevent deadlocking on timeline->lock inside - * timeline_fence_release(). - */ + /* We need to take a reference to avoid a release during + * signalling (which can cause a recursive lock of obj->lock). + * If refcount was already zero, another thread is already taking + * care of destructing the fence, so the signal cannot release + * it again and we hence will not have the recursive lock. */ + if (dma_fence_get_rcu(&pt->base)) + list_add_tail(&pt->link, &ref_list); + dma_fence_signal_locked(&pt->base); } spin_unlock_irq(&obj->lock); + + list_for_each_entry_safe(pt, next, &ref_list, link) { + /* This needs to be cleared before release, otherwise the + * timeline_fence_release function gets confused about also + * removing the fence from the pt_tree. */ + list_del_init(&pt->link); + + dma_fence_put(&pt->base); + } } /**