Re: [PATCH 11/11] USB: serial: io_edgeport: switch to synchronous commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: serial: io_edgeport: command buffer leak in write_cmd_usb() callers
- [Medium] usb: serial: io_edgeport: jiffies timeout passed to usb_bulk_msg()
--

commit 76948cdcd262d6f36f2d2e2ca6b395a61167a6ea
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: switch to synchronous commands

This commit converts Edgeport command writes in write_cmd_usb() from
asynchronous URB submissions to synchronous usb_bulk_msg() calls and
removes the edge_bulk_out_cmd_callback() completion handler.

> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index fbb8249bb5b74..3f04898a7cf2d 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
> @@ -52,6 +52,7 @@
>  #define MAX_NAME_LEN		64
>  
>  #define OPEN_TIMEOUT		(5*HZ)		/* 5 seconds */
> +#define EDGE_CMD_TIMEOUT	(5 * HZ)

[Severity: Medium]
Should this timeout be defined in milliseconds (5000) rather than jiffies?

write_cmd_usb() passes this value to usb_bulk_msg(), which forwards it to
usb_start_wait_urb() in drivers/usb/core/message.c:

	expire = (timeout > 0) ? msecs_to_jiffies(timeout) : MAX_SCHEDULE_TIMEOUT;

Because usb_bulk_msg() expects milliseconds, passing (5 * HZ) results in a
500 ms timeout when HZ is 100 or 1250 ms when HZ is 250 instead of the
intended 5 seconds.

[ ... ]

> @@ -766,44 +763,6 @@ static void edge_bulk_out_data_callback(struct urb *urb)
>  }
>  
>  
> -/*****************************************************************************
> - * BulkOutCmdCallback
> - *	this is the callback function for when we have finished sending a
> - *	command	on the bulk out endpoint.
> - *****************************************************************************/
> -static void edge_bulk_out_cmd_callback(struct urb *urb)
> -{
> -	struct edgeport_port *edge_port = urb->context;
> -	struct device *dev = &urb->dev->dev;
> -	int status = urb->status;
> -
> -	atomic_dec(&CmdUrbs);
> -	dev_dbg(dev, "%s - FREE URB %p (outstanding %d)\n", __func__, urb,
> -		atomic_read(&CmdUrbs));
> -
> -
> -	/* clean up the transfer buffer */
> -	kfree(urb->transfer_buffer);
> -
> -	/* Free the command urb */
> -	usb_free_urb(urb);
> -
> -	if (status) {
> -		dev_dbg(dev, "%s - nonzero write bulk status received: %d\n",
> -			__func__, status);
> -		return;
> -	}
> -
> -	/* tell the tty driver that something has changed */
> -	if (edge_port->open)
> -		tty_port_tty_wakeup(&edge_port->port->port);
> -
> -	/* we have completed the command */
> -	edge_port->commandPending = false;
> -	wake_up(&edge_port->wait_command);
> -}
> -
> -
>  /*****************************************************************************
>   * Driver tty interface functions
>   *****************************************************************************/
> @@ -2105,49 +2064,22 @@ static int send_iosp_ext_cmd(struct edgeport_port *edge_port,
>  static int write_cmd_usb(struct edgeport_port *edge_port,
>  					unsigned char *buffer, int length)
>  {
> -	struct edgeport_serial *edge_serial =
> -				usb_get_serial_data(edge_port->port->serial);
> +	struct usb_serial *serial = edge_port->port->serial;
> +	struct edgeport_serial *edge_serial = usb_get_serial_data(serial);
>  	struct device *dev = &edge_port->port->dev;
> -	int status = 0;
> -	struct urb *urb;
> +	int status;
>  
>  	usb_serial_debug_data(dev, __func__, length, buffer);
>  
> -	/* Allocate our next urb */
> -	urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!urb)
> -		return -ENOMEM;
> -
> -	atomic_inc(&CmdUrbs);
> -	dev_dbg(dev, "%s - ALLOCATE URB %p (outstanding %d)\n",
> -		__func__, urb, atomic_read(&CmdUrbs));
> -
> -	usb_fill_bulk_urb(urb, edge_serial->serial->dev,
> -			usb_sndbulkpipe(edge_serial->serial->dev,
> -					edge_serial->bulk_out_endpoint),
> -			buffer, length, edge_bulk_out_cmd_callback, edge_port);
> -
> -	edge_port->commandPending = true;
> -	status = usb_submit_urb(urb, GFP_KERNEL);
> +	status = usb_bulk_msg(serial->dev,
> +			usb_sndbulkpipe(serial->dev, edge_serial->bulk_out_endpoint),
> +			buffer, length, NULL, EDGE_CMD_TIMEOUT);
>  	if (status) {
> -		/* something went wrong */
> -		dev_err(dev, "%s - usb_submit_urb(write command) failed, status = %d\n",
> -			__func__, status);
> -		usb_free_urb(urb);
> -		atomic_dec(&CmdUrbs);
> +		dev_err(dev, "failed to send command: %d\n", status);
>  		return status;
>  	}
>  
> -#if 0
> -	wait_event(&edge_port->wait_command, !edge_port->commandPending);
> -
> -	if (edge_port->commandPending) {
> -		/* command timed out */
> -		dev_dbg(dev, "%s - command timed out\n", __func__);
> -		status = -EINVAL;
> -	}
> -#endif
> -	return status;
> +	return 0;
>  }

[Severity: High]
Does this leak the kmalloc()-allocated command buffer on every successful
command submission?

Previously, edge_bulk_out_cmd_callback() freed urb->transfer_buffer when the
URB completed. The three callers of write_cmd_usb() in
drivers/usb/serial/io_edgeport.c — send_iosp_ext_cmd(),
send_cmd_write_baud_rate(), and send_cmd_write_uart_register() — only free
their buffer (buffer or cmdBuffer) when write_cmd_usb() returns an error:

drivers/usb/serial/io_edgeport.c:send_iosp_ext_cmd() {
	buffer = kmalloc(10, GFP_KERNEL);
	if (!buffer)
		return -ENOMEM;
	...
	status = write_cmd_usb(edge_port, buffer, length);
	if (status) {
		/* something bad happened, let's free up the memory */
		kfree(buffer);
	}

	return status;
}

send_cmd_write_baud_rate() and send_cmd_write_uart_register() follow the
same pattern with cmdBuffer. Since usb_bulk_msg() does not free the caller's
transfer buffer, nothing frees buffer or cmdBuffer when write_cmd_usb()
returns 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021141154.2324793-1-johan@kernel.org?part=11