Re: [PATCH v2 1/3] serial: core: Potential overflow of frame_time

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3870 bytes --]

On Mon, 16 Oct 2023, Jiri Slaby wrote:

> On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > From: VAMSHI GAJJELA <vamshigajjela@google.com>
> > 
> > uart_update_timeout() sets a u64 value to an unsigned int frame_time.
> > While it can be cast to u32 before assignment, there's a specific case
> > where frame_time is cast to u64. Since frame_time consistently
> > participates in u64 arithmetic, its data type is converted to u64 to
> > eliminate the need for explicit casting.
> 
> And make the divisions by the order of magnutude slower for no good reason?
> (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 32bit yet?)
> 
> Unless you provide a reason it can overflow in real (in fact you spell the
> opposite in the text above):
> NACKED-by: Jiri Slaby <jirislaby@kernel.org>

I sorry but I have to concur Jiri heavily here,

NACKED-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@google.com>
> > ---
> > v2:
> > - use DIV64_U64_ROUND_UP with frame_time

Please, I barely managed to read your v1 and it's v2 already, don't send 
the next version this soon. There's absolutely no need to fill our inboxes 
with n versions of your patch over a weekend, just remain patient 
and wait reasonable amount of time to gather feedback, please. ...I know 
it's often hard to wait, it's hard for me too at times.

You even failed to convert the other divide done on ->frame_time to 
DIV64_u64_ROUND_UP(), which looks a mindboggling oversight to me.
So far you've managed to cause so many problems with these two attempts to 
fix a non-problem I heavily suggest you focus on something that doesn't 
relate to fixing types. It takes time to understand the related code when 
doing something as simple as type change, which you clearly did not have 
as demonstrated by you missing that other divide which can be trivially 
found with git grep.

> > 
> >   drivers/tty/serial/8250/8250_port.c | 2 +-
> >   include/linux/serial_core.h         | 4 ++--
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index 141627370aab..d1bf794498c4 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1510,7 +1510,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
> >   			 * rather than after it is fully sent.
> >   			 * Roughly estimate 1 extra bit here with / 7.
> >   			 */
> > -			stop_delay = p->port.frame_time +
> > DIV_ROUND_UP(p->port.frame_time, 7);
> > +			stop_delay = p->port.frame_time +
> > DIV64_U64_ROUND_UP(p->port.frame_time, 7);
> >   		}
> >     		__stop_tx_rs485(p, stop_delay);
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index bb6f073bc159..b128513b009a 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -558,7 +558,7 @@ struct uart_port {
> >     	bool			hw_stopped;		/* sw-assisted CTS
> > flow state */
> >   	unsigned int		mctrl;			/* current modem ctrl
> > settings */
> > -	unsigned int		frame_time;		/* frame timing in ns
> > */
> > +	unsigned long		frame_time;		/* frame timing in ns
> > */

As with v1, u64 != unsigned long, I hope you do know that much about 
different architectures...

-- 
 i.

> >   	unsigned int		type;			/* port type */
> >   	const struct uart_ops	*ops;
> >   	unsigned int		custom_divisor;
> > @@ -764,7 +764,7 @@ unsigned int uart_get_divisor(struct uart_port *port,
> > unsigned int baud);
> >    */
> >   static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> >   {
> > -	u64 fifo_timeout = (u64)READ_ONCE(port->frame_time) * port->fifosize;
> > +	u64 fifo_timeout = READ_ONCE(port->frame_time) * port->fifosize;
> >     	/* Add .02 seconds of slop */
> >   	fifo_timeout += 20 * NSEC_PER_MSEC;
> 
>