From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="HPGndlVu" Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.115]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 982391BB; Tue, 28 Nov 2023 05:49:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701179376; x=1732715376; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ho8ya67y0bcAC8iRE41B+MB3YSCUZNByqFAIWF4TZg8=; b=HPGndlVuHasbVuRsfz4yI9RO89sry5KLzLp2AkdsjazcIgwsgjFtzv6O oa1F/rZYDpgA04yo57DUT3vhQ5UucrlAk2jQuC772WTFjs1jA/RiqrhDt Ei1R0mWvvHeuJtQtn6OdGrlWa39/mycnacPbKL59QRq+wcHFn/ePFk8Be MWSaW6ZFPz5c4aENMcrB6Enov2Q64uC2m5jPB2em6et9RtSJvpSjkyx9C AmJ1JUb+Jsfr+vuLRgIK6wOOTzBi3J+Y+V/4u3fnKf2/BBeBE+hQe2IdJ g/SV5HFIp198uQO7G+sFzEf8hbWFxWx5M1pncDmtQD+k0by75r7ZxqE1/ w==; X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="392676347" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="392676347" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2023 05:49:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="761938211" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="761938211" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga007.jf.intel.com with SMTP; 28 Nov 2023 05:49:08 -0800 Received: by stinkbox (sSMTP sendmail emulation); Tue, 28 Nov 2023 15:49:08 +0200 Date: Tue, 28 Nov 2023 15:49:08 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Cc: Jani Nikula , Thomas Zimmermann , Emma Anholt , Jonathan Corbet , linux-kernel@vger.kernel.org, Samuel Holland , Sandy Huang , Jernej Skrabec , linux-doc@vger.kernel.org, Hans Verkuil , linux-rockchip@lists.infradead.org, Chen-Yu Tsai , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments Message-ID: References: <20231128-kms-hdmi-connector-state-v4-0-c7602158306e@kernel.org> <20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org> <87h6l66nth.fsf@intel.com> Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > Hi Jani, > > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > All the drm_connector_init variants take at least a pointer to the > > > device, connector and hooks implementation. > > > > > > However, none of them check their value before dereferencing those > > > pointers which can lead to a NULL-pointer dereference if the author > > > isn't careful. > > > > Arguably oopsing on the spot is preferrable when this can't be caused by > > user input. It's always a mistake that should be caught early during > > development. > > > > Not everyone checks the return value of drm_connector_init and friends, > > so those cases will lead to more mysterious bugs later. And probably > > oopses as well. > > So maybe we can do both then, with something like > > if (WARN_ON(!dev)) > return -EINVAL > > if (drm_WARN_ON(dev, !connector || !funcs)) > return -EINVAL; > > I'd still like to check for this, so we can have proper testing, and we > already check for those pointers in some places (like funcs in > drm_connector_init), so if we don't cover everything we're inconsistent. People will invariably cargo-cult this kind of stuff absolutely everywhere and then all your functions will have tons of dead code to check their arguments. I'd prefer not to go there usually. Should we perhaps start to use the (arguably hideous) - void f(struct foo *bar) + void f(struct foo bar[static 1]) syntax to tell the compiler we don't accept NULL pointers? Hmm. Apparently that has the same problem as using any other kind of array syntax in the prototype. That is, the compiler demands to know the definition of 'struct foo' even though we're passing in effectively a pointer. Sigh. -- Ville Syrjälä Intel 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 440C2C4167B for ; Tue, 28 Nov 2023 13:50:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Nz1sQeZ7nJ33HzTNbZC+kxVjAoFYFhLRfSXAdByek6Y=; b=AuHmAgUDE6O0Cz rFhlCBxsVXIVLSATye/ld9PDRvvA/5pfnXxfRdw0VoUKhMEdQcqhnL+qT7WADpr2pbZ/ElS70ihUJ 0CpGPlEZuUg/pcfdnOwe6B+RDyGpO6B68Qtvy20S0Cp+iw8Nmrlg+okEN3/pRBgmozf1FMTLY/fmI IUqJvxbcW4fEHMonhNeX0yk+zzai4gKfvXF1DQGH/afusG2NpZGrswxYA8w2sRBp9bY82mGZdU22V I0tKwSnhRxuCTF0zpR4bHdvW2m3rZLKGUjI2RDaRgTXztDeHPoha+8P/9j9iWylp1AHJhTrTAmSm+ EO/fNtJvbA/P9MafzwGA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r7yTC-005Pju-05; Tue, 28 Nov 2023 13:49:46 +0000 Received: from mgamail.intel.com ([192.55.52.115]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1r7yT8-005PiC-1O; Tue, 28 Nov 2023 13:49:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701179382; x=1732715382; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ho8ya67y0bcAC8iRE41B+MB3YSCUZNByqFAIWF4TZg8=; b=BkXECsL9o4udUY40LmKJ7iFUvkkuXhFJ4jfZlr6p0wDS+Ifxw2s1v8a1 Z77hAeysdaz/eHnY3NnTMS0ITckHxLhIu72q2c4rnq/bHu204ALGmrpBa cBypY45NZS/7UmdJED3XR/YLFS8P/bDXqEQsLOlp9MAvr4ijSAF8yQy1q J3a+vacWkjbm1ah/qBkaKSwZrGK+/VgpwnUqXje9s719eIzHlp//RyO7k JU/Cgg6hiZaOoqWutTlXiIAqHSsc8Tx7Qws8yzrnODUS5NT3dp3aaIEwD NPb+QAdbeNH063auyaC4Foln37cGg0mkvjGkD5HoRZqPEXwmndUi/8g8A w==; X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="392676339" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="392676339" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2023 05:49:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="761938211" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="761938211" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga007.jf.intel.com with SMTP; 28 Nov 2023 05:49:08 -0800 Received: by stinkbox (sSMTP sendmail emulation); Tue, 28 Nov 2023 15:49:08 +0200 Date: Tue, 28 Nov 2023 15:49:08 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Cc: Jani Nikula , Thomas Zimmermann , Emma Anholt , Jonathan Corbet , linux-kernel@vger.kernel.org, Samuel Holland , Sandy Huang , Jernej Skrabec , linux-doc@vger.kernel.org, Hans Verkuil , linux-rockchip@lists.infradead.org, Chen-Yu Tsai , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments Message-ID: References: <20231128-kms-hdmi-connector-state-v4-0-c7602158306e@kernel.org> <20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org> <87h6l66nth.fsf@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Patchwork-Hint: comment X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231128_054942_505413_D5B07BC7 X-CRM114-Status: GOOD ( 22.41 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > Hi Jani, > = > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > All the drm_connector_init variants take at least a pointer to the > > > device, connector and hooks implementation. > > > > > > However, none of them check their value before dereferencing those > > > pointers which can lead to a NULL-pointer dereference if the author > > > isn't careful. > > = > > Arguably oopsing on the spot is preferrable when this can't be caused by > > user input. It's always a mistake that should be caught early during > > development. > > = > > Not everyone checks the return value of drm_connector_init and friends, > > so those cases will lead to more mysterious bugs later. And probably > > oopses as well. > = > So maybe we can do both then, with something like > = > if (WARN_ON(!dev)) > return -EINVAL > = > if (drm_WARN_ON(dev, !connector || !funcs)) > return -EINVAL; > = > I'd still like to check for this, so we can have proper testing, and we > already check for those pointers in some places (like funcs in > drm_connector_init), so if we don't cover everything we're inconsistent. People will invariably cargo-cult this kind of stuff absolutely everywhere and then all your functions will have tons of dead code to check their arguments. I'd prefer not to go there usually. Should we perhaps start to use the (arguably hideous) - void f(struct foo *bar) + void f(struct foo bar[static 1]) syntax to tell the compiler we don't accept NULL pointers? Hmm. Apparently that has the same problem as using any other kind of array syntax in the prototype. That is, the compiler demands to know the definition of 'struct foo' even though we're passing in effectively a pointer. Sigh. -- = Ville Syrj=E4l=E4 Intel _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 724ACC4167B for ; Tue, 28 Nov 2023 13:50:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7WYd26RuIoa/VgLXS/TeXbE4fmRAyYRT+pjEcbCcucs=; b=he+8Q3EOmwsO0w 5N7gmoyJOActdVrsJEACRq0+xD9Oe7JDevUJ0wgSxcnxdk1LydnGOnMrTwXKMDmKxWA3fRyO6Ceqd cOovg2HkmJUjL5KksLrt8TlHnMeFJKCrtuul2/jnjtmD/+XnWNng5IcrbRbAoFzYnweTxmMPW8RY7 qQ/iFwRI9mV+95k3941rkzH3Ybxea3YB7mXZwYe5gpJr8DEI51gjLeRR9otYmmKc5r0nH0+9q95ie yqQBMPZDMw05cdsGUIA/Kqf/xp3rxvOIqmqHttM6pp5q6m3b3Sdx/nHmNjjH53Jbc0ZIpdoMQxiog Mg4e6CF1hfOPZNoEUJkg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r7yTC-005Pk5-1W; Tue, 28 Nov 2023 13:49:46 +0000 Received: from mgamail.intel.com ([192.55.52.115]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1r7yT8-005PiC-1O; Tue, 28 Nov 2023 13:49:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701179382; x=1732715382; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ho8ya67y0bcAC8iRE41B+MB3YSCUZNByqFAIWF4TZg8=; b=BkXECsL9o4udUY40LmKJ7iFUvkkuXhFJ4jfZlr6p0wDS+Ifxw2s1v8a1 Z77hAeysdaz/eHnY3NnTMS0ITckHxLhIu72q2c4rnq/bHu204ALGmrpBa cBypY45NZS/7UmdJED3XR/YLFS8P/bDXqEQsLOlp9MAvr4ijSAF8yQy1q J3a+vacWkjbm1ah/qBkaKSwZrGK+/VgpwnUqXje9s719eIzHlp//RyO7k JU/Cgg6hiZaOoqWutTlXiIAqHSsc8Tx7Qws8yzrnODUS5NT3dp3aaIEwD NPb+QAdbeNH063auyaC4Foln37cGg0mkvjGkD5HoRZqPEXwmndUi/8g8A w==; X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="392676339" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="392676339" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2023 05:49:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="761938211" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="761938211" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga007.jf.intel.com with SMTP; 28 Nov 2023 05:49:08 -0800 Received: by stinkbox (sSMTP sendmail emulation); Tue, 28 Nov 2023 15:49:08 +0200 Date: Tue, 28 Nov 2023 15:49:08 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Cc: Jani Nikula , Thomas Zimmermann , Emma Anholt , Jonathan Corbet , linux-kernel@vger.kernel.org, Samuel Holland , Sandy Huang , Jernej Skrabec , linux-doc@vger.kernel.org, Hans Verkuil , linux-rockchip@lists.infradead.org, Chen-Yu Tsai , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org, linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments Message-ID: References: <20231128-kms-hdmi-connector-state-v4-0-c7602158306e@kernel.org> <20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org> <87h6l66nth.fsf@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Patchwork-Hint: comment X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231128_054942_505413_D5B07BC7 X-CRM114-Status: GOOD ( 22.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > Hi Jani, > = > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > All the drm_connector_init variants take at least a pointer to the > > > device, connector and hooks implementation. > > > > > > However, none of them check their value before dereferencing those > > > pointers which can lead to a NULL-pointer dereference if the author > > > isn't careful. > > = > > Arguably oopsing on the spot is preferrable when this can't be caused by > > user input. It's always a mistake that should be caught early during > > development. > > = > > Not everyone checks the return value of drm_connector_init and friends, > > so those cases will lead to more mysterious bugs later. And probably > > oopses as well. > = > So maybe we can do both then, with something like > = > if (WARN_ON(!dev)) > return -EINVAL > = > if (drm_WARN_ON(dev, !connector || !funcs)) > return -EINVAL; > = > I'd still like to check for this, so we can have proper testing, and we > already check for those pointers in some places (like funcs in > drm_connector_init), so if we don't cover everything we're inconsistent. People will invariably cargo-cult this kind of stuff absolutely everywhere and then all your functions will have tons of dead code to check their arguments. I'd prefer not to go there usually. Should we perhaps start to use the (arguably hideous) - void f(struct foo *bar) + void f(struct foo bar[static 1]) syntax to tell the compiler we don't accept NULL pointers? Hmm. Apparently that has the same problem as using any other kind of array syntax in the prototype. That is, the compiler demands to know the definition of 'struct foo' even though we're passing in effectively a pointer. Sigh. -- = Ville Syrj=E4l=E4 Intel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 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 947B4C4167B for ; Tue, 28 Nov 2023 13:49:38 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C0D0610E532; Tue, 28 Nov 2023 13:49:37 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0611010E532 for ; Tue, 28 Nov 2023 13:49:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701179375; x=1732715375; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ho8ya67y0bcAC8iRE41B+MB3YSCUZNByqFAIWF4TZg8=; b=bLUBN1mZZou5l2DpSwqd76IRKi9lCnF/q03rmow5bzk4VnOvGAomiCuf ZsGChg6ngoHVcKaUurgXNNY5PASkeGIz+vtbaSpPXeS/BSsmGiDcXyQG/ 4T8WQjacTQKvvxdzGeFJQ5MX5tIaprmUeh3G5g7LNQGlwZu0+Y3Eja4x3 NsyxUasf3QMTOzg0yn0TVzFhGLnwy5aG5Wo2kk8MGlXnMgAgbANxbzLPj B40sIc9zCXFje6wMt/gDeN4bFU/luJ48jhUW6a464PtZXuHu8h876iloi G7sQhCSwmPWU2i0xsCkZiq0ZNEXtvu9twpdU69xeGToxLEljCgJaIaUJq w==; X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="392676344" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="392676344" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Nov 2023 05:49:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10907"; a="761938211" X-IronPort-AV: E=Sophos;i="6.04,233,1695711600"; d="scan'208";a="761938211" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga007.jf.intel.com with SMTP; 28 Nov 2023 05:49:08 -0800 Received: by stinkbox (sSMTP sendmail emulation); Tue, 28 Nov 2023 15:49:08 +0200 Date: Tue, 28 Nov 2023 15:49:08 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Subject: Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments Message-ID: References: <20231128-kms-hdmi-connector-state-v4-0-c7602158306e@kernel.org> <20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org> <87h6l66nth.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment 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: Emma Anholt , Samuel Holland , dri-devel@lists.freedesktop.org, Jonathan Corbet , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Sandy Huang , Hans Verkuil , linux-rockchip@lists.infradead.org, Chen-Yu Tsai , Jernej Skrabec , Thomas Zimmermann , linux-sunxi@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > Hi Jani, > > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > All the drm_connector_init variants take at least a pointer to the > > > device, connector and hooks implementation. > > > > > > However, none of them check their value before dereferencing those > > > pointers which can lead to a NULL-pointer dereference if the author > > > isn't careful. > > > > Arguably oopsing on the spot is preferrable when this can't be caused by > > user input. It's always a mistake that should be caught early during > > development. > > > > Not everyone checks the return value of drm_connector_init and friends, > > so those cases will lead to more mysterious bugs later. And probably > > oopses as well. > > So maybe we can do both then, with something like > > if (WARN_ON(!dev)) > return -EINVAL > > if (drm_WARN_ON(dev, !connector || !funcs)) > return -EINVAL; > > I'd still like to check for this, so we can have proper testing, and we > already check for those pointers in some places (like funcs in > drm_connector_init), so if we don't cover everything we're inconsistent. People will invariably cargo-cult this kind of stuff absolutely everywhere and then all your functions will have tons of dead code to check their arguments. I'd prefer not to go there usually. Should we perhaps start to use the (arguably hideous) - void f(struct foo *bar) + void f(struct foo bar[static 1]) syntax to tell the compiler we don't accept NULL pointers? Hmm. Apparently that has the same problem as using any other kind of array syntax in the prototype. That is, the compiler demands to know the definition of 'struct foo' even though we're passing in effectively a pointer. Sigh. -- Ville Syrjälä Intel