| Message ID | 93bf8f574310256fcea50e5c5a62b5c37e20bb14.1686285892.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 832e231cff476102e8204a9e7bddfe5c6154a375 |
| Headers | show |
| Series | [1/2] tty: serial: samsung_tty: Fix a memory leak in s3c24xx_serial_getclk() in case of error | expand |
On 09/06/2023 06:45, Christophe JAILLET wrote: > When the best clk is searched, we iterate over all possible clk. > > If we find a better match, the previous one, if any, needs to be freed. > If a better match has already been found, we still need to free the new > one, otherwise it leaks. > > Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is speculative. Review with care. > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 09/06/2023 10:57, Walter Harms wrote: > > while we are here .... > > perhaps INT_MAX from kernel.h ? > > int deviation = (1 << 30) - 1; > > the part before looks a bit strange > > if (ourport->info->has_divslot) { > unsigned long div = rate / req_baud; > > /* The UDIVSLOT register on the newer UARTs allows us to > * get a divisor adjustment of 1/16th on the baud clock. > * > * We don't keep the UDIVSLOT value (the 16ths we > * calculated by not multiplying the baud by 16) as it > * is easy enough to recalculate. > */ > > quot = div / 16; > baud = rate / div; > because > baud=rate/rate/req_baud = req_baud > can this be simplyfied ? (or is the numeric required ?) > > > Homebrew abs() kernel.h has a abs() can we use it here ? > > if (calc_deviation < 0) > calc_deviation = -calc_deviation; > > to the patch: > > + /* > + * If we find a better clk, release the previous one, if > + * any. > + */ > + if (!IS_ERR(*best_clk)) > + clk_put(*best_clk); > > the intentions are good. *best_clk is user supplied (and should be NULL) > filled & released in the next round but IMHO must be valid (is clk). > so no need to check. (ntl clk_put seems to handle NULL and ERR ) > if (!clk || WARN_ON_ONCE(IS_ERR(clk))) > return; Don't top-post. Anyway, I don't understand what you want to say here. Best regards, Krzysztof
Hi Christophe, On Fri, Jun 09, 2023 at 06:45:39AM +0200, Christophe JAILLET wrote: > When the best clk is searched, we iterate over all possible clk. > > If we find a better match, the previous one, if any, needs to be freed. > If a better match has already been found, we still need to free the new > one, otherwise it leaks. > > Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is speculative. Review with care. > > I think that some clk_put() are also missing somewhere else in the driver > but won't be able to investigate further. > --- > drivers/tty/serial/samsung_tty.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index dd751e7010e3..c07877dd25fa 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, > calc_deviation = -calc_deviation; > > if (calc_deviation < deviation) { > + /* > + * If we find a better clk, release the previous one, if > + * any. > + */ > + if (!IS_ERR(*best_clk)) what is the case when *best_clk has an error in it? Andi > + clk_put(*best_clk); > *best_clk = clk; > best_quot = quot; > *clk_num = cnt; > deviation = calc_deviation; > + } else { > + clk_put(clk); > } > } > > -- > 2.34.1 >
On 10/06/2023 12:39, Andi Shyti wrote: > Hi Christophe, > > On Fri, Jun 09, 2023 at 06:45:39AM +0200, Christophe JAILLET wrote: >> When the best clk is searched, we iterate over all possible clk. >> >> If we find a better match, the previous one, if any, needs to be freed. >> If a better match has already been found, we still need to free the new >> one, otherwise it leaks. >> >> Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> This patch is speculative. Review with care. >> >> I think that some clk_put() are also missing somewhere else in the driver >> but won't be able to investigate further. >> --- >> drivers/tty/serial/samsung_tty.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index dd751e7010e3..c07877dd25fa 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, >> calc_deviation = -calc_deviation; >> >> if (calc_deviation < deviation) { >> + /* >> + * If we find a better clk, release the previous one, if >> + * any. >> + */ >> + if (!IS_ERR(*best_clk)) > > what is the case when *best_clk has an error in it? The initial one? Open the place where the function is being called. Best regards, Krzysztof
Hi Krzysztof, > On Sat, Jun 10, 2023 at 12:45:53PM +0200, Krzysztof Kozlowski wrote: > On 10/06/2023 12:39, Andi Shyti wrote: > > Hi Christophe, > > > > On Fri, Jun 09, 2023 at 06:45:39AM +0200, Christophe JAILLET wrote: > >> When the best clk is searched, we iterate over all possible clk. > >> > >> If we find a better match, the previous one, if any, needs to be freed. > >> If a better match has already been found, we still need to free the new > >> one, otherwise it leaks. > >> > >> Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > >> --- > >> This patch is speculative. Review with care. > >> > >> I think that some clk_put() are also missing somewhere else in the driver > >> but won't be able to investigate further. > >> --- > >> drivers/tty/serial/samsung_tty.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > >> index dd751e7010e3..c07877dd25fa 100644 > >> --- a/drivers/tty/serial/samsung_tty.c > >> +++ b/drivers/tty/serial/samsung_tty.c > >> @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, > >> calc_deviation = -calc_deviation; > >> > >> if (calc_deviation < deviation) { > >> + /* > >> + * If we find a better clk, release the previous one, if > >> + * any. > >> + */ > >> + if (!IS_ERR(*best_clk)) > > > > what is the case when *best_clk has an error in it? > > The initial one? Open the place where the function is being called. Right! Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi > > Best regards, > Krzysztof >
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index dd751e7010e3..c07877dd25fa 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -1488,10 +1488,18 @@ static unsigned int s3c24xx_serial_getclk(struct s3c24xx_uart_port *ourport, calc_deviation = -calc_deviation; if (calc_deviation < deviation) { + /* + * If we find a better clk, release the previous one, if + * any. + */ + if (!IS_ERR(*best_clk)) + clk_put(*best_clk); *best_clk = clk; best_quot = quot; *clk_num = cnt; deviation = calc_deviation; + } else { + clk_put(clk); } }
When the best clk is searched, we iterate over all possible clk. If we find a better match, the previous one, if any, needs to be freed. If a better match has already been found, we still need to free the new one, otherwise it leaks. Fixes: 5f5a7a5578c5 ("serial: samsung: switch to clkdev based clock lookup") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- This patch is speculative. Review with care. I think that some clk_put() are also missing somewhere else in the driver but won't be able to investigate further. --- drivers/tty/serial/samsung_tty.c | 8 ++++++++ 1 file changed, 8 insertions(+)