Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <0ce8658b-9b8a-923c-a5bd-c63284d2db96@intel.com>
Date:   Fri, 29 Sep 2023 11:29:50 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] tipc: fix a potential deadlock on &tx->lock
Content-Language: en-US
To:     Chengfeng Ye <dg573847474@gmail.com>, <jmaloy@redhat.com>,
        <ying.xue@windriver.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
CC:     <netdev@vger.kernel.org>, <tipc-discussion@lists.sourceforge.net>,
        <linux-kernel@vger.kernel.org>
References: <20230927181414.59928-1-dg573847474@gmail.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230927181414.59928-1-dg573847474@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SzdCQU5qWkZrYjdxWDZ6MU92SXM3SW5oaEZSaXdKUkE4MHJRaDQ5bGlnbDFz?=
 =?utf-8?B?UlM2dFVZckFzVWFkbmJNQXF2U0tyUUZmMDFBVExSSjJyTk8remV0OVQvOHIw?=
 =?utf-8?B?dkNocnhPWVhLT2FsU1lYdTk3RGNKMThTWDdhWjNIK09JSkc3TFVaR0UyWjBH?=
 =?utf-8?B?VzU1L2NCQThrRnpTOEFKOXRtRG5PQ0E1d2xBaXFFTXJPdWlpK2hMdlZwTXpr?=
 =?utf-8?B?NlJUUmJjU2RidUQ5SE5DRWNVemxPVXVxMjlFQ1pNWmhNdUJjajVFWW5oQjdI?=
 =?utf-8?B?WTFMeGxRVjBuK3A0aDV5b1h6c2FSSmM0Zm9aYk5VR3h4bXloM1prNFVSODZZ?=
 =?utf-8?B?eVAvVVJLeURYeVdFYzIrNDhkUVdxeExkaTYwK2tiS0M3czVMbTJDSWlieVJj?=
 =?utf-8?B?ejdtWnJVWlBkQ3BXR1l3WlVVUEVHa3JoVG5zMVgrb3RhaTQ0cXZBNktYaGQr?=
 =?utf-8?B?WnIzNlFPU1VxUDlkdE1nR3BhNmFRQ0xSdUVxMitsM0dWQ2dxYXhZNHlzTkdp?=
 =?utf-8?B?MXdvdFhIc1V6V2FiVGRPWEMyUWlSRjBaUnVWKzQ0ZWdNSGRBZ1p1RVdOOUdH?=
 =?utf-8?B?Vkp1YWtvVERJZjRyWUxHQnhTMVU5STFBL0htT3BKZzhKekZHbnluVUNvU1g1?=
 =?utf-8?B?dDdManRjV2UrdzFhZVhqU0JGbHN1RGpzZmw3R01LUmk1NmQzaFhNQUF1T0RY?=
 =?utf-8?B?UGcyQjBBWmpMV1BVSU15OGU4bXBsd2hKZFJDYXVrYkJOM1BZVzQ1WjEvdENr?=
 =?utf-8?B?SG1GVkhaY1A1bFEwQjk4VEVzK0NudHpPNUpuazBIM1hwRStNTG5LOUhkWCt3?=
 =?utf-8?B?RHYyeTR2MkRyYmttOUtsVlNFQm5WQWxGSjhhZnkxNlp5d0xaNUh0R3FvQlVn?=
 =?utf-8?B?ejNCNFhmUUhoS1R3MDdxQUxueFdBaGtySXhvQUFLOWFpS0xBeVlrT0ljWXFP?=
 =?utf-8?B?dW9nM1JXTWhhbDlSY2NrRUFDSk0wQWRlY09xbzM3SHcxT2JTUDBJRzVrRGJG?=
 =?utf-8?B?RTNPOUt2b2Nubm5aVDc5ZUdiL1hBdThuTGo5ZStjY2U1THNObXlaaGtJcmdS?=
 =?utf-8?B?UFc3OWljcHZkN244a0U3eWhXYjk4MjRxUElMQnRuZ0w3Y0hvOXQ4T3RzTi9u?=
 =?utf-8?B?enNEWHdmN0JRWHN4bFhpaXNnYVdBam14eWYwWVlQMnhtQ1V6WXhBRjJ3di9t?=
 =?utf-8?B?SVZNTHJlM0RiMk5BckROVk9mQUN2M3NqZ0ZJMjNYSXNwN0hsemhtSWZxR2Vn?=
 =?utf-8?B?TnkveXdtWnRaYmR1eU4xWDdnWDhQc2Q3TGRTSDROOVYvc1BRRGRWZGgyYUxB?=
 =?utf-8?B?UytCQXpta3U4QkhTcEhUOHd2UU5zZS9pakllVEpLWlF6WnFxSjVVWEg2QlNN?=
 =?utf-8?B?TkRuZWFOS2RHTk1acVhEck1pcVJjRHRaWkJMeHJTd2NEek5kYkxzOG9DK1Av?=
 =?utf-8?B?Snh5dDhmbHJmQ0ZIRXVOMGszc2RMV0dNdnZicWdwVFhtZ1gxLzdqb0xJQ3o5?=
 =?utf-8?B?T2svMWlrREd2MkJWUHJZbXlTcXdYakJUYmlJdHI2RWJ1cWN3MU90UmdXc1Fk?=
 =?utf-8?B?c1B5K0EyR1pva2RPR1JHL2Z0SVRWUC91bTV6YjhsQUloVjFEWHlYVUNDb2xN?=
 =?utf-8?B?WE91SXNJQXdmSW5NMExoYmUwby9Jc25paW10TWtveWFsRW9nUndzWmNOMjFa?=
 =?utf-8?B?RWJSMmFKaS93dmt6dVNnWXM4cDNFaHUveDRvQU1GaGxCa0UxNHFZTnBBeENS?=
 =?utf-8?B?SmtpeFRPVGh6UVlrMEZqbDZEL3oyWnVJNDFyOGtONnkzNnY4R2Jnb204b0hC?=
 =?utf-8?B?cjZQUUZIZHlrYzE4RGh4WjhSWll3SzQwOGFXQktxeURkNk5SWEVTak01SVNV?=
 =?utf-8?B?c09palV3dndtVzVXVk5Kc1lCSTZyTk02TnRkM3dlWXJBUjZ4RUVTMUMyTFEw?=
 =?utf-8?B?Mlp5RHhyVFVPbmEzTVBsWkNNOGNjOTNkdCt2ek5qNktYQk45b09GeGdObUJv?=
 =?utf-8?B?RUFmaFMwb2llQ3JScnJESzlUM2F6OGZMRmlCbUE3d0N6OGQzbFVNZFQwZTl6?=
 =?utf-8?B?OVMzUkREbWVWODhHNHVWWC8wc3pUa0lNYlVPUUdZT3FOUStUa2FxMFB3dEJh?=
 =?utf-8?B?Mkg2QTdqaUgza3NldTdLU2t3SklUK2pkTzV3c1BPSTRGZnlFT0g5RnpiaEN2?=
 =?utf-8?B?V3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8fd6ad2a-025e-451b-ca28-08dbc11a132e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 18:29:53.9729
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RnFCSeR0Ll4AgMs2iTEG0Cd6M0OHB0roPwccrdnJ5HUK4GHLmr9kGT4vbInNC7YK0lCP7frcMlB3Yt6drBhW3E+UPFOA4kKb0ZwqVC1tlX0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7515
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Fri, 29 Sep 2023 13:18:12 -0700 (PDT)


