Re: [PATCH] tty: fix pid refcount leak in tty_signal_session_leader
On Sun, Jul 26, 2020 at 8:30 AM Xin Xiong wrote: > > In the loop, every time when p->signal->leader is true, the function > tty_signal_session_leader() will invoke get_pid() and return a > reference of tty->pgrp with increased refcount to the local variable > tty_pgrp or return NULL if it fails. After finishing the loop, the > function invokes put_pid() for only once, decreasing the refcount that > tty_pgrp keeps. > > Refcount leaks may occur when the scenario that p->signal->leader is > true happens more than once. In this assumption, if the above scenario > happens n times in the loop, the function forgets to decrease the > refcount for n-1 times, which causes refcount leaks. > > Fix the issue by decreasing the current refcount of the local variable > tty_pgrp before assigning new objects to it. > > Signed-off-by: Xiyu Yang > Signed-off-by: Xin Tan > Signed-off-by: Xin Xiong This SoB chain is out of order. If you are the author, your SoB should go first, if you are a commiter, the From line should correspond to the first SoB (not yours), if it's a group of authors (funny for one-/twoliner) then you consider to use Co-developed-by. Please, read Submitting Patches document. ... > put_pid(p->signal->tty_old_pgrp); /* A noop */ > spin_lock(>ctrl_lock); > + if (tty_pgrp) > + put_pid(tty_pgrp); > tty_pgrp = get_pid(tty->pgrp); > if (tty->pgrp) > p->signal->tty_old_pgrp = get_pid(tty->pgrp); I guess this patch wasn't thought thru. You see the get_pid for it happens twice in a row. Perhaps you have to get the logic behind all these first? P.S. ...on top of what Greg said. -- With Best Regards, Andy Shevchenko
Re: [PATCH] tty: fix pid refcount leak in tty_signal_session_leader
On Sun, Jul 26, 2020 at 01:28:04PM +0800, Xin Xiong wrote: > In the loop, every time when p->signal->leader is true, the function > tty_signal_session_leader() will invoke get_pid() and return a > reference of tty->pgrp with increased refcount to the local variable > tty_pgrp or return NULL if it fails. After finishing the loop, the > function invokes put_pid() for only once, decreasing the refcount that > tty_pgrp keeps. > > Refcount leaks may occur when the scenario that p->signal->leader is > true happens more than once. In this assumption, if the above scenario > happens n times in the loop, the function forgets to decrease the > refcount for n-1 times, which causes refcount leaks. > > Fix the issue by decreasing the current refcount of the local variable > tty_pgrp before assigning new objects to it. > > Signed-off-by: Xiyu Yang > Signed-off-by: Xin Tan > Signed-off-by: Xin Xiong > --- > drivers/tty/tty_jobctrl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c > index f8ed50a16848..9e6bf693ade1 100644 > --- a/drivers/tty/tty_jobctrl.c > +++ b/drivers/tty/tty_jobctrl.c > @@ -212,6 +212,8 @@ int tty_signal_session_leader(struct tty_struct *tty, int > exit_session) > __group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p); > put_pid(p->signal->tty_old_pgrp); /* A noop */ > spin_lock(>ctrl_lock); > + if (tty_pgrp) > + put_pid(tty_pgrp); No need to check this before calling it. But, the real question is why is this needed now? Nothing has changed in this area of the kernel for a very long time, so how did things get broken here? How are you triggering this and what is the result when we have that additional reference? thanks, greg k-h
[PATCH] tty: fix pid refcount leak in tty_signal_session_leader
In the loop, every time when p->signal->leader is true, the function tty_signal_session_leader() will invoke get_pid() and return a reference of tty->pgrp with increased refcount to the local variable tty_pgrp or return NULL if it fails. After finishing the loop, the function invokes put_pid() for only once, decreasing the refcount that tty_pgrp keeps. Refcount leaks may occur when the scenario that p->signal->leader is true happens more than once. In this assumption, if the above scenario happens n times in the loop, the function forgets to decrease the refcount for n-1 times, which causes refcount leaks. Fix the issue by decreasing the current refcount of the local variable tty_pgrp before assigning new objects to it. Signed-off-by: Xiyu Yang Signed-off-by: Xin Tan Signed-off-by: Xin Xiong --- drivers/tty/tty_jobctrl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c index f8ed50a16848..9e6bf693ade1 100644 --- a/drivers/tty/tty_jobctrl.c +++ b/drivers/tty/tty_jobctrl.c @@ -212,6 +212,8 @@ int tty_signal_session_leader(struct tty_struct *tty, int exit_session) __group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p); put_pid(p->signal->tty_old_pgrp); /* A noop */ spin_lock(>ctrl_lock); + if (tty_pgrp) + put_pid(tty_pgrp); tty_pgrp = get_pid(tty->pgrp); if (tty->pgrp) p->signal->tty_old_pgrp = get_pid(tty->pgrp); -- 2.25.1
