The global pointer 'sprd_port' maybe not zero when sprd_probe returns
fail, that is a risk for sprd_port to be accessed afterward, and will
lead unexpected errors.
For example:
There're two UART ports, UART1 is used for console and configured in kernel
command line, i.e. "console=";
The UART1 probe fail and the memory allocated to sprd_port[1] was released,
but sprd_port[1] was not set to NULL;
In UART2 probe, the same virtual address was allocated to sprd_port[2],
and UART2 probe process finally will go into sprd_console_setup() to
register UART1 as console since it is configured as preferred console
(filled to console_cmdline[]), but the console parameters (sprd_port[1])
actually belongs to UART2.
So move the sprd_port[] assignment to where the port already initialized
can avoid the above issue.
Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index b58f51296ace..942808517393 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
static int sprd_clk_init(struct uart_port *uport)
{
struct clk *clk_uart, *clk_parent;
- struct sprd_uart_port *u = sprd_port[uport->line];
+ struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
clk_uart = devm_clk_get(uport->dev, "uart");
if (IS_ERR(clk_uart)) {
@@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
{
struct resource *res;
struct uart_port *up;
+ struct sprd_uart_port *sport;
int irq;
int index;
int ret;
index = of_alias_get_id(pdev->dev.of_node, "serial");
- if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
+ if (index < 0 || index >= UART_NR_MAX) {
dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
return -EINVAL;
}
- sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
- GFP_KERNEL);
- if (!sprd_port[index])
+ sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
+ if (!sport)
return -ENOMEM;
- up = &sprd_port[index]->port;
+ up = &sport->port;
up->dev = &pdev->dev;
up->line = index;
up->type = PORT_SPRD;
@@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
* Allocate one dma buffer to prepare for receive transfer, in case
* memory allocation failure at runtime.
*/
- ret = sprd_rx_alloc_buf(sprd_port[index]);
+ ret = sprd_rx_alloc_buf(sport);
if (ret)
return ret;
@@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
}
sprd_ports_num++;
+ sprd_port[index] = sport;
+
ret = uart_add_one_port(&sprd_uart_driver, up);
if (ret)
- sprd_remove(pdev);
+ goto clean_port;
platform_set_drvdata(pdev, up);
+ return 0;
+
+clean_port:
+ sprd_port[index] = NULL;
+ sprd_ports_num--;
+ uart_unregister_driver(&sprd_uart_driver);
return ret;
}
--
2.41.0
Release DMA buffer when _probe() returns fail to avoid memory leak.
Fixes: f4487db58eb7 ("serial: sprd: Add DMA mode support")
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/tty/serial/sprd_serial.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
index 942808517393..e1f11382fc39 100644
--- a/drivers/tty/serial/sprd_serial.c
+++ b/drivers/tty/serial/sprd_serial.c
@@ -1203,7 +1203,7 @@ static int sprd_probe(struct platform_device *pdev)
ret = uart_register_driver(&sprd_uart_driver);
if (ret < 0) {
pr_err("Failed to register SPRD-UART driver\n");
- return ret;
+ goto free_rx_buf;
}
}
sprd_ports_num++;
@@ -1222,6 +1222,8 @@ static int sprd_probe(struct platform_device *pdev)
sprd_port[index] = NULL;
sprd_ports_num--;
uart_unregister_driver(&sprd_uart_driver);
+free_rx_buf:
+ sprd_rx_free_buf(sport);
return ret;
}
--
2.41.0
On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> Release DMA buffer when _probe() returns fail to avoid memory leak.
>
> Fixes: f4487db58eb7 ("serial: sprd: Add DMA mode support")
> Signed-off-by: Chunyan Zhang <[email protected]>
LGTM.
Reviewed-by: Baolin Wang <[email protected]>
> ---
> drivers/tty/serial/sprd_serial.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 942808517393..e1f11382fc39 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1203,7 +1203,7 @@ static int sprd_probe(struct platform_device *pdev)
> ret = uart_register_driver(&sprd_uart_driver);
> if (ret < 0) {
> pr_err("Failed to register SPRD-UART driver\n");
> - return ret;
> + goto free_rx_buf;
> }
> }
> sprd_ports_num++;
> @@ -1222,6 +1222,8 @@ static int sprd_probe(struct platform_device *pdev)
> sprd_port[index] = NULL;
> sprd_ports_num--;
> uart_unregister_driver(&sprd_uart_driver); > +free_rx_buf:
> + sprd_rx_free_buf(sport);
> return ret;
> }
>
On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> fail, that is a risk for sprd_port to be accessed afterward, and will
> lead unexpected errors.
>
> For example:
>
> There're two UART ports, UART1 is used for console and configured in kernel
> command line, i.e. "console=";
>
> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> but sprd_port[1] was not set to NULL;
IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
>
> In UART2 probe, the same virtual address was allocated to sprd_port[2],
> and UART2 probe process finally will go into sprd_console_setup() to
> register UART1 as console since it is configured as preferred console
> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> actually belongs to UART2.
I'm confusing why the console parameters belongs to UART2? Since the
console_cmdline[] will specify the serial index, that belongs to UART1.
Please correct me if I miss something.
> So move the sprd_port[] assignment to where the port already initialized
> can avoid the above issue.
>
> Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index b58f51296ace..942808517393 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
> static int sprd_clk_init(struct uart_port *uport)
> {
> struct clk *clk_uart, *clk_parent;
> - struct sprd_uart_port *u = sprd_port[uport->line];
> + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
>
> clk_uart = devm_clk_get(uport->dev, "uart");
> if (IS_ERR(clk_uart)) {
> @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
> {
> struct resource *res;
> struct uart_port *up;
> + struct sprd_uart_port *sport;
> int irq;
> int index;
> int ret;
>
> index = of_alias_get_id(pdev->dev.of_node, "serial");
> - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
> + if (index < 0 || index >= UART_NR_MAX) {
> dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
> return -EINVAL;
> }
>
> - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
> - GFP_KERNEL);
> - if (!sprd_port[index])
> + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
> + if (!sport)
> return -ENOMEM;
>
> - up = &sprd_port[index]->port;
> + up = &sport->port;
> up->dev = &pdev->dev;
> up->line = index;
> up->type = PORT_SPRD;
> @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
> * Allocate one dma buffer to prepare for receive transfer, in case
> * memory allocation failure at runtime.
> */
> - ret = sprd_rx_alloc_buf(sprd_port[index]);
> + ret = sprd_rx_alloc_buf(sport);
> if (ret)
> return ret;
>
> @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
> }
> sprd_ports_num++;
>
> + sprd_port[index] = sport;
> +
> ret = uart_add_one_port(&sprd_uart_driver, up);
> if (ret)
> - sprd_remove(pdev);
> + goto clean_port;
>
> platform_set_drvdata(pdev, up);
>
> + return 0;
> +
> +clean_port:
> + sprd_port[index] = NULL;
> + sprd_ports_num--;
> + uart_unregister_driver(&sprd_uart_driver);
> return ret;
> }
>
On Mon, Jul 10, 2023 at 01:36:07PM +0200, Markus Elfring wrote:
> > Release DMA buffer when _probe() returns fail to avoid memory leak.
>
> I would appreciate if this change description can be improved another bit.
Again, I would appreciate it if you do not provide reviews for any
kernel subsystems in which I am a maintainer of.
Please stop, it is not helpful at all.
greg k-h
On Mon, 10 Jul 2023 at 17:57, Baolin Wang <[email protected]> wrote:
>
>
>
> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> > The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> > fail, that is a risk for sprd_port to be accessed afterward, and will
> > lead unexpected errors.
> >
> > For example:
> >
> > There're two UART ports, UART1 is used for console and configured in kernel
> > command line, i.e. "console=";
> >
> > The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> > but sprd_port[1] was not set to NULL;
>
> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
This patch just does like this indeed, in the label of 'clean_port'.
Adding a local variable instead of using global pointer (sprd_port[])
to store the virtual address allocated for sprd_port can avoid
overmany goto labels.
>
> >
> > In UART2 probe, the same virtual address was allocated to sprd_port[2],
> > and UART2 probe process finally will go into sprd_console_setup() to
> > register UART1 as console since it is configured as preferred console
> > (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> > actually belongs to UART2.
>
> I'm confusing why the console parameters belongs to UART2? Since the
> console_cmdline[] will specify the serial index, that belongs to UART1.
The same virtual address stored in sprd_port[1] was reallocated to
sprd_port[2] after the UART1 probe returned failure.
Thanks for the review,
Chunyan
> Please correct me if I miss something.
>
> > So move the sprd_port[] assignment to where the port already initialized
> > can avoid the above issue.
> >
> > Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> > index b58f51296ace..942808517393 100644
> > --- a/drivers/tty/serial/sprd_serial.c
> > +++ b/drivers/tty/serial/sprd_serial.c
> > @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
> > static int sprd_clk_init(struct uart_port *uport)
> > {
> > struct clk *clk_uart, *clk_parent;
> > - struct sprd_uart_port *u = sprd_port[uport->line];
> > + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
> >
> > clk_uart = devm_clk_get(uport->dev, "uart");
> > if (IS_ERR(clk_uart)) {
> > @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
> > {
> > struct resource *res;
> > struct uart_port *up;
> > + struct sprd_uart_port *sport;
> > int irq;
> > int index;
> > int ret;
> >
> > index = of_alias_get_id(pdev->dev.of_node, "serial");
> > - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
> > + if (index < 0 || index >= UART_NR_MAX) {
> > dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
> > return -EINVAL;
> > }
> >
> > - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
> > - GFP_KERNEL);
> > - if (!sprd_port[index])
> > + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
> > + if (!sport)
> > return -ENOMEM;
> >
> > - up = &sprd_port[index]->port;
> > + up = &sport->port;
> > up->dev = &pdev->dev;
> > up->line = index;
> > up->type = PORT_SPRD;
> > @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
> > * Allocate one dma buffer to prepare for receive transfer, in case
> > * memory allocation failure at runtime.
> > */
> > - ret = sprd_rx_alloc_buf(sprd_port[index]);
> > + ret = sprd_rx_alloc_buf(sport);
> > if (ret)
> > return ret;
> >
> > @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
> > }
> > sprd_ports_num++;
> >
> > + sprd_port[index] = sport;
> > +
> > ret = uart_add_one_port(&sprd_uart_driver, up);
> > if (ret)
> > - sprd_remove(pdev);
> > + goto clean_port;
> >
> > platform_set_drvdata(pdev, up);
> >
> > + return 0;
> > +
> > +clean_port:
> > + sprd_port[index] = NULL;
> > + sprd_ports_num--;
> > + uart_unregister_driver(&sprd_uart_driver);
> > return ret;
> > }
> >
On Mon, 10 Jul 2023 at 19:36, Markus Elfring <[email protected]> wrote:
>
> > Release DMA buffer when _probe() returns fail to avoid memory leak.
>
> I would appreciate if this change description can be improved another bit.
>
I will try my best.
>
> Will a cover letter become helpful also for the presented small patch series?
These two patches are all fixes and describe the details in each,
that's why I didn't write a cover letter which I don't think can
provide more useful information.
Thanks for your review,
Chunyan
>
> Regards,
> Markus
On 7/11/2023 10:57 AM, Chunyan Zhang wrote:
> On Mon, 10 Jul 2023 at 17:57, Baolin Wang <[email protected]> wrote:
>>
>>
>>
>> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
>>> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
>>> fail, that is a risk for sprd_port to be accessed afterward, and will
>>> lead unexpected errors.
>>>
>>> For example:
>>>
>>> There're two UART ports, UART1 is used for console and configured in kernel
>>> command line, i.e. "console=";
>>>
>>> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
>>> but sprd_port[1] was not set to NULL;
>>
>> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
>
> This patch just does like this indeed, in the label of 'clean_port'.
> Adding a local variable instead of using global pointer (sprd_port[])
> to store the virtual address allocated for sprd_port can avoid
> overmany goto labels.
>
>>
>>>
>>> In UART2 probe, the same virtual address was allocated to sprd_port[2],
>>> and UART2 probe process finally will go into sprd_console_setup() to
>>> register UART1 as console since it is configured as preferred console
>>> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
>>> actually belongs to UART2.
>>
>> I'm confusing why the console parameters belongs to UART2? Since the
>> console_cmdline[] will specify the serial index, that belongs to UART1.
>
> The same virtual address stored in sprd_port[1] was reallocated to
> sprd_port[2] after the UART1 probe returned failure.
>
After more thinking, I understood your case. :)
But I see a nit in this patch, you added a 'clean_port' label to clear
the resource for the fail-probe-port instead of sprd_remove(), however
sprd_remove() will call sprd_rx_free_buf() to free the DMA buffer
originally. I know the 2nd patch will add it back, but patch 1 is not
git-bisect safe, right?
So I think you should also add sprd_rx_free_buf() under the 'clean_port'
label in patch 1, then patch 2 moves the sprd_rx_free_buf() to the
correct place.
>> Please correct me if I miss something.
>>
>>> So move the sprd_port[] assignment to where the port already initialized
>>> can avoid the above issue.
>>>
>>> Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support")
>>> Signed-off-by: Chunyan Zhang <[email protected]>
>>> ---
>>> drivers/tty/serial/sprd_serial.c | 24 ++++++++++++++++--------
>>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
>>> index b58f51296ace..942808517393 100644
>>> --- a/drivers/tty/serial/sprd_serial.c
>>> +++ b/drivers/tty/serial/sprd_serial.c
>>> @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport)
>>> static int sprd_clk_init(struct uart_port *uport)
>>> {
>>> struct clk *clk_uart, *clk_parent;
>>> - struct sprd_uart_port *u = sprd_port[uport->line];
>>> + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port);
>>>
>>> clk_uart = devm_clk_get(uport->dev, "uart");
>>> if (IS_ERR(clk_uart)) {
>>> @@ -1149,22 +1149,22 @@ static int sprd_probe(struct platform_device *pdev)
>>> {
>>> struct resource *res;
>>> struct uart_port *up;
>>> + struct sprd_uart_port *sport;
>>> int irq;
>>> int index;
>>> int ret;
>>>
>>> index = of_alias_get_id(pdev->dev.of_node, "serial");
>>> - if (index < 0 || index >= ARRAY_SIZE(sprd_port)) {
>>> + if (index < 0 || index >= UART_NR_MAX) {
>>> dev_err(&pdev->dev, "got a wrong serial alias id %d\n", index);
>>> return -EINVAL;
>>> }
>>>
>>> - sprd_port[index] = devm_kzalloc(&pdev->dev, sizeof(*sprd_port[index]),
>>> - GFP_KERNEL);
>>> - if (!sprd_port[index])
>>> + sport = devm_kzalloc(&pdev->dev, sizeof(*sport), GFP_KERNEL);
>>> + if (!sport)
>>> return -ENOMEM;
>>>
>>> - up = &sprd_port[index]->port;
>>> + up = &sport->port;
>>> up->dev = &pdev->dev;
>>> up->line = index;
>>> up->type = PORT_SPRD;
>>> @@ -1195,7 +1195,7 @@ static int sprd_probe(struct platform_device *pdev)
>>> * Allocate one dma buffer to prepare for receive transfer, in case
>>> * memory allocation failure at runtime.
>>> */
>>> - ret = sprd_rx_alloc_buf(sprd_port[index]);
>>> + ret = sprd_rx_alloc_buf(sport);
>>> if (ret)
>>> return ret;
>>>
>>> @@ -1208,12 +1208,20 @@ static int sprd_probe(struct platform_device *pdev)
>>> }
>>> sprd_ports_num++;
>>>
>>> + sprd_port[index] = sport;
>>> +
>>> ret = uart_add_one_port(&sprd_uart_driver, up);
>>> if (ret)
>>> - sprd_remove(pdev);
>>> + goto clean_port;
>>>
>>> platform_set_drvdata(pdev, up);
>>>
>>> + return 0;
>>> +
>>> +clean_port:
>>> + sprd_port[index] = NULL;
>>> + sprd_ports_num--;
>>> + uart_unregister_driver(&sprd_uart_driver);
>>> return ret;
>>> }
>>>
On Wed, 12 Jul 2023 at 09:39, Baolin Wang <[email protected]> wrote:
>
>
>
> On 7/11/2023 10:57 AM, Chunyan Zhang wrote:
> > On Mon, 10 Jul 2023 at 17:57, Baolin Wang <[email protected]> wrote:
> >>
> >>
> >>
> >> On 7/10/2023 4:03 PM, Chunyan Zhang wrote:
> >>> The global pointer 'sprd_port' maybe not zero when sprd_probe returns
> >>> fail, that is a risk for sprd_port to be accessed afterward, and will
> >>> lead unexpected errors.
> >>>
> >>> For example:
> >>>
> >>> There're two UART ports, UART1 is used for console and configured in kernel
> >>> command line, i.e. "console=";
> >>>
> >>> The UART1 probe fail and the memory allocated to sprd_port[1] was released,
> >>> but sprd_port[1] was not set to NULL;
> >>
> >> IMO, we should just set sprd_port[1] to be NULL, which seems simpler?
> >
> > This patch just does like this indeed, in the label of 'clean_port'.
> > Adding a local variable instead of using global pointer (sprd_port[])
> > to store the virtual address allocated for sprd_port can avoid
> > overmany goto labels.
> >
> >>
> >>>
> >>> In UART2 probe, the same virtual address was allocated to sprd_port[2],
> >>> and UART2 probe process finally will go into sprd_console_setup() to
> >>> register UART1 as console since it is configured as preferred console
> >>> (filled to console_cmdline[]), but the console parameters (sprd_port[1])
> >>> actually belongs to UART2.
> >>
> >> I'm confusing why the console parameters belongs to UART2? Since the
> >> console_cmdline[] will specify the serial index, that belongs to UART1.
> >
> > The same virtual address stored in sprd_port[1] was reallocated to
> > sprd_port[2] after the UART1 probe returned failure.
> >
>
> After more thinking, I understood your case. :)
>
> But I see a nit in this patch, you added a 'clean_port' label to clear
> the resource for the fail-probe-port instead of sprd_remove(), however
> sprd_remove() will call sprd_rx_free_buf() to free the DMA buffer
> originally. I know the 2nd patch will add it back, but patch 1 is not
> git-bisect safe, right?
Actually it doesn't make git-bisect unsafe, the driver can work with a
memory leak issue which has been there and fixed in another patch.
But I can add back sprd_remove() to keep the unrelated code logic the same.
Thanks for the review,
Chunyan
>
> So I think you should also add sprd_rx_free_buf() under the 'clean_port'
> label in patch 1, then patch 2 moves the sprd_rx_free_buf() to the
> correct place.
>