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;
Date:   Sat, 8 Jul 2023 14:36:56 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     Paolo Abeni <pabeni@redhat.com>,
        Alexandra Winter <wintera@linux.ibm.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        "David S. Miller" <davem@davemloft.net>,
        Stefan Raspl <raspl@linux.ibm.com>,
        Jan Karcher <jaka@linux.ibm.com>, linux-s390@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients
Message-ID: <ZKlmeDUEZf7F8+HW@corigine.com>
References: <20230707104359.3324039-1-schnelle@linux.ibm.com>
 <20230707104359.3324039-2-schnelle@linux.ibm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230707104359.3324039-2-schnelle@linux.ibm.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?rCTtjhx8tY0KVaoeEBJyrGACQZIt2LsG1mWxMpBUKmpTIBEP6lbuylYVH99U?=
 =?us-ascii?Q?qNNK1debnoxE02rrTi8ZFopFKtBQSL3IOvcKb/otoXQ6Owwl9qV3cyaWu0mF?=
 =?us-ascii?Q?CsfCDCJpKbA+1AcaWHR8nPcV4ebBhScY43MjPPJXjlxygScxcsSJ4RcKBuZj?=
 =?us-ascii?Q?avUdGKJid9WHJCqq/0gIN2SiiW8sjeOmYdKBTXvHfnnUQGc0k1MEHtAtvBzL?=
 =?us-ascii?Q?ju8MLNQxLxTGVA2hneLzC3ufrcoGoS3kEMFqhHdDPo+KOxKCrq04bI5gNOcC?=
 =?us-ascii?Q?ZKl10hTwNEIFEDmbCb1+cjHywmHRBAvjyx3uPAUWA3rKvXfwZH9tEW8fAHtD?=
 =?us-ascii?Q?tiLe04vazCSLobPaRZK8XiFK6Nl3MklqFRua6+Wwi3S4Zc+85KRLbcdYw/Ka?=
 =?us-ascii?Q?SAlD0gPufFzIrMRaZ5TL7mEbMa4xNhUhzr7YViowyzz3gdJKn+nAxP6lNC8N?=
 =?us-ascii?Q?hWNKlV8ldM3Sa0WLlo7wA5YtDdz59to425Xz7Ht7isYtdGHW6r6JQwKXnsp9?=
 =?us-ascii?Q?zxtR9RQsbdAUrrZEq8llH/d9C1lUdL6lFXjle667JQIdMoJVp5qfI6LDIsRL?=
 =?us-ascii?Q?M2J8A2Qbxc4QKY8I1fajWxoZ5yDHXoORS7VNsKseleMAIPirZhFIVim7UC7a?=
 =?us-ascii?Q?vNoPQuaB0sv8sSogxrIRbpnti44FHejlMzUOMV1wMtK8l82l3KO6YO2TPkpJ?=
 =?us-ascii?Q?DUoDRWGEwIQOBBmOBQbmye/eacWdViPQGhElCw+mr+JuCHpSe0+L9QUaaLWz?=
 =?us-ascii?Q?f4oJ/3EgxOv+p8MrnotERuQRTxHqXr7STf+MziUtf3XAd7lqLCfuEL6HFWeN?=
 =?us-ascii?Q?1U2oveRH+z91PAGvC7Dh1wdE70bU7B0nPrFt+JaNAVQ41wcHvQ9+OTcnAHct?=
 =?us-ascii?Q?y9t2S47fhkUrOH6Xf5NrTaolsgnxdHugYGPzKpfit++9zP7AJtkXKOCzsiKZ?=
 =?us-ascii?Q?NX749Kzq+Tnao9Cw1CdNXas5Aa7myHTNHakkCP58zmEnuFmA4THMEgFFyGzC?=
 =?us-ascii?Q?zLhbSdbOr/FzpvMUgswOqPIAZnhGtraI2eQE4VPoTPJO4ozNpkBFRuD74Lpo?=
 =?us-ascii?Q?hZMtfKMqAvICJy04DvOSt6VWjQxImFLfkg9FlWkQYpzfYyxUrI7SlYy016Jy?=
 =?us-ascii?Q?Vwu8UqlJ3gMeHl4PvEe+b8a4MbME09tA9IADxjtrtyVr2Za0+D/1wCScvY5X?=
 =?us-ascii?Q?JHJBHGo/G0pCfAi2dlKVtWhCrZ/GwKf1IbDOHJ4dHJiw7zyTH9Eq43Es/NkW?=
 =?us-ascii?Q?p5sijN/sbaTFSHqpFYuiTcxDl9bU1Toay8OHdVUdh39bMfRD+t9oSM2mrxBs?=
 =?us-ascii?Q?2yoo3A7ztdb632JhGbNdG3HT9fMC4v4VpMdoXcIN5xhvQQRuF/5gxt8FxcdA?=
 =?us-ascii?Q?wCAUDqacc119CHvl0NHIWfqCDEOWJozIuso6fpIBVb6mgM9TjxfIcc6a90mS?=
 =?us-ascii?Q?HUBUqWA4BDcy82s2xzwO6yJdd8MNn8t5uWBhp2wNUcsXuPTq8SK8F3XdIWyo?=
 =?us-ascii?Q?nFpVS4fw2CveAUVz170UKn3KnFxTwQVuGtaR5VY7YiYq2TmEJl7zlP6SzEcs?=
 =?us-ascii?Q?L0tmhtMMDqQ7OdLVZytvRiHjUcQkPUf2fOyq/wx+35dqFVhdpqQZPtmXcoGt?=
 =?us-ascii?Q?dQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9506ae66-5cea-4354-2c5d-08db7fb86a33
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jul 2023 13:37:03.8392
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: m/wWsTLpdc2bsBIiARXJ2eC+RwdrH5pAAb7exSG6Vz03/BDJEyXpuuwoDaxADKM28Hk32Q8nA+TdGb53KSbeJKeyC0x39zjGy/aiWLk4gWc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR13MB5985
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE 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

