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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78C11C433FE for ; Wed, 30 Nov 2022 11:22:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231314AbiK3LWb (ORCPT ); Wed, 30 Nov 2022 06:22:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42892 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234905AbiK3LVj (ORCPT ); Wed, 30 Nov 2022 06:21:39 -0500 Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6755077219 for ; Wed, 30 Nov 2022 03:21:23 -0800 (PST) Received: by mail-wm1-x333.google.com with SMTP id t1so12959619wmi.4 for ; Wed, 30 Nov 2022 03:21:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=RuSCu6Sc5O77Oj3wELaaUy6VtrciSxnoo8y2CPFyY+U=; b=M5OU+UwmCE1dEQUm9MGdXnRpdzmbCWLfqjalSYP23MuqwTZ9RwFy/pfMOsPhuvK6s3 RiK7JxFxDC/TRdJDjog5UlAx3+QHvvk+4eHlFtUCvbrEYR1WO7WCBR59V08EJk4u9QnO N8B08dkiBlp3ZJc8e7cHv8OxsGRBeXy47Ce3k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=RuSCu6Sc5O77Oj3wELaaUy6VtrciSxnoo8y2CPFyY+U=; b=C3+V7aKC9hRBjJ7qY6421EZ7XQNiPSqA6lLjcOW0DubqTO07Lc+MMpmEYRjIvaYU0u 0vRVSoST2e5/a8m6JKaXEkZDuESpkUTl8fozGHX9cyQAweCzcxwnfsyhPPViwwokf1EG TgcJWWduvTs5lWNHRjwcP0uMd0N75t+4IzlYfL+749n/qnWVDJ9e3+dhzrB1o0nbPON6 lCvmlqRoJDr8aBiKTeq2xbeqQRMFKilhYx29hT1eSmxWVlmUTgX0QpLKISgnLK41dqU/ 7dJQOpHKtTWRwwwCTxKrRA2To8Iy/95LKEkYIlS230vf/jAgT+iRh/ispKaMbBHn494O fubw== X-Gm-Message-State: ANoB5pl0lTOfYFhv1wBbt2afnhgacDJ0HI/QbrUB2DeoTbehvwRB5ewD 1HF1isQLWO+Wy5/eEZZlvXc/kg== X-Google-Smtp-Source: AA0mqf7Y6ivv5bQ1OH3MnYDxUtlCUbzKEC1tIMe0iHw9hdRx5C5HeGZP24b3Lywyd5EKm1ITulLBpg== X-Received: by 2002:a05:600c:2241:b0:3cf:9ced:dce4 with SMTP id a1-20020a05600c224100b003cf9ceddce4mr43768262wmm.120.1669807281946; Wed, 30 Nov 2022 03:21:21 -0800 (PST) Received: from phenom.ffwll.local (212-51-149-33.fiber7.init7.net. [212.51.149.33]) by smtp.gmail.com with ESMTPSA id f11-20020a05600c154b00b003c6f3e5ba42sm5960234wmg.46.2022.11.30.03.21.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Nov 2022 03:21:21 -0800 (PST) Date: Wed, 30 Nov 2022 12:21:19 +0100 From: Daniel Vetter To: Rob Clark Cc: Guenter Roeck , Rob Clark , open list , stable@vger.kernel.org, Eric Anholt , Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, Thomas Zimmermann Subject: Re: [2/2] drm/shmem-helper: Avoid vm_open error paths Message-ID: Mail-Followup-To: Rob Clark , Guenter Roeck , Rob Clark , open list , stable@vger.kernel.org, Eric Anholt , Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, Thomas Zimmermann References: <20221129200242.298120-3-robdclark@gmail.com> <20221129203205.GA2132357@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.19.0-2-amd64 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 29, 2022 at 12:47:42PM -0800, Rob Clark wrote: > On Tue, Nov 29, 2022 at 12:32 PM Guenter Roeck wrote: > > > > On Tue, Nov 29, 2022 at 12:02:42PM -0800, Rob Clark wrote: > > > From: Rob Clark > > > > > > vm_open() is not allowed to fail. Fortunately we are guaranteed that > > > the pages are already pinned, and only need to increment the refcnt. So > > > just increment it directly. > > > > I don't know anything about drm or gem, but I am wondering _how_ > > this would be guaranteed. Would it be through the pin function ? > > Just wondering, because that function does not seem to be mandatory. > > We've pinned the pages already in mmap.. vm->open() is perhaps not the > best name for the callback function, but it is called for copying an > existing vma into a new process (and for some other cases which do not > apply here because VM_DONTEXPAND). > > (Other drivers pin pages in the fault handler, where there is actually > potential to return an error, but that change was a bit more like > re-writing shmem helper ;-)) Yhea vm_ops->open should really be called vm_ops->dupe or ->copy or something like that ... -Daniel > > BR, > -R > > > > > > > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Rob Clark > > > --- > > > drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++++--- > > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > index 110a9eac2af8..9885ba64127f 100644 > > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > > @@ -571,12 +571,20 @@ static void drm_gem_shmem_vm_open(struct vm_area_struct *vma) > > > { > > > struct drm_gem_object *obj = vma->vm_private_data; > > > struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > > > - int ret; > > > > > > WARN_ON(shmem->base.import_attach); > > > > > > - ret = drm_gem_shmem_get_pages(shmem); > > > - WARN_ON_ONCE(ret != 0); > > > + mutex_lock(&shmem->pages_lock); > > > + > > > + /* > > > + * We should have already pinned the pages, vm_open() just grabs > > > > should or guaranteed ? This sounds a bit weaker than the commit > > description. > > > > > + * an additional reference for the new mm the vma is getting > > > + * copied into. > > > + */ > > > + WARN_ON_ONCE(!shmem->pages_use_count); > > > + > > > + shmem->pages_use_count++; > > > + mutex_unlock(&shmem->pages_lock); > > > > The previous code, in that situation, would not increment pages_use_count, > > and it would not set not set shmem->pages. Hopefully, it would not try to > > do anything with the pages it was unable to get. The new code assumes that > > shmem->pages is valid even if pages_use_count is 0, while at the same time > > taking into account that this can possibly happen (or the WARN_ON_ONCE > > would not be needed). > > > > Again, I don't know anything about gem and drm, but it seems to me that > > there might now be a severe problem later on if the WARN_ON_ONCE() > > ever triggers. > > > > Thanks, > > Guenter > > > > > > > > drm_gem_vm_open(vma); > > > } -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch