[PATCH 1/3] Revert "USB: cdc-acm: fix rounding error in TIOCSSERIAL"

[PATCH 1/3] Revert "USB: cdc-acm: fix rounding error in TIOCSSERIAL"

[Johan Hovold]

This reverts commit b401f8c4f492cbf74f3f59c9141e5be3071071bb.

The offending commit claimed that trying to set the values reported back
by TIOCGSERIAL as a regular user could result in an -EPERM error when HZ
is 250, but that was never the case.

With HZ=250, the default 0.5 second value of close_delay is converted to
125 jiffies when set and is converted back to 50 centiseconds by
TIOCGSERIAL as expected (not 12 cs as was claimed).

Comparing the internal current and new jiffies values is just fine to
determine if the value is about to change so drop the bogus workaround
(which was also backported to stable).

For completeness: With different default values for these parameters or
with a HZ value not divisible by two, the lack of rounding when setting
the default values in tty_port_init() could result in an -EPERM being
returned, but this is hardly something we need to worry about.

Cc: Anthony Mallet <anthony.mallet@laas.fr>
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 3fda1ec961d7..96e221803fa6 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -942,7 +942,6 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 {
 	struct acm *acm = tty->driver_data;
 	unsigned int closing_wait, close_delay;
-	unsigned int old_closing_wait, old_close_delay;
 	int retval = 0;
 
 	close_delay = msecs_to_jiffies(ss->close_delay * 10);
@@ -950,17 +949,11 @@ static int set_serial_info(struct tty_struct *tty, struct serial_struct *ss)
 			ASYNC_CLOSING_WAIT_NONE :
 			msecs_to_jiffies(ss->closing_wait * 10);
 
-	/* we must redo the rounding here, so that the values match */
-	old_close_delay	= jiffies_to_msecs(acm->port.close_delay) / 10;
-	old_closing_wait = acm->port.closing_wait == ASYNC_CLOSING_WAIT_NONE ?
-				ASYNC_CLOSING_WAIT_NONE :
-				jiffies_to_msecs(acm->port.closing_wait) / 10;
-
 	mutex_lock(&acm->port.mutex);
 
 	if (!capable(CAP_SYS_ADMIN)) {
-		if ((ss->close_delay != old_close_delay) ||
-		    (ss->closing_wait != old_closing_wait))
+		if ((close_delay != acm->port.close_delay) ||
+		    (closing_wait != acm->port.closing_wait))
 			retval = -EPERM;
 		else
 			retval = -EOPNOTSUPP;
-- 
2.26.3

[PATCH 1/3] Revert "USB: cdc-acm: fix rounding error in TIOCSSERIAL"

[Anthony Mallet]

On Wednesday  7 Apr 2021, at 12:28, Johan Hovold wrote:
> With HZ=250, the default 0.5 second value of close_delay is converted to
> 125 jiffies when set and is converted back to 50 centiseconds by
> TIOCGSERIAL as expected (not 12 cs as was claimed).

It was "12" (instead of 50) because the conversion gor TIOCGSERIAL was
initially broken, and that was fixed in the previous commit
633e2b2ded739a34bd0fb1d8b5b871f7e489ea29

> For completeness: With different default values for these parameters or
> with a HZ value not divisible by two, the lack of rounding when setting
> the default values in tty_port_init() could result in an -EPERM being
> returned, but this is hardly something we need to worry about.

The -EPERM is harmful when a regular user wants to update other
members of serial_struct without changing the close delays,
e.g. ASYNC_LOW_LATENCY, which is granted to regular users.

Best,
Anthony

Re: [PATCH 1/3] Revert "USB: cdc-acm: fix rounding error in TIOCSSERIAL"

[Johan Hovold]

On Wed, Apr 07, 2021 at 01:04:31PM +0200, Anthony Mallet wrote:
> On Wednesday  7 Apr 2021, at 12:28, Johan Hovold wrote:
> > With HZ=250, the default 0.5 second value of close_delay is converted to
> > 125 jiffies when set and is converted back to 50 centiseconds by
> > TIOCGSERIAL as expected (not 12 cs as was claimed).
> 
> It was "12" (instead of 50) because the conversion gor TIOCGSERIAL was
> initially broken, and that was fixed in the previous commit
> 633e2b2ded739a34bd0fb1d8b5b871f7e489ea29

Right, so this patch is still just broken. The missing jiffies
conversion had already been added.

> > For completeness: With different default values for these parameters or
> > with a HZ value not divisible by two, the lack of rounding when setting
> > the default values in tty_port_init() could result in an -EPERM being
> > returned, but this is hardly something we need to worry about.
> 
> The -EPERM is harmful when a regular user wants to update other
> members of serial_struct without changing the close delays,
> e.g. ASYNC_LOW_LATENCY, which is granted to regular users.

You're missing the point; -EPERM will *not* be returned -- and this
patch was never needed.

Johan