From patchwork Mon Jun 12 13:58:37 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans Verkuil X-Patchwork-Id: 13276569 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 93F82C88CB4 for ; Mon, 12 Jun 2023 14:00:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233523AbjFLN7s (ORCPT ); Mon, 12 Jun 2023 09:59:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46174 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232421AbjFLN7r (ORCPT ); Mon, 12 Jun 2023 09:59:47 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B443170B for ; Mon, 12 Jun 2023 06:59:18 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 9882F62987 for ; Mon, 12 Jun 2023 13:58:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 44A78C4339B; Mon, 12 Jun 2023 13:58:42 +0000 (UTC) From: Hans Verkuil To: linux-media@vger.kernel.org Cc: Zheng Zhang , Hans Verkuil Subject: [PATCH 1/3] media: cec: core: add adap_nb_transmit_canceled() callback Date: Mon, 12 Jun 2023 15:58:37 +0200 Message-Id: <20230612135839.254935-2-hverkuil-cisco@xs4all.nl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> References: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org A potential deadlock was found by Zheng Zhang with a local syzkaller instance. The problem is that when a non-blocking CEC transmit is canceled by calling cec_data_cancel, that in turn can call the high-level received() driver callback, which can call cec_transmit_msg() to transmit a new message. The cec_data_cancel() function is called with the adap->lock mutex held, and cec_transmit_msg() tries to take that same lock. The root cause is that the received() callback can either be used to pass on a received message (and then adap->lock is not held), or to report a canceled transmit (and then adap->lock is held). This is confusing, so create a new low-level adap_nb_transmit_canceled callback that reports back that a non-blocking transmit was canceled. And the received() callback is only called when a message is received, as was the case before commit f9d0ecbf56f4 ("media: cec: correctly pass on reply results") complicated matters. Reported-by: Zheng Zhang Signed-off-by: Hans Verkuil Fixes: f9d0ecbf56f4 ("media: cec: correctly pass on reply results") --- drivers/media/cec/core/cec-adap.c | 4 ++-- include/media/cec.h | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c index 241b1621b197..f8cb8542e5e9 100644 --- a/drivers/media/cec/core/cec-adap.c +++ b/drivers/media/cec/core/cec-adap.c @@ -385,8 +385,8 @@ static void cec_data_cancel(struct cec_data *data, u8 tx_status, u8 rx_status) cec_queue_msg_monitor(adap, &data->msg, 1); if (!data->blocking && data->msg.sequence) - /* Allow drivers to process the message first */ - call_op(adap, received, &data->msg); + /* Allow drivers to react to a canceled transmit */ + call_op(adap, adap_nb_transmit_canceled, &data->msg); cec_data_completed(data); } diff --git a/include/media/cec.h b/include/media/cec.h index abee41ae02d0..6556cc161dc0 100644 --- a/include/media/cec.h +++ b/include/media/cec.h @@ -121,14 +121,16 @@ struct cec_adap_ops { void (*adap_configured)(struct cec_adapter *adap, bool configured); int (*adap_transmit)(struct cec_adapter *adap, u8 attempts, u32 signal_free_time, struct cec_msg *msg); + void (*adap_nb_transmit_canceled)(struct cec_adapter *adap, + const struct cec_msg *msg); void (*adap_status)(struct cec_adapter *adap, struct seq_file *file); void (*adap_free)(struct cec_adapter *adap); - /* Error injection callbacks */ + /* Error injection callbacks, called without adap->lock held */ int (*error_inj_show)(struct cec_adapter *adap, struct seq_file *sf); bool (*error_inj_parse_line)(struct cec_adapter *adap, char *line); - /* High-level CEC message callback */ + /* High-level CEC message callback, called without adap->lock held */ int (*received)(struct cec_adapter *adap, struct cec_msg *msg); }; From patchwork Mon Jun 12 13:58:38 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans Verkuil X-Patchwork-Id: 13276568 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 EC2C6C88CB2 for ; Mon, 12 Jun 2023 13:59:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232004AbjFLN70 (ORCPT ); Mon, 12 Jun 2023 09:59:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45866 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229900AbjFLN7Z (ORCPT ); Mon, 12 Jun 2023 09:59:25 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B841B19AF for ; Mon, 12 Jun 2023 06:58:51 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id C178962983 for ; Mon, 12 Jun 2023 13:58:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6E7E9C433D2; Mon, 12 Jun 2023 13:58:43 +0000 (UTC) From: Hans Verkuil To: linux-media@vger.kernel.org Cc: Zheng Zhang , Hans Verkuil Subject: [PATCH 2/3] media: cec: core: add adap_unconfigured() callback Date: Mon, 12 Jun 2023 15:58:38 +0200 Message-Id: <20230612135839.254935-3-hverkuil-cisco@xs4all.nl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> References: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org The adap_configured() callback was called with the adap->lock mutex held if the 'configured' argument was false, and without the adap->lock mutex held if that argument was true. That was very confusing, and so split this up in a adap_unconfigured() callback and a high-level configured() callback. This also makes it easier to understand when the mutex is held: all low-level adap_* callbacks are called with the mutex held. All other callbacks are called without that mutex held. Signed-off-by: Hans Verkuil Fixes: f1b57164305d ("media: cec: add optional adap_configured callback") --- drivers/media/cec/core/cec-adap.c | 4 ++-- include/media/cec.h | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/media/cec/core/cec-adap.c b/drivers/media/cec/core/cec-adap.c index f8cb8542e5e9..17f780740e2b 100644 --- a/drivers/media/cec/core/cec-adap.c +++ b/drivers/media/cec/core/cec-adap.c @@ -1348,7 +1348,7 @@ static void cec_adap_unconfigure(struct cec_adapter *adap) cec_flush(adap); wake_up_interruptible(&adap->kthread_waitq); cec_post_state_event(adap); - call_void_op(adap, adap_configured, false); + call_void_op(adap, adap_unconfigured); } /* @@ -1539,7 +1539,7 @@ static int cec_config_thread_func(void *arg) adap->kthread_config = NULL; complete(&adap->config_completion); mutex_unlock(&adap->lock); - call_void_op(adap, adap_configured, true); + call_void_op(adap, configured); return 0; unconfigure: diff --git a/include/media/cec.h b/include/media/cec.h index 6556cc161dc0..9c007f83569a 100644 --- a/include/media/cec.h +++ b/include/media/cec.h @@ -113,12 +113,12 @@ struct cec_fh { #define CEC_FREE_TIME_TO_USEC(ft) ((ft) * 2400) struct cec_adap_ops { - /* Low-level callbacks */ + /* Low-level callbacks, called with adap->lock held */ int (*adap_enable)(struct cec_adapter *adap, bool enable); int (*adap_monitor_all_enable)(struct cec_adapter *adap, bool enable); int (*adap_monitor_pin_enable)(struct cec_adapter *adap, bool enable); int (*adap_log_addr)(struct cec_adapter *adap, u8 logical_addr); - void (*adap_configured)(struct cec_adapter *adap, bool configured); + void (*adap_unconfigured)(struct cec_adapter *adap); int (*adap_transmit)(struct cec_adapter *adap, u8 attempts, u32 signal_free_time, struct cec_msg *msg); void (*adap_nb_transmit_canceled)(struct cec_adapter *adap, @@ -131,6 +131,7 @@ struct cec_adap_ops { bool (*error_inj_parse_line)(struct cec_adapter *adap, char *line); /* High-level CEC message callback, called without adap->lock held */ + void (*configured)(struct cec_adapter *adap); int (*received)(struct cec_adapter *adap, struct cec_msg *msg); }; From patchwork Mon Jun 12 13:58:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans Verkuil X-Patchwork-Id: 13276571 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 284CCC88CB4 for ; Mon, 12 Jun 2023 14:00:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233694AbjFLOAX (ORCPT ); Mon, 12 Jun 2023 10:00:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46188 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233262AbjFLN7s (ORCPT ); Mon, 12 Jun 2023 09:59:48 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 935AA170E for ; Mon, 12 Jun 2023 06:59:19 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E845A62985 for ; Mon, 12 Jun 2023 13:58:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 98E12C433EF; Mon, 12 Jun 2023 13:58:44 +0000 (UTC) From: Hans Verkuil To: linux-media@vger.kernel.org Cc: Zheng Zhang , Hans Verkuil Subject: [PATCH 3/3] Documentation: media: cec: describe new callbacks Date: Mon, 12 Jun 2023 15:58:39 +0200 Message-Id: <20230612135839.254935-4-hverkuil-cisco@xs4all.nl> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> References: <20230612135839.254935-1-hverkuil-cisco@xs4all.nl> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Describe the new callbacks and clarify when the adap->lock mutex is held or not. Signed-off-by: Hans Verkuil --- Documentation/driver-api/media/cec-core.rst | 44 ++++++++++++++++----- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/Documentation/driver-api/media/cec-core.rst b/Documentation/driver-api/media/cec-core.rst index ae0d20798edc..f1ffdec388f3 100644 --- a/Documentation/driver-api/media/cec-core.rst +++ b/Documentation/driver-api/media/cec-core.rst @@ -109,9 +109,11 @@ your driver: int (*adap_monitor_all_enable)(struct cec_adapter *adap, bool enable); int (*adap_monitor_pin_enable)(struct cec_adapter *adap, bool enable); int (*adap_log_addr)(struct cec_adapter *adap, u8 logical_addr); - void (*adap_configured)(struct cec_adapter *adap, bool configured); + void (*adap_unconfigured)(struct cec_adapter *adap); int (*adap_transmit)(struct cec_adapter *adap, u8 attempts, u32 signal_free_time, struct cec_msg *msg); + void (*adap_nb_transmit_canceled)(struct cec_adapter *adap, + const struct cec_msg *msg); void (*adap_status)(struct cec_adapter *adap, struct seq_file *file); void (*adap_free)(struct cec_adapter *adap); @@ -122,8 +124,8 @@ your driver: ... }; -The seven low-level ops deal with various aspects of controlling the CEC adapter -hardware: +These low-level ops deal with various aspects of controlling the CEC adapter +hardware. They are all called with the mutex adap->lock held. To enable/disable the hardware:: @@ -179,14 +181,12 @@ can receive directed messages to that address. Note that adap_log_addr must return 0 if logical_addr is CEC_LOG_ADDR_INVALID. -Called when the adapter is fully configured or unconfigured:: +Called when the adapter is unconfigured:: - void (*adap_configured)(struct cec_adapter *adap, bool configured); + void (*adap_unconfigured)(struct cec_adapter *adap); -If configured == true, then the adapter is fully configured, i.e. all logical -addresses have been successfully claimed. If configured == false, then the -adapter is unconfigured. If the driver has to take specific actions after -(un)configuration, then that can be done through this optional callback. +The adapter is unconfigured. If the driver has to take specific actions after +unconfiguration, then that can be done through this optional callback. To transmit a new message:: @@ -207,6 +207,19 @@ The CEC_FREE_TIME_TO_USEC macro can be used to convert signal_free_time to microseconds (one data bit period is 2.4 ms). +To pass on the result of a canceled non-blocking transmit:: + + void (*adap_nb_transmit_canceled)(struct cec_adapter *adap, + const struct cec_msg *msg); + +This optional callback can be used to obtain the result of a canceled +non-blocking transmit with sequence number msg->sequence. This is +called if the transmit was aborted, the transmit timed out (i.e. the +hardware never signaled that the transmit finished), or the transmit +was successful, but the wait for the expected reply was either aborted +or it timed out. + + To log the current CEC hardware status:: void (*adap_status)(struct cec_adapter *adap, struct seq_file *file); @@ -372,7 +385,8 @@ Implementing the High-Level CEC Adapter --------------------------------------- The low-level operations drive the hardware, the high-level operations are -CEC protocol driven. The following high-level callbacks are available: +CEC protocol driven. The high-level callbacks are called without the adap->lock +mutex being held. The following high-level callbacks are available: .. code-block:: none @@ -384,9 +398,19 @@ CEC protocol driven. The following high-level callbacks are available: ... /* High-level CEC message callback */ + void (*configured)(struct cec_adapter *adap); int (*received)(struct cec_adapter *adap, struct cec_msg *msg); }; +Called when the adapter is configured:: + + void (*configured)(struct cec_adapter *adap); + +The adapter is fully configured, i.e. all logical addresses have been +successfully claimed. If the driver has to take specific actions after +configuration, then that can be done through this optional callback. + + The received() callback allows the driver to optionally handle a newly received CEC message::