Message ID | 20231029230205.93277-23-dmitry.osipenko@collabora.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:d641:0:b0:403:3b70:6f57 with SMTP id cy1csp1895321vqb; Sun, 29 Oct 2023 16:20:38 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHFAMdZ3JBCKoUl0p7RnnqHnIbYhm1IvLjvivm6s4nK+TV7IVvGkIJhbIqlJYopzKpZVAJW X-Received: by 2002:a17:90b:100b:b0:27f:fe79:eb6e with SMTP id gm11-20020a17090b100b00b0027ffe79eb6emr7829749pjb.8.1698621638707; Sun, 29 Oct 2023 16:20:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698621638; cv=none; d=google.com; s=arc-20160816; b=Z8+9oTMgKEdAARwdoP8YgbZzCK9Hoev6wxuDkcPjnFskKyYb4cWrj5v1PK+cLpy58A 0EiAmIErgu8gB6kUW0BIln6M/h8rtqmatF1y6QSo413KGPC4Aa2P41W7EH+h78E7LDbv iJd/r1h+O6TQlcy4RsGjAUJsDrozatOKYx1OIsaCXLN3T6xjtaMKl+5oDCNSTOjdPi8q HSvYVn/SoeaJy7w1M7Bi4w93YAAuWyJAncDn9sbpEYjSSqwtcZ/VW2hhz6vQgMSEvY5m opeF5jF8TnzP6x4pSuwHs8I7qcrSn1DZnNYb1Jmy/c/Nzswk13BPpit44jQY3RhPt2Bj 9X3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=Wn2UqZ6ZiYruYvg5J5Hi5GHeHLYxvT1f2wGltq/E2es=; fh=5cel2jD5h+yPMXVwxbomyVhwojHUqATy6nFjd4aOh4o=; b=CIZTbn43IvmcGDlpV9i6t5pjfdsTGRbGutYJ1PU2JKGsYVCsUjfX6CdD/mGB1b6ZTT wguqKRPDTtAR3eo6wKijDnofg6XiJ3paB7uM1jwRtOZlxmk2hnaHKfvxqF/Z4LD7Syd9 Rlz9qBX2uODq1VbVYcfGKyH2QCyLCUX+RB1pr61osPs3WczYyzmmxk5yHYTIKzkPLaOm 2Hn3IgzzixX+t0k5DCI4gi0i1Y6SlydTuC7pbLZi3S3OQkWJaDMICv55akhhyxM6Q8XP bJ+/EZbHHGcgX68ftsnPesqOCuQ96gd6raN0OT4sF2FO+gcX9AQ02c5yMMVx1RlLrcK8 tO1w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=MRGBZwF1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id v23-20020a17090ae99700b002768ab837bfsi741989pjy.48.2023.10.29.16.20.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 29 Oct 2023 16:20:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=MRGBZwF1; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 9A52D807BA39; Sun, 29 Oct 2023 16:20:36 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231847AbjJ2XUW (ORCPT <rfc822;zxc52fgh@gmail.com> + 31 others); Sun, 29 Oct 2023 19:20:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41360 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231668AbjJ2XUK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 29 Oct 2023 19:20:10 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 381CE7692 for <linux-kernel@vger.kernel.org>; Sun, 29 Oct 2023 16:04:21 -0700 (PDT) Received: from workpc.. (109-252-153-31.dynamic.spd-mgts.ru [109.252.153.31]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: dmitry.osipenko) by madras.collabora.co.uk (Postfix) with ESMTPSA id 546C36607396; Sun, 29 Oct 2023 23:02:58 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1698620579; bh=8rtXn19fvaBXhb48S1Y4vYdXdzNTfF4j95pl2pB4Jj8=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=MRGBZwF1NKhPQjB1XQlhRcOVPXHE84vqpNBhZrkK8krkrkiczEXKlHJqJFSt5Ww9A li5hV7sh/Zq/Gg8hOZjrbJ9jpEIg9QckJmNImb4IvRjWhWqpNpxAvikQCnNIelyYfe LBNNSlcea5YhZowelFyeGDaSya0NHk+bytBWQ1/1BxxYsUIgX7OHsQGKsMo+HFNH+s 71KYOKFuX8jktieetnuG3bIEH/j5eJOHRWcuFXlSBi1nt+QhKLXN+XqfN3fV7kTL8d JIwe/KHanbZO0DjJ6j6zPyZTADs7al9+xga2M3r83xbeXc/8hUodG7MCY27isI+Edj xQFYRgY+PCaug== From: Dmitry Osipenko <dmitry.osipenko@collabora.com> To: David Airlie <airlied@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Gurchetan Singh <gurchetansingh@chromium.org>, Chia-I Wu <olvaffe@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, =?utf-8?q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>, Qiang Yu <yuq825@gmail.com>, Steven Price <steven.price@arm.com>, Boris Brezillon <boris.brezillon@collabora.com>, Emma Anholt <emma@anholt.net>, Melissa Wen <mwen@igalia.com> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel@collabora.com, virtualization@lists.linux-foundation.org Subject: [PATCH v18 22/26] drm/shmem-helper: Don't free refcounted GEM Date: Mon, 30 Oct 2023 02:02:01 +0300 Message-ID: <20231029230205.93277-23-dmitry.osipenko@collabora.com> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20231029230205.93277-1-dmitry.osipenko@collabora.com> References: <20231029230205.93277-1-dmitry.osipenko@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Sun, 29 Oct 2023 16:20:36 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781133883471078096 X-GMAIL-MSGID: 1781133883471078096 |
Series |
Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
|
|
Commit Message
Dmitry Osipenko
Oct. 29, 2023, 11:02 p.m. UTC
Don't free refcounted shmem object to prevent use-after-free bug that
is worse than a memory leak.
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
Comments
On Mon, 30 Oct 2023 02:02:01 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > Don't free refcounted shmem object to prevent use-after-free bug that > is worse than a memory leak. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 6dd087f19ea3..4253c367dc07 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > if (obj->import_attach) > drm_prime_gem_destroy(obj, shmem->sgt); > > - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); > - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); > - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); > + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) || > + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) || > + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count))) > + return; I guess you're worried about ->sgt being referenced by the driver after the GEM is destroyed. If we assume drivers don't cache the sgt and always call get_pages_sgt() when they need it that shouldn't be an issue. What we really don't want to release is the pages themselves, but the GPU MMU might still have active mappings pointing to these pages. In any case, I'm not against leaking the GEM object when any of these counters are not zero, but can we at least have a comment in the code explaining why we're doing that, so people don't have to go look at the git history to figure it out. > > drm_gem_object_release(obj); > kfree(shmem);
On 11/13/23 12:54, Boris Brezillon wrote: > On Mon, 30 Oct 2023 02:02:01 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> Don't free refcounted shmem object to prevent use-after-free bug that >> is worse than a memory leak. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index 6dd087f19ea3..4253c367dc07 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> if (obj->import_attach) >> drm_prime_gem_destroy(obj, shmem->sgt); >> >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); >> + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) || >> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) || >> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count))) >> + return; > > I guess you're worried about ->sgt being referenced by the driver after > the GEM is destroyed. If we assume drivers don't cache the sgt and > always call get_pages_sgt() when they need it that shouldn't be an > issue. What we really don't want to release is the pages themselves, > but the GPU MMU might still have active mappings pointing to these > pages. > > In any case, I'm not against leaking the GEM object when any of these > counters are not zero, but can we at least have a comment in the > code explaining why we're doing that, so people don't have to go look > at the git history to figure it out. This patch is a minor improvement, it doesn't address any specific issue. This should be a common pattern in kernel. If you're giving a warning and know about the inevitable catastrophe, then avoid it if you can. Actually, there are other similar cases in drm-shmem that can be improved.
On Thu, 23 Nov 2023 01:30:24 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/13/23 12:54, Boris Brezillon wrote: > > On Mon, 30 Oct 2023 02:02:01 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> Don't free refcounted shmem object to prevent use-after-free bug that > >> is worse than a memory leak. > >> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index 6dd087f19ea3..4253c367dc07 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> if (obj->import_attach) > >> drm_prime_gem_destroy(obj, shmem->sgt); > >> > >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); > >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); > >> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); > >> + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) || > >> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) || > >> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count))) > >> + return; > > > > I guess you're worried about ->sgt being referenced by the driver after > > the GEM is destroyed. If we assume drivers don't cache the sgt and > > always call get_pages_sgt() when they need it that shouldn't be an > > issue. What we really don't want to release is the pages themselves, > > but the GPU MMU might still have active mappings pointing to these > > pages. > > > > In any case, I'm not against leaking the GEM object when any of these > > counters are not zero, but can we at least have a comment in the > > code explaining why we're doing that, so people don't have to go look > > at the git history to figure it out. > > This patch is a minor improvement, it doesn't address any specific > issue. This should be a common pattern in kernel. If you're giving a > warning and know about the inevitable catastrophe, then avoid it if you can. Sure, I'm just asking that we add a comment to explain why we leak memory here. Is that too much to ask?
On 11/23/23 12:08, Boris Brezillon wrote: > On Thu, 23 Nov 2023 01:30:24 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 11/13/23 12:54, Boris Brezillon wrote: >>> On Mon, 30 Oct 2023 02:02:01 +0300 >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: >>> >>>> Don't free refcounted shmem object to prevent use-after-free bug that >>>> is worse than a memory leak. >>>> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> index 6dd087f19ea3..4253c367dc07 100644 >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> if (obj->import_attach) >>>> drm_prime_gem_destroy(obj, shmem->sgt); >>>> >>>> - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); >>>> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); >>>> - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); >>>> + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) || >>>> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) || >>>> + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count))) >>>> + return; >>> >>> I guess you're worried about ->sgt being referenced by the driver after >>> the GEM is destroyed. If we assume drivers don't cache the sgt and >>> always call get_pages_sgt() when they need it that shouldn't be an >>> issue. What we really don't want to release is the pages themselves, >>> but the GPU MMU might still have active mappings pointing to these >>> pages. >>> >>> In any case, I'm not against leaking the GEM object when any of these >>> counters are not zero, but can we at least have a comment in the >>> code explaining why we're doing that, so people don't have to go look >>> at the git history to figure it out. >> >> This patch is a minor improvement, it doesn't address any specific >> issue. This should be a common pattern in kernel. If you're giving a >> warning and know about the inevitable catastrophe, then avoid it if you can. > > Sure, I'm just asking that we add a comment to explain why we leak > memory here. Is that too much to ask? Will add the comment. The reason why I added this patch was unrelated to the sgt, that's what I'm talking about.
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 6dd087f19ea3..4253c367dc07 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) if (obj->import_attach) drm_prime_gem_destroy(obj, shmem->sgt); - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) || + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) || + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count))) + return; drm_gem_object_release(obj); kfree(shmem);