Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper

From: Eric Anholt <eric@anholt•net>
To: Rob Herring <robh@kernel•org>
Cc: Neil Armstrong <narmstrong@baylibre•com>,
	Maxime Ripard <maxime.ripard@bootlin•com>,
	Sean Paul <sean@poorly•run>, Will Deacon <will.deacon@arm•com>,
	Linux Kernel Mailing List <linux-kernel@vger•kernel.org>,
	dri-devel <dri-devel@lists•freedesktop.org>,
	Chris Wilson <chris@chris-wilson•co.uk>,
	David Airlie <airlied@linux•ie>,
	"list@263•net:IOMMU DRIVERS <iommu@lists•linux-foundation.org>,
	Joerg Roedel <joro@8bytes•org>,
	" <iommu@lists•linux-foundation.org>,
	Alyssa Rosenzweig <alyssa@rosenzweig•io>,
	Daniel Vetter <daniel@ffwll•ch>,
	Steven Price <steven.price@arm•com>,
	Robin Murphy <robin.murphy@arm•com>,
	Linux ARM <linux-arm-kernel@lists•infradead.org>
Subject: Re: [PATCH v2 2/3] drm: Add a drm_gem_objects_lookup helper
Date: Tue, 09 Apr 2019 09:55:27 -0700	[thread overview]
Message-ID: <878swjnmo0.fsf@anholt.net> (raw)
In-Reply-To: <CAL_Jsq+So-kadir8o8T-YNYkfOYD8vopAS3UWGbSdRVLjH_JGQ@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --]

Rob Herring <robh@kernel•org> writes:

> On Mon, Apr 1, 2019 at 10:43 AM Eric Anholt <eric@anholt•net> wrote:
>>
>> Chris Wilson <chris@chris-wilson•co.uk> writes:
>>
>> > Quoting Daniel Vetter (2019-04-01 14:06:48)
>> >> On Mon, Apr 1, 2019 at 9:47 AM Rob Herring <robh@kernel•org> wrote:
>> >> > +{
>> >> > +       int i, ret = 0;
>> >> > +       struct drm_gem_object *obj;
>> >> > +
>> >> > +       spin_lock(&filp->table_lock);
>> >> > +
>> >> > +       for (i = 0; i < count; i++) {
>> >> > +               /* Check if we currently have a reference on the object */
>> >> > +               obj = idr_find(&filp->object_idr, handle[i]);
>> >> > +               if (!obj) {
>> >> > +                       ret = -ENOENT;
>> >
>> > Unwind previous drm_gem_object_get(), the caller has no idea how many
>> > were processed before the error.
>>
>> I had the same thought, but the pattern we have is that you're loading
>> into a refcounted struct that will free the BOs when you're done,
>> anyway.
>
> The real bug here is if allocation of the array fails. The BO array
> may be NULL when the count is not. So this V3D cleanup hunk:
>
> for (i = 0; i < exec->bo_count; i++)
>   drm_gem_object_put_unlocked(&exec->bo[i]->base.base);
>   kvfree(exec->bo);
>
> Needs to be wrapped with 'if (exec->bo)'. We have a similar problem
> with fence arrays too.

Yeah, seems legit.  Not really going to write new patches when I've got
month-old critical patches I can't get acked, though. :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel