Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6b37315d-74d0-713a-3544-b8d4c873488d@amd.com>
Date:   Thu, 27 Oct 2022 08:17:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v1 2/2] drm/gem: Check whether object is NULL in
 drm_gem_vunmap()
Content-Language: en-US
To:     Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, noralf@tronnes.org,
        Dan Carpenter <dan.carpenter@oracle.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
References: <20221026224640.7542-1-dmitry.osipenko@collabora.com>
 <20221026224640.7542-3-dmitry.osipenko@collabora.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <20221026224640.7542-3-dmitry.osipenko@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cmNEU3VockVOUTluZGg0SStyRXJma3N2ZUtXT2oxYTlKMjBwaURpNWdlaGt1?=
 =?utf-8?B?dGNqK2E1aEROOUlraEpOVkloVGRUdWdSZ1FhL2daRmhZNWFDeGhkeG5RdVNh?=
 =?utf-8?B?STNmZkJEMUhQVVJDU3JxQUh2YjRTTEo3dkxvZ211ME1uQkNMU3J6dnBnaEhM?=
 =?utf-8?B?STRjMWgxYjV6RkczdXI4Si9JVDhhQUF4THZZUUNVZmRnN1V5VHZ1RTdPRzhr?=
 =?utf-8?B?ZUM2QUg5SXEvWjArRFZ5alp3Ylc4U2l0b1haTUVBM2Z6QXRIb1NZYlNzbmha?=
 =?utf-8?B?OVJLMjdlazZKQmU4M2VRL2tETWZiVWpSY1FBRXBLNWR5d2NYd2p4anE0RUFn?=
 =?utf-8?B?aDRZelBISU1xaXRGQW5pYjA2RVFVdC9EeTV0Yzk3ZnYvelBMVU1pd2pyaHdG?=
 =?utf-8?B?aTRDdmdwQ2hDdmZOR1dZeWlHUkJUbExTZndxVWRPNGZSK0MzZU1icWNPTmh1?=
 =?utf-8?B?UjVHUFR2SDZsb1JSNFpFc1VpYnZnOVZVek5jQzJaWDd0MnZqcElIaDRKL0ly?=
 =?utf-8?B?YmRyUUZoUWY5VEtvNVhwVkIzVlVBbjFJTDIxS3FMYzFiSS9YaUpxZTA2VW5D?=
 =?utf-8?B?ZjVEMFltanNYMGNGZFA5VGlVSjNqVTFXL0Z1WFc0cTluNGVmUk9ZNzZlaklm?=
 =?utf-8?B?TnlhREw4SjBXMXh0eEl0TG90MUZFaEdZZTNadVBuUmdNaytZWDhzL2VsQWpW?=
 =?utf-8?B?K2YxK3Z0aXd0WHIwblRQeU9nTXEvZ3daenhIaGtZcHZESjZMeDJOK2ZEZDJr?=
 =?utf-8?B?VWZEd04rU09VS252cll2czR3WldmbW1CUCtjMFZlUWhtakpVcEdVQmswclVv?=
 =?utf-8?B?aUFCck4zcTdpaTNDeTYrY1hBTnJlcmk4dEVhL0l6aFNTUDkxQndMcy94WVlo?=
 =?utf-8?B?bGlSL3VOc2U3dWdNbm0vczY5M2FLRFpyLzdiMnZXM3haY2ZUcmhvejR4Vklo?=
 =?utf-8?B?WTltYWxtTXl5QnFvKzZtcnh2M24ybkxpNjcxODFCNXpVMWJtakg5QWhCQVds?=
 =?utf-8?B?eE56bGUzbHFSVzdmVitHUWF3ZElnUWs5NEp4MjRIWXdVUWU0UGthVGlkYkFT?=
 =?utf-8?B?ejE2ZlVtaHpqQm5rUHNpU0NLUWVzYUFlYy93L0tjcG5WbEhTMzRYMmFuYmlJ?=
 =?utf-8?B?OEVJTVZIQWhZWkpabGF1UVY1MUN6NnFLQ2V0UXRaRG93VlJNTHdBbDhxWVdC?=
 =?utf-8?B?d0Q2VjVzYndlTnI5VDVrRjN3WjJZOFViZDFGWDJhSHROOE9ZM2dSOGhiaEdF?=
 =?utf-8?B?UHhPS1ZISWF3Nmx3TnJIUk91dU5VTmdiQzMxTW5YdEVDdGZ2b1hlVDd2bUc4?=
 =?utf-8?B?cXhXQlNrTU5EOFV4RENBYThZTDVVdmVEQ2N5QTBXQ0lKU1hnM1RJSUdkRlBy?=
 =?utf-8?B?NHpFbnVVdEZ5Rm03ek5wZTBCVmtzVk1UUUcwcE5aZUg3MWtpRXRjSHNpeVZk?=
 =?utf-8?B?OE9TWWhXQTVkZzhHeGJOOTgvRDB3Vkl2TzdDQXFCbnJQWHQzNDI3NGRDS0Js?=
 =?utf-8?B?VFVlZVJUWXpYTzdxeHhmbmhkdFBaQTRrZlRJcGxaMXZBaVQ3eHRyZ1dxUExw?=
 =?utf-8?B?czNDTkFBdVRhRzhRMUt2Y1o4cjdFYnVHcStZYnVhb2RtNU12Tm9qNVN0ZHFm?=
 =?utf-8?B?dVIwL3VRbjdEcldwdXU2aFZETUY3SGN6WUhRT0FaZVkrb05iZlhyM29Dcisr?=
 =?utf-8?B?bElRL1lhVFFQVTVJVEpHa21pZkhRUitUbkRJVGNKTlQ5ZEVUMVB0TDg3SHQy?=
 =?utf-8?B?bnllV2N6Z0VNTHdIU2w0bXBGWkk0eUVoUVc3Y3BrN1NNWHUvbnNsWkFKY01K?=
 =?utf-8?B?Tlg4ZEJMekprc0NwVno1YWNKbHpTeUZ4SkNudkMzQTdPVjR3SEtIUFp6ODNy?=
 =?utf-8?B?T21jcXpUNVJzNGNLanFwS2tSRlo4UHN4RXBrMTV5d1NKWDF1eDBqbVVwNWVn?=
 =?utf-8?B?a1hINS9KOWwzcVprMCt2NVRXMEpMMDMyR3RIZ0VLd0RGb0IzejEyYnhWZ25r?=
 =?utf-8?B?N3dZWG1UVWE2VjA4UW9qc1hzdG95alNkQjdjYlB3SkV5RVpHekZqV1ZYazhz?=
 =?utf-8?B?bm5uM1ZFL0NNSUNTTllmZVlFdVp1Z3FJOG0xNmtIYmhZR3lxNkVJQ2dBdEg3?=
 =?utf-8?Q?AaEt9yxsU6XSKOPXxIBxkllbu?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f505b8c-83c1-48f7-7763-08dab7e2f1f2
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2022 06:17:37.8225
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2mMYtOOZlR6xYF24sbr67mf6YR5ydmSyVSFD4udni0eNPXsyZ08Ac4pCbDeO0eH7
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5305
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Am 27.10.22 um 00:46 schrieb Dmitry Osipenko:
> The drm_gem_vunmap() will crash with a NULL dereference if the passed
> object pointer is NULL. It wasn't a problem before we added the locking
> support to drm_gem_vunmap function because the mapping argument was always
> NULL together with the object. Make drm_gem_vunmap() functions to handle
> the NULL pointers better.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Link: https://lore.kernel.org/dri-devel/Y1kFEGxT8MVlf32V@kili/
> Fixes: 79e2cf2e7a19 ("drm/gem: Take reservation lock for vmap/vunmap operations")
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>   drivers/gpu/drm/drm_gem.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index b8db675e7fb5..ee0a246ff4ac 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1175,11 +1175,11 @@ EXPORT_SYMBOL(drm_gem_vmap);
>   
>   void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
>   {
> -	dma_resv_assert_held(obj->resv);
> -
> -	if (iosys_map_is_null(map))
> +	if (!obj || iosys_map_is_null(map))
>   		return;

I'm not very keen about that. Calling a function with all parameters 
NULL doesn't make much sense and is clearly a coding error. Hiding that 
somehow doesn't help but rather makes things worse.

The only execption to that are things like kfree() or *_put() which work 
with the lifetime of objects.

Why is the static checker complaining about that in the first place?

Regards,
Christian.

>   
> +	dma_resv_assert_held(obj->resv);
> +
>   	if (obj->funcs->vunmap)
>   		obj->funcs->vunmap(obj, map);
>   
> @@ -1202,6 +1202,9 @@ EXPORT_SYMBOL(drm_gem_vmap_unlocked);
>   
>   void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
>   {
> +	if (!obj || iosys_map_is_null(map))
> +		return;
> +
>   	dma_resv_lock(obj->resv, NULL);
>   	drm_gem_vunmap(obj, map);
>   	dma_resv_unlock(obj->resv);