Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs and events to clients

[Simon Horman <simon.horman@corigine.com>]

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);