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

[VAMSHI GAJJELA]

On Mon, Oct 16, 2023 at 4:30 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 16 Oct 2023, Jiri Slaby wrote:
>
> > On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > > From: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
>
> I sorry but I have to concur Jiri heavily here,
>
> NACKED-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>
> > > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
> > > ---
> > > 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.
Apologies for the inconvenience caused, I should have waited for the response
on v1 before making v2, by leaving a comment about the anticipated change.

I have clearly not considered all the architectures in the patch, and
the overhead
of division on 32-archs, mistake from my side. once again apologies for that.

Thanks Jiri Slaby & Ilpo Järvinen for the review.
>
> > >
> > >   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;
> >
> >