* [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup @ 2020-06-13 12:52 Xiyu Yang 2020-06-24 9:34 ` Greg Kroah-Hartman 0 siblings, 1 reply; 4+ messages in thread From: Xiyu Yang @ 2020-06-13 12:52 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel Cc: yuanxzhang, kjlu, Xiyu Yang, Xin Tan uart_port_startup() invokes uart_port_lock(), which returns a reference of the uart_port object if increases the refcount of the uart_state object successfully or returns NULL if fails. However, uart_port_startup() don't take the return value of uart_port_lock() as the new uart_port object to "uport" and use the old "uport" instead to balance refcount in uart_port_unlock(), which may cause a redundant decrement of refcount occurred when the new "uport" equals to NULL and then cause a potential memory leak. Fix this issue by update the "uport" object to the return value of uart_port_lock() when invoking uart_port_lock(). Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> --- drivers/tty/serial/serial_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 57840cf90388..968fd619aec0 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -205,7 +205,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, if (!page) return -ENOMEM; - uart_port_lock(state, flags); + uport = uart_port_lock(state, flags); if (!state->xmit.buf) { state->xmit.buf = (unsigned char *) page; uart_circ_clear(&state->xmit); -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup 2020-06-13 12:52 [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup Xiyu Yang @ 2020-06-24 9:34 ` Greg Kroah-Hartman 2020-06-24 9:42 ` Jiri Slaby 0 siblings, 1 reply; 4+ messages in thread From: Greg Kroah-Hartman @ 2020-06-24 9:34 UTC (permalink / raw) To: Xiyu Yang Cc: Jiri Slaby, linux-serial, linux-kernel, yuanxzhang, kjlu, Xin Tan On Sat, Jun 13, 2020 at 08:52:18PM +0800, Xiyu Yang wrote: > uart_port_startup() invokes uart_port_lock(), which returns a reference > of the uart_port object if increases the refcount of the uart_state > object successfully or returns NULL if fails. > > However, uart_port_startup() don't take the return value of > uart_port_lock() as the new uart_port object to "uport" and use the old > "uport" instead to balance refcount in uart_port_unlock(), which may > cause a redundant decrement of refcount occurred when the new "uport" > equals to NULL and then cause a potential memory leak. > > Fix this issue by update the "uport" object to the return value of > uart_port_lock() when invoking uart_port_lock(). > > Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > --- > drivers/tty/serial/serial_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 57840cf90388..968fd619aec0 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -205,7 +205,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, > if (!page) > return -ENOMEM; > > - uart_port_lock(state, flags); > + uport = uart_port_lock(state, flags); How is this a different pointer than you originally had? And if it is a different pointer, shouldn't you be calling this function and using the pointer much earlier in the function instead of just here? Can you trigger a problem that this patch solves? If so, how? thanks, greg k-h ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup 2020-06-24 9:34 ` Greg Kroah-Hartman @ 2020-06-24 9:42 ` Jiri Slaby 2020-06-27 11:43 ` Greg Kroah-Hartman 0 siblings, 1 reply; 4+ messages in thread From: Jiri Slaby @ 2020-06-24 9:42 UTC (permalink / raw) To: Greg Kroah-Hartman, Xiyu Yang Cc: linux-serial, linux-kernel, yuanxzhang, kjlu, Xin Tan On 24. 06. 20, 11:34, Greg Kroah-Hartman wrote: > On Sat, Jun 13, 2020 at 08:52:18PM +0800, Xiyu Yang wrote: >> uart_port_startup() invokes uart_port_lock(), which returns a reference >> of the uart_port object if increases the refcount of the uart_state >> object successfully or returns NULL if fails. >> >> However, uart_port_startup() don't take the return value of >> uart_port_lock() as the new uart_port object to "uport" and use the old >> "uport" instead to balance refcount in uart_port_unlock(), which may >> cause a redundant decrement of refcount occurred when the new "uport" >> equals to NULL and then cause a potential memory leak. >> >> Fix this issue by update the "uport" object to the return value of >> uart_port_lock() when invoking uart_port_lock(). >> >> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> >> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> >> --- >> drivers/tty/serial/serial_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 57840cf90388..968fd619aec0 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -205,7 +205,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, >> if (!page) >> return -ENOMEM; >> >> - uart_port_lock(state, flags); >> + uport = uart_port_lock(state, flags); > > How is this a different pointer than you originally had? Was this patch sent twice? As I had very same questions on the other one, but never received a feedback: https://lore.kernel.org/linux-serial/bf6c1e7b-3dc6-aba6-955a-fee351a6d800@suse.com/ Oh, wait: this is uart_port_startup, I commented on the uart_shutdown one. But whatever, I would scratch both of them. > And if it is a different pointer, shouldn't you be calling this function > and using the pointer much earlier in the function instead of just here? > > Can you trigger a problem that this patch solves? If so, how? thanks, -- js suse labs ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup 2020-06-24 9:42 ` Jiri Slaby @ 2020-06-27 11:43 ` Greg Kroah-Hartman 0 siblings, 0 replies; 4+ messages in thread From: Greg Kroah-Hartman @ 2020-06-27 11:43 UTC (permalink / raw) To: Jiri Slaby Cc: Xiyu Yang, linux-serial, linux-kernel, yuanxzhang, kjlu, Xin Tan On Wed, Jun 24, 2020 at 11:42:59AM +0200, Jiri Slaby wrote: > On 24. 06. 20, 11:34, Greg Kroah-Hartman wrote: > > On Sat, Jun 13, 2020 at 08:52:18PM +0800, Xiyu Yang wrote: > >> uart_port_startup() invokes uart_port_lock(), which returns a reference > >> of the uart_port object if increases the refcount of the uart_state > >> object successfully or returns NULL if fails. > >> > >> However, uart_port_startup() don't take the return value of > >> uart_port_lock() as the new uart_port object to "uport" and use the old > >> "uport" instead to balance refcount in uart_port_unlock(), which may > >> cause a redundant decrement of refcount occurred when the new "uport" > >> equals to NULL and then cause a potential memory leak. > >> > >> Fix this issue by update the "uport" object to the return value of > >> uart_port_lock() when invoking uart_port_lock(). > >> > >> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn> > >> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com> > >> --- > >> drivers/tty/serial/serial_core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > >> index 57840cf90388..968fd619aec0 100644 > >> --- a/drivers/tty/serial/serial_core.c > >> +++ b/drivers/tty/serial/serial_core.c > >> @@ -205,7 +205,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state, > >> if (!page) > >> return -ENOMEM; > >> > >> - uart_port_lock(state, flags); > >> + uport = uart_port_lock(state, flags); > > > > How is this a different pointer than you originally had? > > Was this patch sent twice? As I had very same questions on the other > one, but never received a feedback: > https://lore.kernel.org/linux-serial/bf6c1e7b-3dc6-aba6-955a-fee351a6d800@suse.com/ > > > Oh, wait: this is uart_port_startup, I commented on the uart_shutdown > one. But whatever, I would scratch both of them. Yeah, you are right, dropping them both now, thanks. greg k-h ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-27 11:43 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-13 12:52 [PATCH] tty: serial_core: Fix uart_state refcnt leak when the port startup Xiyu Yang 2020-06-24 9:34 ` Greg Kroah-Hartman 2020-06-24 9:42 ` Jiri Slaby 2020-06-27 11:43 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).