Re: [PATCH 3/9] firewire: cdev: add context analysis annotations for isochronous context

[Takashi Sakamoto]

On Mon, Nov 02, 2026 at 01:19:07PM +0900, Takashi Sakamoto wrote:
> A client can operate on a single isochronous context at a time. A mutex
> is used to ensure exclusive access to the context.
> 
> Use guard annotations to describe the synchronization requirements for
> accessing the context.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> ---
>  drivers/firewire/core-cdev.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 512afa589e2e..418b72cb8835 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -62,7 +62,7 @@ struct client {
>  	wait_queue_head_t tx_flush_wait;
>  	u64 bus_reset_closure;
>  
> -	struct fw_iso_context *iso_context;
> +	struct fw_iso_context *iso_context __pt_guarded_by(iso_context_mutex);
>  	struct mutex iso_context_mutex;
>  	u64 iso_closure;
>  	struct fw_iso_buffer buffer;
> @@ -1129,8 +1129,11 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
>  static int ioctl_set_iso_channels(struct client *client, union ioctl_arg *arg)
>  {
>  	struct fw_cdev_set_iso_channels *a = &arg->set_iso_channels;
> -	struct fw_iso_context *ctx = client->iso_context;
> +	struct fw_iso_context *ctx;
>  
> +	guard(mutex)(&client->iso_context_mutex);
> +
> +	ctx = client->iso_context;
>  	if (ctx == NULL || a->handle != 0)
>  		return -EINVAL;
>  
> @@ -1149,12 +1152,15 @@ static int ioctl_queue_iso(struct client *client, union ioctl_arg *arg)
>  {
>  	struct fw_cdev_queue_iso *a = &arg->queue_iso;
>  	struct fw_cdev_iso_packet __user *p, *end, *next;
> -	struct fw_iso_context *ctx = client->iso_context;
> +	struct fw_iso_context *ctx;
>  	unsigned long payload, buffer_end, transmit_header_bytes = 0;
>  	u32 control;
>  	int count;
>  	DEFINE_RAW_FLEX(struct fw_iso_packet, u, header, 64);
>  
> +	guard(mutex)(&client->iso_context_mutex);

Sashiko reports that get_user()/copy_from_user() under mutex locking can
cause A-B/B-A deadlock between the above mutex and mm->mmap_lock.

https://sashiko.dev/#/patchset/20261102041913.1187814-1-o-takashi%40sakamocchi.jp

Let me rework for the series.

> +	ctx = client->iso_context;
>  	if (ctx == NULL || a->handle != 0)
>  		return -EINVAL;
>
> @@ -1252,6 +1258,8 @@ static int ioctl_start_iso(struct client *client, union ioctl_arg *arg)
>  	    FW_CDEV_ISO_CONTEXT_MATCH_TAG3 != FW_ISO_CONTEXT_MATCH_TAG3 ||
>  	    FW_CDEV_ISO_CONTEXT_MATCH_ALL_TAGS != FW_ISO_CONTEXT_MATCH_ALL_TAGS);
>  
> +	guard(mutex)(&client->iso_context_mutex);
> +
>  	if (client->iso_context == NULL || a->handle != 0)
>  		return -EINVAL;
>  
> @@ -1267,6 +1275,8 @@ static int ioctl_stop_iso(struct client *client, union ioctl_arg *arg)
>  {
>  	struct fw_cdev_stop_iso *a = &arg->stop_iso;
>  
> +	guard(mutex)(&client->iso_context_mutex);
> +
>  	if (client->iso_context == NULL || a->handle != 0)
>  		return -EINVAL;
>  
> @@ -1277,6 +1287,8 @@ static int ioctl_flush_iso(struct client *client, union ioctl_arg *arg)
>  {
>  	struct fw_cdev_flush_iso *a = &arg->flush_iso;
>  
> +	guard(mutex)(&client->iso_context_mutex);
> +
>  	if (client->iso_context == NULL || a->handle != 0)
>  		return -EINVAL;
>  
> @@ -1937,8 +1949,10 @@ static int fw_device_op_release(struct inode *inode, struct file *file)
>  	scoped_guard(mutex, &client->device->client_list_mutex)
>  		list_del(&client->link);
>  
> -	if (client->iso_context)
> -		fw_iso_context_destroy(client->iso_context);
> +	scoped_guard(mutex, &client->iso_context_mutex) {
> +		if (client->iso_context)
> +			fw_iso_context_destroy(client->iso_context);
> +	}
>  	mutex_destroy(&client->iso_context_mutex);
>  
>  	if (client->buffer.pages)
> -- 
> 2.53.0

Regards

Takashi Sakamoto