On 9/27/2023 11:14 AM, Chengfeng Ye wrote:
> It seems that tipc_crypto_key_revoke() could be be invoked by
> wokequeue tipc_crypto_work_rx() under process context and
> timer/rx callback under softirq context, thus the lock acquisition
> on &tx->lock seems better use spin_lock_bh() to prevent possible
> deadlock.
> 
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
> 

Interesting.

> tipc_crypto_work_rx() <workqueue>
> --> tipc_crypto_key_distr()
> --> tipc_bcast_xmit()
> --> tipc_bcbase_xmit()
> --> tipc_bearer_bc_xmit()
> --> tipc_crypto_xmit()
> --> tipc_ehdr_build()
> --> tipc_crypto_key_revoke()
> --> spin_lock(&tx->lock)
> <timer interrupt>
>    --> tipc_disc_timeout()
>    --> tipc_bearer_xmit_skb()
>    --> tipc_crypto_xmit()
>    --> tipc_ehdr_build()
>    --> tipc_crypto_key_revoke()
>    --> spin_lock(&tx->lock) <deadlock here>
> 
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  net/tipc/crypto.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
> index 302fd749c424..43c3f1c971b8 100644
> --- a/net/tipc/crypto.c
> +++ b/net/tipc/crypto.c
> @@ -1441,14 +1441,14 @@ static int tipc_crypto_key_revoke(struct net *net, u8 tx_key)
>  	struct tipc_crypto *tx = tipc_net(net)->crypto_tx;
>  	struct tipc_key key;
>  
> -	spin_lock(&tx->lock);
> +	spin_lock_bh(&tx->lock);
>  	key = tx->key;
>  	WARN_ON(!key.active || tx_key != key.active);
>  
>  	/* Free the active key */
>  	tipc_crypto_key_set_state(tx, key.passive, 0, key.pending);
>  	tipc_crypto_key_detach(tx->aead[key.active], &tx->lock);
> -	spin_unlock(&tx->lock);
> +	spin_unlock_bh(&tx->lock);
>  
>  	pr_warn("%s: key is revoked\n", tx->name);
>  	return -EKEYREVOKED;