Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

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

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

On Mon, 29 Aug 2022, Li Zhong wrote:

> On Mon, Aug 29, 2022 at 12:09 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Sun, 28 Aug 2022, Li Zhong wrote:
> >
> > > On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> > > > >
> > > > > uart_port_check() will return NULL pointer when state->uart_port is
> > > > > NULL. Check the return value before dereference it to avoid
> > > > > null-pointer-dereference error.
> > > >
> > > > Have you taken the locking into consideration?
> > > > If no, please do, if yes, expand your commit message to explain why
> > > > the current locking scheme doesn't prevent an error from happening.
> > > >
> > >
> > > The locking is taken into consideration but these three checks do not need to
> > > unlock in error-handling because unlock() will be called in the callers. Will
> > > add the comment in v2 patch.
> >
> > I think he meant you should indicate why the current locking doesn't cover
> > the case you're fixing, not whether this function should call unlock() or
> > not.
> >
> 
> Thanks for clarifications. The locking does not guarantee the return value of
> uart_port_check()  is not NULL.

Please put such explanation into the commit message like Andy was asking, 
thank you.

And make sure you properly mention what has changed for any new version 
of any patch you send so that Greg don't need to auto-mail you about it 
(and end up ignoring your patch).

> Actually in line 773 of this file
> (drivers/tty/serial/serial_core.c), uart_port_check() is also called in
> critical section but still there is check on whether the return value is NULL.

Existance of such a check elsewhere alone isn't enough to guarantee that 
the check is necessary (and not even that the check in that other place 
would be necessary). You need a deeper analysis than that. I'm not 
claiming its either way here, just pointing out to the direction/details 
you should consider while writing the analysis of the problem.


-- 
 i.