[PATCH 1/4] igorplugusb: respect DMA coherency

[PATCH 1/4] igorplugusb: respect DMA coherency

[Oliver Neukum]

The coherency rules mean that you cannot embed
a buffer inside a descriptor. kmalloc() separately.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/media/rc/igorplugusb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index b40dbf500186..b46362da8623 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -38,7 +38,7 @@ struct igorplugusb {
 
 	struct timer_list timer;
 
-	uint8_t buf_in[MAX_PACKET];
+	u8 *buf_in;
 
 	char phys[64];
 };
@@ -177,6 +177,9 @@ static int igorplugusb_probe(struct usb_interface *intf,
 	if (!ir->urb)
 		goto fail;
 
+	ir->buf_in = kmalloc(MAX_PACKET, GFP_KERNEL);
+	if (!ir->buf_in)
+		goto fail;
 	usb_fill_control_urb(ir->urb, udev,
 		usb_rcvctrlpipe(udev, 0), (uint8_t *)&ir->request,
 		ir->buf_in, sizeof(ir->buf_in), igorplugusb_callback, ir);
@@ -223,6 +226,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
 	rc_free_device(ir->rc);
 	usb_free_urb(ir->urb);
 	del_timer(&ir->timer);
+	kfree(ir->buf_in);
 
 	return ret;
 }
@@ -236,6 +240,7 @@ static void igorplugusb_disconnect(struct usb_interface *intf)
 	usb_set_intfdata(intf, NULL);
 	usb_kill_urb(ir->urb);
 	usb_free_urb(ir->urb);
+	kfree(ir->buf_in);
 }
 
 static const struct usb_device_id igorplugusb_table[] = {
-- 
2.35.3

[PATCH 2/4] igorplugusb: prevent use after free in probe error

[Oliver Neukum]

The timer uses the URB. Free it only after the timer
has been stopped.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/media/rc/igorplugusb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index b46362da8623..1afba95409ff 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -223,9 +223,9 @@ static int igorplugusb_probe(struct usb_interface *intf,
 
 	return 0;
 fail:
-	rc_free_device(ir->rc);
-	usb_free_urb(ir->urb);
 	del_timer(&ir->timer);
+	usb_free_urb(ir->urb);
+	rc_free_device(ir->rc);
 	kfree(ir->buf_in);
 
 	return ret;
-- 
2.35.3

[PATCH 3/4] igorplugusb: break cyclical race on disconnect

[Oliver Neukum]

The driver uses a timer, that may submit the URB and
the URB may start the timer. No simple order of killing
can break te cycle. Poison the URB before killing
the timer.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/media/rc/igorplugusb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index 1afba95409ff..b2245849f7aa 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -126,7 +126,7 @@ static void igorplugusb_cmd(struct igorplugusb *ir, int cmd)
 	ir->request.bRequest = cmd;
 	ir->urb->transfer_flags = 0;
 	ret = usb_submit_urb(ir->urb, GFP_ATOMIC);
-	if (ret)
+	if (ret && ret != -EPERM)
 		dev_err(ir->dev, "submit urb failed: %d", ret);
 }
 
@@ -223,7 +223,9 @@ static int igorplugusb_probe(struct usb_interface *intf,
 
 	return 0;
 fail:
+	usb_poison_urb(ir->urb);
 	del_timer(&ir->timer);
+	usb_unpoison_urb(ir->urb);
 	usb_free_urb(ir->urb);
 	rc_free_device(ir->rc);
 	kfree(ir->buf_in);
@@ -236,9 +238,10 @@ static void igorplugusb_disconnect(struct usb_interface *intf)
 	struct igorplugusb *ir = usb_get_intfdata(intf);
 
 	rc_unregister_device(ir->rc);
+	usb_poison_urb(ir->urb);
 	del_timer_sync(&ir->timer);
 	usb_set_intfdata(intf, NULL);
-	usb_kill_urb(ir->urb);
+	usb_unpoison_urb(ir->urb);
 	usb_free_urb(ir->urb);
 	kfree(ir->buf_in);
 }
-- 
2.35.3

[PATCH 4/4] igorplugusb: remove superfluous usb_unlink_urb()

[Oliver Neukum]

Calling that on yourself while the completion handler
is running is a NOP. Remove it.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/media/rc/igorplugusb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index b2245849f7aa..12ee5dd0a61a 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -110,7 +110,6 @@ static void igorplugusb_callback(struct urb *urb)
 	case -ECONNRESET:
 	case -ENOENT:
 	case -ESHUTDOWN:
-		usb_unlink_urb(urb);
 		return;
 	default:
 		dev_warn(ir->dev, "Error: urb status = %d\n", urb->status);
-- 
2.35.3

Re: [PATCH 1/4] igorplugusb: respect DMA coherency

[Sean Young]

On Thu, May 12, 2022 at 02:38:46PM +0200, Oliver Neukum wrote:
> The coherency rules mean that you cannot embed
> a buffer inside a descriptor. kmalloc() separately.

I've merged this series to my tree. Thanks!

Sean
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/media/rc/igorplugusb.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
> index b40dbf500186..b46362da8623 100644
> --- a/drivers/media/rc/igorplugusb.c
> +++ b/drivers/media/rc/igorplugusb.c
> @@ -38,7 +38,7 @@ struct igorplugusb {
>  
>  	struct timer_list timer;
>  
> -	uint8_t buf_in[MAX_PACKET];
> +	u8 *buf_in;
>  
>  	char phys[64];
>  };
> @@ -177,6 +177,9 @@ static int igorplugusb_probe(struct usb_interface *intf,
>  	if (!ir->urb)
>  		goto fail;
>  
> +	ir->buf_in = kmalloc(MAX_PACKET, GFP_KERNEL);
> +	if (!ir->buf_in)
> +		goto fail;
>  	usb_fill_control_urb(ir->urb, udev,
>  		usb_rcvctrlpipe(udev, 0), (uint8_t *)&ir->request,
>  		ir->buf_in, sizeof(ir->buf_in), igorplugusb_callback, ir);
> @@ -223,6 +226,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
>  	rc_free_device(ir->rc);
>  	usb_free_urb(ir->urb);
>  	del_timer(&ir->timer);
> +	kfree(ir->buf_in);
>  
>  	return ret;
>  }
> @@ -236,6 +240,7 @@ static void igorplugusb_disconnect(struct usb_interface *intf)
>  	usb_set_intfdata(intf, NULL);
>  	usb_kill_urb(ir->urb);
>  	usb_free_urb(ir->urb);
> +	kfree(ir->buf_in);
>  }
>  
>  static const struct usb_device_id igorplugusb_table[] = {
> -- 
> 2.35.3