Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <161dac7a-5aad-150e-7c14-7bb195ecf30f@canonical.com>
In-Reply-To: <161dac7a-5aad-150e-7c14-7bb195ecf30f@canonical.com>
From:   Matthew Auld <matthew.william.auld@gmail.com>
Date:   Mon, 12 Jul 2021 17:22:23 +0100
Message-ID: <CAM0jSHMO052Tcr_EMKDioOedHuX-foxz7_ejRVSLZuK8j+j9tA@mail.gmail.com>
Subject: Re: drm/i915/ttm Initialize the ttm device and memory managers
To:     Colin Ian King <colin.king@canonical.com>
Cc:     =?UTF-8?Q?Thomas_Hellstr=C3=B6m?= 
        <thomas.hellstrom@linux.intel.com>,
        Matthew Auld <matthew.auld@intel.com>,
        David Airlie <airlied@linux.ie>,
        intel-gfx <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 12 Jul 2021 at 16:17, Colin Ian King <colin.king@canonical.com> wro=
te:
>
> Hi,
>
> Static analysis with Coverity on linux-next has found a potential issue
> in drivers/gpu/drm/i915/selftests/intel_memory_region.c in function
> igt_mock_fill - the problematic commit is as follows:
>
> commit d148738923fdb5077089e48ec15555e6008100d0
> Author: Thomas Hellstr=C3=B6m <thomas.hellstrom@linux.intel.com>
> Date:   Wed Jun 2 10:38:08 2021 +0200
>
>     drm/i915/ttm Initialize the ttm device and memory managers
>
> The analysis is as follows:
>
>
>  49 static int igt_mock_fill(void *arg)
>  50 {
>  51        struct intel_memory_region *mem =3D arg;
>  52        resource_size_t total =3D resource_size(&mem->region);
>  53        resource_size_t page_size;
>  54        resource_size_t rem;
>  55        unsigned long max_pages;
>  56        unsigned long page_num;
>  57        LIST_HEAD(objects);
>  58        int err =3D 0;
>  59
>  60        page_size =3D mem->chunk_size;
>  61        rem =3D total;
>  62 retry:
>
> value_overwrite: Overwriting previous write to max_pages with value from
> div64_u64(rem, page_size).
>
>  63        max_pages =3D div64_u64(rem, page_size);
>  64
>  65        for_each_prime_number_from(page_num, 1, max_pages) {
>  66                resource_size_t size =3D page_num * page_size;
>  67                struct drm_i915_gem_object *obj;
>  68
>  69                obj =3D i915_gem_object_create_region(mem, size, 0);
>  70                if (IS_ERR(obj)) {
>  71                        err =3D PTR_ERR(obj);
>  72                        break;
>  73                }
>  74
>  75                err =3D i915_gem_object_pin_pages_unlocked(obj);
>  76                if (err) {
>  77                        i915_gem_object_put(obj);
>  78                        break;
>  79                }
>  80
>  81                list_add(&obj->st_link, &objects);
>  82                rem -=3D size;
>  83        }
>  84
>  85        if (err =3D=3D -ENOMEM)
>  86                err =3D 0;
>  87        if (err =3D=3D -ENXIO) {
>  88                if (page_num * page_size <=3D rem) {
>  89                        if (mem->is_range_manager && max_pages > 1) {
>
> Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from max_pages >> 1 to max_pages here,
> but that stored value is overwritten before it can be used.

Yeah, that doesn't look good.

AFAIK this should be fixed with d53ec322dc7d ("drm/i915/ttm: switch
over to ttm_buddy_man"), which is in drm-tip, but I guess has not made
its way over to linux-next yet.

Thanks for the report.

>
>  90                                max_pages >>=3D 1;
>  91                                goto retry;
>  92                        }
>  93
>
> The right shift to max_pages on line 90 is being overwritten on line 64
> on the retry.
>
> Colin