RE: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls

[David Laight <David.Laight@ACULAB.COM>]

From: Jarkko Sonninen
> Sent: 13 March 2023 08:28
> 
> Add support for RS-485 in Exar USB adapters.
> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
> Gpio mode register is set to enable RS-485.

The locking is entirely dubious.
Summary:

Taking the lock to read the flags is pretty pointless.
You are only looking at one bit and nothing else is tied
to the lock.
Even a READ_ONCE() isn't needed.
> +	spin_lock_irqsave(&data->lock, flags);
> +	rs485_flags = data->rs485.flags;
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	if (rs485_flags & SER_RS485_ENABLED)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H;
> +	else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
> +

The ioctl read code reads the data unlocked.
> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
> +		return -EFAULT;
So could return old and new data if the ioctl write code
runs concurrently (and you get a hardware interrupt or page
fault mid-buffer).

The ioctl write code acquires the lock across a structure copy.
(which should be a structure copy, not a memcpy).
The only way the lock will have any effect is if multiple
threads are doing updates at the same time.
Code doing that won't work anyway.
> +	if (copy_from_user(&rs485, argp, sizeof(rs485)))
> +		return -EFAULT;
> +
> +	dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> +	rs485.flags &= SER_RS485_ENABLED;
> +	spin_lock_irqsave(&data->lock, flags);
> +	memcpy(&data->rs485, &rs485, sizeof(rs485));
> +	spin_unlock_irqrestore(&data->lock, flags);

In any case you one seem to be implementing one bit of
the flags - so the rest of the data can be ignored.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)