Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

[Christophe JAILLET]

Le 26/10/2022 à 13:12, Tharunkumar.Pasumarthi@xxxxxxxxxxxxx a écrit :
> On Mon, 2022-10-03 at 21:36 +0200, Christophe JAILLET wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > > +             }
> > > +             priv->line[i] = serial8250_register_8250_port(&uart);
> >
> > In case of error, this should be undone in an error handling path in the
> > probe, as done in the remove() function below.
> >
> > If we break, we still continue and return success. But the last
> > priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove()
> > is called?
>
> This will not cause a problem in pci1xxxx_serial_remove as this condition 'priv-
> > line[i] >= 0' will be checked for each of the ports before calling
> serial8250_unregister_port API.

Yes, this is my point.

We check for >=0 in pci1xxxx_serial_remove(), but if we have an error in 
the "for (i = 0; i < nr_ports; i++)" loop, some line[i] we'll still be 
zeroed because 'priv' is zalloc'ed.

In such a case, the probe still succeeds.

So, if pci1xxxx_serial_remove() is called later, we potentially call 
serial8250_unregister_port(0) several times.

This can lead to double (or more) free in serial8250_em485_destroy() 
(but maybe this path can't be taken here?) or maybe some other troubles 
elsewhere (I've not checked all the logic in uart_remove_one_port() to 
make sure that calling several times this function with the same args is 
fine).

So my point is maybe just a "can't happen" case.
If so, apologize for the noise.

CJ

> Thanks,
> Tharun Kumar P