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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5298CC43334 for ; Sun, 24 Jul 2022 18:42:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9CAC711B4AD; Sun, 24 Jul 2022 18:41:58 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by gabe.freedesktop.org (Postfix) with ESMTPS id 278321125D2 for ; Sun, 24 Jul 2022 18:41:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1658688116; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gIe4/7e2xCRrMXD7HUhMOPa/pdJzjLUmsuMLjrqfl6A=; b=IE2KQqX/YT6s9pbtw7utDFBTycezBUd1Zf8bsV/NGc1DVX7lH2wPG7lwrGhwJED+bp3qpf ojhH93qer4CO6iMeJrCbIMAI3PcdmpAq/YUr0y0NxD2WCZ5wTcJO6Ty3Jyvyeh2TgaDF+n NSOqPBASXTyA98Heym4E793lhdfKpo0= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-537-nxT87W2tNraHhblTBCc3Cw-1; Sun, 24 Jul 2022 14:41:54 -0400 X-MC-Unique: nxT87W2tNraHhblTBCc3Cw-1 Received: by mail-wm1-f71.google.com with SMTP id r10-20020a05600c284a00b003a2ff6c9d6aso7492380wmb.4 for ; Sun, 24 Jul 2022 11:41:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=gIe4/7e2xCRrMXD7HUhMOPa/pdJzjLUmsuMLjrqfl6A=; b=B2Ih4TAG1dIEVXRAdeJOnvrncw4yGkuNwIEBvPWB7TWiuUWBPZGna13p8SYw/nDjSr IcmmEB0wkJEUz+7FLEoVeQbyvoq8hp8YNNLw5IX5JIIyKt1OaFKYR2HkWRfobxIAI43y J579DZ+xw423G9Tzbkra8OtuMo6cA9eCsK6twfAINEI6iYAFYojIQi5Czbk78HBuwf/Z qvhHm/Xeevvk0Dy91zNvaZGgBhJIoH6D0/rwkF2tZYIK4uqXIaHyQkUPxsv+fWnULvn9 DxwgUkmx5Ta6JO3VQG/1UwEIiqavm1XBdMRCkwK96N8amDay193QF/86q+LvpAfRtVWI KDrg== X-Gm-Message-State: AJIora/lsLHOMZS8oiUd34BsxdeeMENoKlBvzcIcC7b/2YuDoR+ctznK cMRS8ZSSukWoC3tFDbW4/k4XEsUjYcrpGWRkXx/+2RvdlhLxt5E2qpZ0m4TbDykLPraMmZneU5f XtdrPvc/xDN6yxTqSl6ihwDAT2tR1 X-Received: by 2002:a5d:6dae:0:b0:21d:ac34:d087 with SMTP id u14-20020a5d6dae000000b0021dac34d087mr6121756wrs.336.1658688113626; Sun, 24 Jul 2022 11:41:53 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sm5M97cVJghcjpTtHe7vDZxpG3Ni/nmo/EqvERBxjz+1W4Vb+26sc27+eLVyAgJWACpipLGg== X-Received: by 2002:a5d:6dae:0:b0:21d:ac34:d087 with SMTP id u14-20020a5d6dae000000b0021dac34d087mr6121744wrs.336.1658688113431; Sun, 24 Jul 2022 11:41:53 -0700 (PDT) Received: from [192.168.1.130] (205.pool92-176-231.dynamic.orange.es. [92.176.231.205]) by smtp.gmail.com with ESMTPSA id b6-20020a056000054600b0021badf3cb26sm12426838wrf.63.2022.07.24.11.41.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 24 Jul 2022 11:41:53 -0700 (PDT) Message-ID: <38128880-5b47-7ba0-54f3-97c4d6e04028@redhat.com> Date: Sun, 24 Jul 2022 20:41:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] drm: Prevent modeset helpers to access an uninitialized drm_mode_config To: Thomas Zimmermann , linux-kernel@vger.kernel.org References: <20220724123741.1268536-1-javierm@redhat.com> From: Javier Martinez Canillas In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=javierm@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , Dmitry Baryshkov , dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hello Thomas, Thanks for your feedback. On 7/24/22 20:24, Thomas Zimmermann wrote: > Hi Javier > > Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas: >> DRM drivers initialize the mode configuration with drmm_mode_config_init() >> and that function (among other things) initializes mutexes that are later >> used by modeset helpers. >> >> But the helpers should only attempt to grab those locks if the mode config >> was properly initialized. Otherwise it can lead to kernel oops. An example >> is when a DRM driver using the component framework does not initialize the >> drm_mode_config, because its .bind callback was not being executed due one >> of its expected sub-devices' driver failing to probe. >> >> Some drivers check the struct drm_driver.registered field as an indication >> on whether their .shutdown callback should call helpers to tearn down the >> mode configuration or not, but most drivers just assume that it is always >> safe to call helpers such as drm_atomic_helper_shutdown() during shutdown. >> >> Let make the DRM core more robust and prevent this to happen, by marking a >> struct drm_mode_config as initialized during drmm_mode_config_init(). that >> way helpers can check for it and not attempt to grab uninitialized mutexes. > > I disagree. This patch looks like cargo-cult programming and entirely > arbitrary. The solution here is to fix drivers. The actual test to > perform is to instrument the mutex implementation to detect > uninitialized mutexes. > While I do agree that drivers should be fixed, IMO we should try to make it hard for the kernel to crash. We already have checks in other DRM helpers to avoid accessing uninitialized data, so I don't see why we couldn't do the same here. I wrote this patch after fixing a bug in the drm/msm driver [0]. By looking at how other drivers handled this case, I'm pretty sure that they have the same problem. A warning is much better than a kernel crash during shutdown. [0]: https://patchwork.kernel.org/project/dri-devel/patch/20220724111327.1195693-1-javierm@redhat.com/ > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat 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 6EE72C433EF for ; Sun, 24 Jul 2022 18:42:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233166AbiGXSmB (ORCPT ); Sun, 24 Jul 2022 14:42:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60278 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229552AbiGXSl7 (ORCPT ); Sun, 24 Jul 2022 14:41:59 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 837E5DF08 for ; Sun, 24 Jul 2022 11:41:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1658688116; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gIe4/7e2xCRrMXD7HUhMOPa/pdJzjLUmsuMLjrqfl6A=; b=IE2KQqX/YT6s9pbtw7utDFBTycezBUd1Zf8bsV/NGc1DVX7lH2wPG7lwrGhwJED+bp3qpf ojhH93qer4CO6iMeJrCbIMAI3PcdmpAq/YUr0y0NxD2WCZ5wTcJO6Ty3Jyvyeh2TgaDF+n NSOqPBASXTyA98Heym4E793lhdfKpo0= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-490-7WZdxT8zPH2TD2RjyeEWKQ-1; Sun, 24 Jul 2022 14:41:55 -0400 X-MC-Unique: 7WZdxT8zPH2TD2RjyeEWKQ-1 Received: by mail-wm1-f70.google.com with SMTP id c62-20020a1c3541000000b003a30d86cb2dso7484632wma.5 for ; Sun, 24 Jul 2022 11:41:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=gIe4/7e2xCRrMXD7HUhMOPa/pdJzjLUmsuMLjrqfl6A=; b=CFT1mwZOb/0IicDAbcqGNeHXgvrAH2N73pmD5AS60uWkFiizyX1hlcd8kFR+c1z1or /zQhP8Xbkgf0tjmVTqjkV7EwiPtqzpMHx+PEgbIztD6LPEii6kGXAtbPOEw9z+AZqmQc LwVZki6/SabPT0xWa4Wwxg9LZZ2jOWlWsOEvKI8XIUyQXmCdSb4iJsoMV0eSGVkhM/XP QuSkldHnpqhniOjv65Lum6QsF9w3KTrWVdbYmt+SBaSYmt6CsQIRTTKbwjAQrBc1LQOh absbC13VfSpEf2WoqxDGVSlH/nFgeJG5Nl7/8o11OnYidZi4+r/8d6YC1/DIDKLb+/Hi dL0g== X-Gm-Message-State: AJIora8aZp23O7g1aUmtfyPLXShImWoj9+AkLhBphmPIvH2NlHEO1zQt kLKJQudmbke+TpftiZbBtcikL2XocTVFSJXyHYUY38yS3ZzTRPFHQG9v2SzpYgFsF98p8qF2bwe /pE5j8yNFF6ERZyc2NKNLxO6d X-Received: by 2002:a5d:6dae:0:b0:21d:ac34:d087 with SMTP id u14-20020a5d6dae000000b0021dac34d087mr6121753wrs.336.1658688113625; Sun, 24 Jul 2022 11:41:53 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sm5M97cVJghcjpTtHe7vDZxpG3Ni/nmo/EqvERBxjz+1W4Vb+26sc27+eLVyAgJWACpipLGg== X-Received: by 2002:a5d:6dae:0:b0:21d:ac34:d087 with SMTP id u14-20020a5d6dae000000b0021dac34d087mr6121744wrs.336.1658688113431; Sun, 24 Jul 2022 11:41:53 -0700 (PDT) Received: from [192.168.1.130] (205.pool92-176-231.dynamic.orange.es. [92.176.231.205]) by smtp.gmail.com with ESMTPSA id b6-20020a056000054600b0021badf3cb26sm12426838wrf.63.2022.07.24.11.41.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 24 Jul 2022 11:41:53 -0700 (PDT) Message-ID: <38128880-5b47-7ba0-54f3-97c4d6e04028@redhat.com> Date: Sun, 24 Jul 2022 20:41:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] drm: Prevent modeset helpers to access an uninitialized drm_mode_config Content-Language: en-US To: Thomas Zimmermann , linux-kernel@vger.kernel.org Cc: David Airlie , dri-devel@lists.freedesktop.org, Dmitry Baryshkov References: <20220724123741.1268536-1-javierm@redhat.com> From: Javier Martinez Canillas In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Thomas, Thanks for your feedback. On 7/24/22 20:24, Thomas Zimmermann wrote: > Hi Javier > > Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas: >> DRM drivers initialize the mode configuration with drmm_mode_config_init() >> and that function (among other things) initializes mutexes that are later >> used by modeset helpers. >> >> But the helpers should only attempt to grab those locks if the mode config >> was properly initialized. Otherwise it can lead to kernel oops. An example >> is when a DRM driver using the component framework does not initialize the >> drm_mode_config, because its .bind callback was not being executed due one >> of its expected sub-devices' driver failing to probe. >> >> Some drivers check the struct drm_driver.registered field as an indication >> on whether their .shutdown callback should call helpers to tearn down the >> mode configuration or not, but most drivers just assume that it is always >> safe to call helpers such as drm_atomic_helper_shutdown() during shutdown. >> >> Let make the DRM core more robust and prevent this to happen, by marking a >> struct drm_mode_config as initialized during drmm_mode_config_init(). that >> way helpers can check for it and not attempt to grab uninitialized mutexes. > > I disagree. This patch looks like cargo-cult programming and entirely > arbitrary. The solution here is to fix drivers. The actual test to > perform is to instrument the mutex implementation to detect > uninitialized mutexes. > While I do agree that drivers should be fixed, IMO we should try to make it hard for the kernel to crash. We already have checks in other DRM helpers to avoid accessing uninitialized data, so I don't see why we couldn't do the same here. I wrote this patch after fixing a bug in the drm/msm driver [0]. By looking at how other drivers handled this case, I'm pretty sure that they have the same problem. A warning is much better than a kernel crash during shutdown. [0]: https://patchwork.kernel.org/project/dri-devel/patch/20220724111327.1195693-1-javierm@redhat.com/ > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat