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: <7fccb1d3-cd05-d1f6-fa68-6c1828925d4c@amd.com>
Date:   Thu, 27 Oct 2022 09:40:27 +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:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     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,
        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>
 <6b37315d-74d0-713a-3544-b8d4c873488d@amd.com> <Y1ozCWTco6dzIA7J@kadam>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <Y1ozCWTco6dzIA7J@kadam>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WTJ2RHIySHBZTXRINXR3Qys4Sm51am54MURqMXZ3VXFwVWxmaHNQWVRBVEpn?=
 =?utf-8?B?WVUxY2tIMHVvYnpEZVFUZklQYmhjeEJBSTBxRW4wdm5JYVdBUDhTY3ZJTmMv?=
 =?utf-8?B?TXF2ZmJrMGR2T2JlbzhVV1g3eE5wSVJWSGN4K3k4VlR4TDA5T2NremdrbWJo?=
 =?utf-8?B?bVFFN01TVHc4V24wSm1Edm5FcTloVXdlS2cvbzhITUI2L0dSTUZHOHp0NHJE?=
 =?utf-8?B?YStZWjAyK0VmMnJyd1dMdEhlcExIS3dJMzRGdjZ6WUo4NUtYdFpqMXNpcGZa?=
 =?utf-8?B?UWZNQ0ZCMFQwNFV6SUVvbElpc1FHRnJLT3ErQTljTzc0dGNHRitKYnAwa0Ex?=
 =?utf-8?B?UE5MTSs1WkRkN3VzK1VYVHlCTjBCOW9pWGJIS1JhZkFVenVadHIyaGw5Q2l5?=
 =?utf-8?B?SDNmU0t2N3hGSC9DWVluVVMvWm5wYjY3anZtREROeW1kMXRRcHZxNENaNllk?=
 =?utf-8?B?N3V6QmVvbzNpZ0Q1M004aDl2RVhBZnMzNkt0ZlBncVpKN2ZMZWp3Q2lsOTIz?=
 =?utf-8?B?cTErTU9LeERVeDVZWDBIaXRnRVdDdzU5YStPSXM1Y3J3NmZveEdGVUY4OXNv?=
 =?utf-8?B?VUw5cWlYVXZBSTFRakNGZVNQQTJiWWFPOTVHZCtEUUJYVG5mU29wVWwxYzRq?=
 =?utf-8?B?NEVrOWQ2VEp1TTdVYXFKakcrNGhNVmxjSFBFTkNGQzl0RnJWNWVjU1pZZkpH?=
 =?utf-8?B?TnFoWTFjWlpPbk83NnZHdVo0S1poR0JvTU9iRUtvalp5d0FMMjFMYWFZL01Z?=
 =?utf-8?B?LzNSNGxhVXVxOSt3bXRPM0R2Tm1uYmN4dWt4MGJCMllsYkNlR1RzU2p0U0Ri?=
 =?utf-8?B?Z2xOU0RodUFiWGc1L2s1bnl6UmZtalE0V3pjSGErV2F2dENhbk84VWw3Zm1h?=
 =?utf-8?B?eitUQjJtNzBBZGJXYlhTanFLYlQ1NFFmVGpDYWRZVHJDU2p1THc0VVliWWIv?=
 =?utf-8?B?blhhS1ZVRVpKTHhZY1VWN08vdE4vdWFvNDkzTk9vTkF3OW1KUXZPLzFmNGJM?=
 =?utf-8?B?QjREWC9pUWlMbUtubFpobDdRYzMyR3RaU0J3UGtIWExPdkxpRWcrVmx1N2Rx?=
 =?utf-8?B?d3JkaWZuZEx6blN0M3FEMGV3dmV5Z0ZWRHpLUWZrRGJrQm14bVZJdTRIa2Za?=
 =?utf-8?B?RHZvQVlrMTBxczBqcGdnNWM3bkZvSUMxZFJiZk9wTlM3Q09KVTZXTEhFY3pn?=
 =?utf-8?B?VlBEb2hDL05sZW91Q0pRVFU3azlUWlRsdHMrQXBoU1BDRWtENkRVY2JmdFB2?=
 =?utf-8?B?bERRZkhEZ3hhaTQrWnFpZFo0TFQrb3ZYRVpZYVFRTFFKeDRMdVlZa1gxMkFM?=
 =?utf-8?B?VWRpQXl5MEZiLzh5aXpNWlY2d3d2NFM1L3FFNUkybElGVjRBdEhWcmVlbFpi?=
 =?utf-8?B?cU5CNis2SUNrRUU4eFpFZXFCRWNzUWEzQ1RDUVpMSGN2RVBNT2xOamt5OU5U?=
 =?utf-8?B?RXRNMXNoZlN4UldlU0NWZ1FBU0ppMlR4K1dIajNGSnExSmRRUUV2NnNuOXBD?=
 =?utf-8?B?d0c3ekIzQk5VT25abVVKeWlTVVVxTElTWVFYNlpjLzFSd2hCZ2V0cDgxdWJV?=
 =?utf-8?B?azJ5Y1oyS0hwTnZwTlI3em5PaVRYTndrMW9vbE1LeU05NWxLSmhiTFhRbXBx?=
 =?utf-8?B?dE5ac0tHem5XZFlmR0gxNDkzNVFwbFk1YUxyYjN6UHdscW1GbE1FNk5RZ0hh?=
 =?utf-8?B?MWIwTzQ2ZlNJRThGSjhraXNrQkhzZHJLK2lnWUI5TUc0NHdPVk5MYjdpaVUv?=
 =?utf-8?B?RnEvYXpDRVJQTTh5V0EwTEMwWUlqMHdBQmdGR2tYbUp0UHVDbUljb3EyU2Vz?=
 =?utf-8?B?NllQR2JRaWhObkxYNTIxa2dhTUdGSDVicklSMDdYTHNLeTBTZkpKRFB5bWN6?=
 =?utf-8?B?MDBkRkNyZSsyTTVmZldJbkVHQS93V1BxRGpad0ZZa3JzYXRoTko0bUtaNllr?=
 =?utf-8?B?enA2T2tKSmhKSDArYzFXTTNUOCtLK244S3dSK0M0NEtEb3NML29DWGhzb292?=
 =?utf-8?B?K2xsNHpyQkNEUGlDeGp1Z3AxSHlOR3ZCYjR3b2kwNFZEYXpqMlowa0xpQXNh?=
 =?utf-8?B?dGRoek9tTUhxNzgvZjlSZkxHZnBBQ3ZSZGgzS2k5ajhDVkc0ZDBIbnloc2ZZ?=
 =?utf-8?Q?aXKVqAlCNKFJy6xzYuVKYu+W5?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8976db38-4729-4995-c755-08dab7ee8793
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2022 07:40:33.3398
 (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: vSOJ7vNF2D6IYe4byWNRLkqVV3TXofJc8fNLqe131fA6gdl2fP4VR4AwVZHKwN/Y
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6433
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 09:28 schrieb Dan Carpenter:
> On Thu, Oct 27, 2022 at 08:17:31AM +0200, Christian König wrote:
>>
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2FY1kFEGxT8MVlf32V%40kili%2F&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C50d3b7e10eca4c7fb72108dab7ecd646%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638024525139985844%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ET0TRHscb1bQVVHgBkmYvTQyV2Q6WqcMC83LDlrY5ZM%3D&amp;reserved=0  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?
>>
> drivers/gpu/drm/drm_client.c:240 drm_client_buffer_delete()
> warn: variable dereferenced before check 'buffer->gem' (see line 238)

Well than rather modify drm_client_buffer_delete() instead.

Calling drm_gem_vunmap() with obj NULL is certainly not a good idea if 
we even check that for drm_gem_object_put().

Interesting that the static checkers complain about that :)

Regards,
Christian.

>
> regards,
> dan carpenter
>