On Fri, Jul 07, 2023 at 12:43:57PM +0200, Niklas Schnelle wrote:
> The clients array references all registered clients and is protected by
> the clients_lock. Besides its use as general list of clients the clients
> array is accessed in ism_handle_irq() to forward ISM device events to
> clients.
> 
> While the clients_lock is taken in the IRQ handler when calling
> handle_event() it is however incorrectly not held during the
> client->handle_irq() call and for the preceding clients[] access leaving
> it unprotected against concurrent client (un-)registration.
> 
> Furthermore the accesses to ism->sba_client_arr[] in ism_register_dmb()
> and ism_unregister_dmb() are not protected by any lock. This is
> especially problematic as the client ID from the ism->sba_client_arr[]
> is not checked against NO_CLIENT and neither is the client pointer
> checked.
> 
> Instead of expanding the use of the clients_lock further add a separate
> array in struct ism_dev which references clients subscribed to the
> device's events and IRQs. This array is protected by ism->lock which is
> already taken in ism_handle_irq() and can be taken outside the IRQ
> handler when adding/removing subscribers or the accessing
> ism->sba_client_arr[]. This also means that the clients_lock is no
> longer taken in IRQ context.
> 
> Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

...

> @@ -71,6 +80,7 @@ int ism_register_client(struct ism_client *client)
>  		list_for_each_entry(ism, &ism_dev_list.list, list) {
>  			ism->priv[i] = NULL;
>  			client->add(ism);
> +			ism_setup_forwarding(client, ism);
>  		}
>  	}
>  	mutex_unlock(&ism_dev_list.mutex);

...

> @@ -92,6 +102,9 @@ int ism_unregister_client(struct ism_client *client)
>  		max_client--;
>  	spin_unlock_irqrestore(&clients_lock, flags);
>  	list_for_each_entry(ism, &ism_dev_list.list, list) {
> +		spin_lock_irqsave(&ism->lock, flags);

Hi Niklas,

The lock is taken here.

> +		/* Stop forwarding IRQs and events */
> +		ism->subs[client->id] = NULL;
>  		for (int i = 0; i < ISM_NR_DMBS; ++i) {
>  			if (ism->sba_client_arr[i] == client->id) {
>  				pr_err("%s: attempt to unregister client '%s'"
> @@ -101,6 +114,7 @@ int ism_unregister_client(struct ism_client *client)
>  				goto out;

But it does not appear to be released
(by the call to spin_unlock_irqrestore() below)
if goto out is called here.

>  			}
>  		}
> +		spin_unlock_irqrestore(&ism->lock, flags);
>  	}
>  out:
>  	mutex_unlock(&ism_dev_list.mutex);