Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231014181333.2579530-1-vamshigajjela@google.com>
 <0d5f9af2-22ac-4753-8c5a-76c4d44dcc9c@kernel.org> <7d5493f0-b9a6-5ddc-75a-9add2eb75a24@linux.intel.com>
In-Reply-To: <7d5493f0-b9a6-5ddc-75a-9add2eb75a24@linux.intel.com>
From:   VAMSHI GAJJELA <vamshigajjela@google.com>
Date:   Wed, 18 Oct 2023 19:31:52 +0530
Message-ID: <CAMTSyjojcrKzWWsDdnuO__x5Zwik5m1rW6c7xi2jETh5=jJrwg@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] serial: core: Potential overflow of frame_time
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, manugautam@google.com,
        Subhash Jadavani <sjadavani@google.com>,
        Channa Kadabi <kadabi@google.com>,
        Jiri Slaby <jirislaby@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 16, 2023 at 4:30=E2=80=AFPM Ilpo J=C3=A4rvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> 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 cas=
e
> > > 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 rea=
son?
> > (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 32bit ye=
t?)
> >
> > 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=C3=A4rvinen <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 inboxe=
s
> 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 t=
o
> 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 respon=
se
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 tha=
t.

Thanks Jiri Slaby & Ilpo J=C3=A4rvinen 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_p=
ort *p)
> > >                      * rather than after it is fully sent.
> > >                      * Roughly estimate 1 extra bit here with / 7.
> > >                      */
> > > -                   stop_delay =3D p->port.frame_time +
> > > DIV_ROUND_UP(p->port.frame_time, 7);
> > > +                   stop_delay =3D 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-ass=
isted CTS
> > > flow state */
> > >     unsigned int            mctrl;                  /* current modem =
ctrl
> > > settings */
> > > -   unsigned int            frame_time;             /* frame timing i=
n ns
> > > */
> > > +   unsigned long           frame_time;             /* frame timing i=
n ns
> > > */
>
> As with v1, u64 !=3D 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 *p=
ort,
> > > unsigned int baud);
> > >    */
> > >   static inline unsigned long uart_fifo_timeout(struct uart_port *por=
t)
> > >   {
> > > -   u64 fifo_timeout =3D (u64)READ_ONCE(port->frame_time) * port->fif=
osize;
> > > +   u64 fifo_timeout =3D READ_ONCE(port->frame_time) * port->fifosize=
;
> > >             /* Add .02 seconds of slop */
> > >     fifo_timeout +=3D 20 * NSEC_PER_MSEC;
> >
> >