[PATCH 1/2] Add missing rcu_read_lock for task_pgrp.

[PATCH 1/2] Add missing rcu_read_lock for task_pgrp.

[Patrick Donnelly]

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/tty_io.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..401d05e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -388,33 +388,40 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
 int tty_check_change(struct tty_struct *tty)
 {
 	unsigned long flags;
+	struct pid *pgrp;
 	int ret = 0;
 
 	if (current->signal->tty != tty)
 		return 0;
 
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
 
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (!tty->pgrp) {
 		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
-		goto out_unlock;
+		goto out_irqunlock;
 	}
-	if (task_pgrp(current) == tty->pgrp)
-		goto out_unlock;
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+	if (pgrp == tty->pgrp)
+		goto out_rcuunlock;
 	if (is_ignored(SIGTTOU))
 		goto out;
 	if (is_current_pgrp_orphaned()) {
 		ret = -EIO;
 		goto out;
 	}
-	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	kill_pgrp(pgrp, SIGTTOU, 1);
+	rcu_read_unlock();
 	set_thread_flag(TIF_SIGPENDING);
 	ret = -ERESTARTSYS;
 out:
 	return ret;
-out_unlock:
+out_irqunlock:
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+out_rcuunlock:
+	rcu_read_unlock();
 	return ret;
 }
 
-- 
Patrick Donnelly

[PATCH 2/2] Check tcsetpgrp p is a process group.

[Patrick Donnelly]

This fixes a bug where a process can set the foreground process group to its
pid even if its pid is not a valid pgrp.

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/tty_io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 401d05e..c20a2fb 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2560,9 +2560,11 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
 {
 	struct pid *pgrp;
 	pid_t pgrp_nr;
-	int retval = tty_check_change(real_tty);
+	int retval;
 	unsigned long flags;
 
+	retval = tty_check_change(real_tty);
+
 	if (retval == -EIO)
 		return -ENOTTY;
 	if (retval)
@@ -2580,6 +2582,10 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
 	retval = -ESRCH;
 	if (!pgrp)
 		goto out_unlock;
+	retval = -EINVAL;
+	if (!pid_task(pgrp, PIDTYPE_PGID)) {
+		goto out_unlock;
+	}
 	retval = -EPERM;
 	if (session_of_pgrp(pgrp) != task_session(current))
 		goto out_unlock;
-- 
Patrick Donnelly

Re: [PATCH 2/2] Check tcsetpgrp p is a process group.

[Greg Kroah-Hartman]

On Sat, Jun 27, 2015 at 05:17:03PM -0400, Patrick Donnelly wrote:
> This fixes a bug where a process can set the foreground process group to its
> pid even if its pid is not a valid pgrp.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/tty_io.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 401d05e..c20a2fb 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2560,9 +2560,11 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>  {
>  	struct pid *pgrp;
>  	pid_t pgrp_nr;
> -	int retval = tty_check_change(real_tty);
> +	int retval;
>  	unsigned long flags;
>  
> +	retval = tty_check_change(real_tty);

Why this churn?

Re: [PATCH 2/2] Check tcsetpgrp p is a process group.

[Patrick Donnelly]

On Sat, Jun 27, 2015 at 7:26 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Jun 27, 2015 at 05:17:03PM -0400, Patrick Donnelly wrote:
>> This fixes a bug where a process can set the foreground process group to its
>> pid even if its pid is not a valid pgrp.
>>
>> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
>> ---
>>  drivers/tty/tty_io.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 401d05e..c20a2fb 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -2560,9 +2560,11 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>>  {
>>       struct pid *pgrp;
>>       pid_t pgrp_nr;
>> -     int retval = tty_check_change(real_tty);
>> +     int retval;
>>       unsigned long flags;
>>
>> +     retval = tty_check_change(real_tty);
>
> Why this churn?

I removed it in the new version, sorry. I thought it made the code
more consistent.

-- 
Patrick Donnelly

Re: [PATCH 2/2] Check tcsetpgrp p is a process group.

[Greg Kroah-Hartman]

On Sat, Jun 27, 2015 at 05:17:03PM -0400, Patrick Donnelly wrote:
> This fixes a bug where a process can set the foreground process group to its
> pid even if its pid is not a valid pgrp.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/tty_io.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 401d05e..c20a2fb 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2560,9 +2560,11 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>  {
>  	struct pid *pgrp;
>  	pid_t pgrp_nr;
> -	int retval = tty_check_change(real_tty);
> +	int retval;
>  	unsigned long flags;
>  
> +	retval = tty_check_change(real_tty);
> +
>  	if (retval == -EIO)
>  		return -ENOTTY;
>  	if (retval)
> @@ -2580,6 +2582,10 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>  	retval = -ESRCH;
>  	if (!pgrp)
>  		goto out_unlock;
> +	retval = -EINVAL;
> +	if (!pid_task(pgrp, PIDTYPE_PGID)) {
> +		goto out_unlock;
> +	}
>  	retval = -EPERM;
>  	if (session_of_pgrp(pgrp) != task_session(current))
>  		goto out_unlock;

Always run your patches though checkpatch.pl so you don't get emails
telling you to fix the things that checkpatch.pl tells you to...

Re: [PATCH 1/2] Add missing rcu_read_lock for task_pgrp.

[Greg Kroah-Hartman]

On Sat, Jun 27, 2015 at 05:17:02PM -0400, Patrick Donnelly wrote:
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>

No changelog text?  Sorry, it's required.

[PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
duration of use.

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/tty_io.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..fbb55db 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -388,33 +388,39 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
 int tty_check_change(struct tty_struct *tty)
 {
 	unsigned long flags;
+	struct pid *pgrp;
 	int ret = 0;
 
 	if (current->signal->tty != tty)
 		return 0;
 
-	spin_lock_irqsave(&tty->ctrl_lock, flags);
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
 
+	spin_lock_irqsave(&tty->ctrl_lock, flags);
 	if (!tty->pgrp) {
 		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
-		goto out_unlock;
+		goto out_irqunlock;
 	}
-	if (task_pgrp(current) == tty->pgrp)
-		goto out_unlock;
+	if (pgrp == tty->pgrp)
+		goto out_irqunlock;
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
 	if (is_ignored(SIGTTOU))
-		goto out;
+		goto out_rcuunlock;
 	if (is_current_pgrp_orphaned()) {
 		ret = -EIO;
-		goto out;
+		goto out_rcuunlock;
 	}
-	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	kill_pgrp(pgrp, SIGTTOU, 1);
+	rcu_read_unlock();
 	set_thread_flag(TIF_SIGPENDING);
 	ret = -ERESTARTSYS;
-out:
 	return ret;
-out_unlock:
+out_irqunlock:
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+out_rcuunlock:
+	rcu_read_unlock();
 	return ret;
 }
 
-- 
Patrick Donnelly

[PATCH v2 2/2] tty: check tcsetpgrp p is a process group

[Patrick Donnelly]

This fixes a bug where a process can set the foreground process group to its
pid even if its pid is not a valid pgrp.

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/tty_io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index fbb55db..01b4769 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2579,6 +2579,9 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
 	retval = -ESRCH;
 	if (!pgrp)
 		goto out_unlock;
+	retval = -EINVAL;
+	if (!pid_task(pgrp, PIDTYPE_PGID))
+		goto out_unlock;
 	retval = -EPERM;
 	if (session_of_pgrp(pgrp) != task_session(current))
 		goto out_unlock;
-- 
Patrick Donnelly

Re: [PATCH v2 2/2] tty: check tcsetpgrp p is a process group

[Peter Hurley]

On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
> This fixes a bug where a process can set the foreground process group to its
> pid even if its pid is not a valid pgrp.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/tty_io.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index fbb55db..01b4769 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2579,6 +2579,9 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>  	retval = -ESRCH;
>  	if (!pgrp)
>  		goto out_unlock;
> +	retval = -EINVAL;
> +	if (!pid_task(pgrp, PIDTYPE_PGID))
> +		goto out_unlock;

This change implies that the sequence in session_of_pgrp() that specifically
checks for pid_task(pgrp, PIDTYPE_PGID) == NULL is not doing anything
useful. However, that hypothesis is directly contradicted by the
comment above session_of_pgrp()

"* This checks not only the pgrp, but falls back on the pid if no
 * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
 * without this..."

Regards,
Peter Hurley

>  	retval = -EPERM;
>  	if (session_of_pgrp(pgrp) != task_session(current))
>  		goto out_unlock;
> 

Re: [PATCH v2 2/2] tty: check tcsetpgrp p is a process group

[Patrick Donnelly]

On Sun, Jun 28, 2015 at 12:07 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
>> This fixes a bug where a process can set the foreground process group to its
>> pid even if its pid is not a valid pgrp.
>>
>> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
>> ---
>>  drivers/tty/tty_io.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index fbb55db..01b4769 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -2579,6 +2579,9 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
>>       retval = -ESRCH;
>>       if (!pgrp)
>>               goto out_unlock;
>> +     retval = -EINVAL;
>> +     if (!pid_task(pgrp, PIDTYPE_PGID))
>> +             goto out_unlock;
>
> This change implies that the sequence in session_of_pgrp() that specifically
> checks for pid_task(pgrp, PIDTYPE_PGID) == NULL is not doing anything
> useful. However, that hypothesis is directly contradicted by the
> comment above session_of_pgrp()
>
> "* This checks not only the pgrp, but falls back on the pid if no
>  * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
>  * without this..."
>
> Regards,
> Peter Hurley
>
>>       retval = -EPERM;
>>       if (session_of_pgrp(pgrp) != task_session(current))
>>               goto out_unlock;
>>

Ah, missed that. Good catch! I guess this patch is no good since it
was already accounted for and it breaks gdb.

-- 
Patrick Donnelly

Re: [PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.

kill_pgrp() obtains tasklist_lock, so I don't see an unsafe deref.

> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/tty_io.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..fbb55db 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -388,33 +388,39 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
>  int tty_check_change(struct tty_struct *tty)
>  {
>  	unsigned long flags;
> +	struct pid *pgrp;
>  	int ret = 0;
>  
>  	if (current->signal->tty != tty)
>  		return 0;
>  
> -	spin_lock_irqsave(&tty->ctrl_lock, flags);
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
>  
> +	spin_lock_irqsave(&tty->ctrl_lock, flags);
>  	if (!tty->pgrp) {
>  		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
> -		goto out_unlock;
> +		goto out_irqunlock;
>  	}
> -	if (task_pgrp(current) == tty->pgrp)
> -		goto out_unlock;
> +	if (pgrp == tty->pgrp)
> +		goto out_irqunlock;
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +
>  	if (is_ignored(SIGTTOU))
> -		goto out;
> +		goto out_rcuunlock;
>  	if (is_current_pgrp_orphaned()) {
>  		ret = -EIO;
> -		goto out;
> +		goto out_rcuunlock;
>  	}
> -	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> +	kill_pgrp(pgrp, SIGTTOU, 1);
> +	rcu_read_unlock();
>  	set_thread_flag(TIF_SIGPENDING);
>  	ret = -ERESTARTSYS;
> -out:
>  	return ret;
> -out_unlock:
> +out_irqunlock:
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +out_rcuunlock:
> +	rcu_read_unlock();
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

On Sun, Jun 28, 2015 at 11:23 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
>> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
>> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
>> duration of use.
>
> kill_pgrp() obtains tasklist_lock, so I don't see an unsafe deref.

I see a race between looking up the pgrp via task_pgrp and passing it
to kill_pgrp. The pgrp struct pid may be freed via setpgid/setsid, as
mentioned in the comment for task_pgrp:

 * Without tasklist or rcu lock it is not safe to dereference
 * the result of task_pgrp/task_session even if task == current,
 * we can race with another thread doing sys_setsid/sys_setpgid.

Getting the lock after the lookup is getting the lock too late. I
could be wrong though as I'm no expert on locking in Linux.

-- 
Patrick Donnelly

Re: [PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

On 06/28/2015 01:20 PM, Patrick Donnelly wrote:
> On Sun, Jun 28, 2015 at 11:23 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
>>> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
>>> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
>>> duration of use.
>>
>> kill_pgrp() obtains tasklist_lock, so I don't see an unsafe deref.
> 
> I see a race between looking up the pgrp via task_pgrp and passing it
> to kill_pgrp. The pgrp struct pid may be freed via setpgid/setsid, as
> mentioned in the comment for task_pgrp:
> 
>  * Without tasklist or rcu lock it is not safe to dereference
>  * the result of task_pgrp/task_session even if task == current,
>  * we can race with another thread doing sys_setsid/sys_setpgid.
> 
> Getting the lock after the lookup is getting the lock too late. I
> could be wrong though as I'm no expert on locking in Linux.

I suppose it can't hurt; please add similar logic to job_control() in
drivers/tty/n_tty.c which handles the corresponding SIGTTIN signal conditions.

Regards,
Peter Hurley

Re: [PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

Hi Patrick,

On 06/27/2015 08:51 PM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/tty_io.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..fbb55db 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -388,33 +388,39 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
>  int tty_check_change(struct tty_struct *tty)
>  {
>  	unsigned long flags;
> +	struct pid *pgrp;
>  	int ret = 0;
>  
>  	if (current->signal->tty != tty)
>  		return 0;
>  
> -	spin_lock_irqsave(&tty->ctrl_lock, flags);
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
>  
> +	spin_lock_irqsave(&tty->ctrl_lock, flags);
>  	if (!tty->pgrp) {
>  		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
> -		goto out_unlock;
> +		goto out_irqunlock;

The label name changing is not really necessary and would reduce diff count.
It would be nice to get the printk() out from the locks as well (in a follow-on
patch?)

Regards,
Peter Hurley

>  	}
> -	if (task_pgrp(current) == tty->pgrp)
> -		goto out_unlock;
> +	if (pgrp == tty->pgrp)
> +		goto out_irqunlock;
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +
>  	if (is_ignored(SIGTTOU))
> -		goto out;
> +		goto out_rcuunlock;
>  	if (is_current_pgrp_orphaned()) {
>  		ret = -EIO;
> -		goto out;
> +		goto out_rcuunlock;
>  	}
> -	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> +	kill_pgrp(pgrp, SIGTTOU, 1);
> +	rcu_read_unlock();
>  	set_thread_flag(TIF_SIGPENDING);
>  	ret = -ERESTARTSYS;
> -out:
>  	return ret;
> -out_unlock:
> +out_irqunlock:
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +out_rcuunlock:
> +	rcu_read_unlock();
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 1/2] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

Hi Peter,

On Sun, Jun 28, 2015 at 3:27 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> The label name changing is not really necessary and would reduce diff count.

I've removed the label changes in the next series. Thanks for the feedback.

> It would be nice to get the printk() out from the locks as well (in a follow-on
> patch?)

I don't follow what you're referring to. Is it these lines?

    if (!tty->pgrp) {
        printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");

-- 
Patrick Donnelly

[PATCH v3] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
duration of use.

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/n_tty.c  | 12 ++++++++++--
 drivers/tty/tty_io.c | 17 ++++++++++++-----
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index c9c27f6..0d631f8 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2137,6 +2137,8 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
 
 static int job_control(struct tty_struct *tty, struct file *file)
 {
+	struct pid *pgrp;
+
 	/* Job control check -- must be done at start and after
 	   every sleep (POSIX.1 7.1.1.4). */
 	/* NOTE: not yet done after every sleep pending a thorough
@@ -2146,18 +2148,24 @@ static int job_control(struct tty_struct *tty, struct file *file)
 	    current->signal->tty != tty)
 		return 0;
 
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
+
 	spin_lock_irq(&tty->ctrl_lock);
+
 	if (!tty->pgrp)
 		printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
-	else if (task_pgrp(current) != tty->pgrp) {
+	else if (pgrp != tty->pgrp) {
 		spin_unlock_irq(&tty->ctrl_lock);
 		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
 			return -EIO;
-		kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+		kill_pgrp(pgrp, SIGTTIN, 1);
+		rcu_read_unlock();
 		set_thread_flag(TIF_SIGPENDING);
 		return -ERESTARTSYS;
 	}
 	spin_unlock_irq(&tty->ctrl_lock);
+	rcu_read_unlock();
 	return 0;
 }
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..6bdfb98 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -388,33 +388,40 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
 int tty_check_change(struct tty_struct *tty)
 {
 	unsigned long flags;
+	struct pid *pgrp;
 	int ret = 0;
 
 	if (current->signal->tty != tty)
 		return 0;
 
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
+
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
 
 	if (!tty->pgrp) {
 		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
 		goto out_unlock;
 	}
-	if (task_pgrp(current) == tty->pgrp)
+	if (pgrp == tty->pgrp)
 		goto out_unlock;
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
 	if (is_ignored(SIGTTOU))
-		goto out;
+		goto out_rcuunlock;
 	if (is_current_pgrp_orphaned()) {
 		ret = -EIO;
-		goto out;
+		goto out_rcuunlock;
 	}
-	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	kill_pgrp(pgrp, SIGTTOU, 1);
+	rcu_read_unlock();
 	set_thread_flag(TIF_SIGPENDING);
 	ret = -ERESTARTSYS;
-out:
 	return ret;
 out_unlock:
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+out_rcuunlock:
+	rcu_read_unlock();
 	return ret;
 }
 
-- 
Patrick Donnelly

Re: [PATCH v3] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

On 06/29/2015 07:59 PM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH v3] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

On 06/29/2015 07:59 PM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/n_tty.c  | 12 ++++++++++--
>  drivers/tty/tty_io.c | 17 ++++++++++++-----
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index c9c27f6..0d631f8 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2137,6 +2137,8 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
>  
>  static int job_control(struct tty_struct *tty, struct file *file)
>  {
> +	struct pid *pgrp;
> +
>  	/* Job control check -- must be done at start and after
>  	   every sleep (POSIX.1 7.1.1.4). */
>  	/* NOTE: not yet done after every sleep pending a thorough
> @@ -2146,18 +2148,24 @@ static int job_control(struct tty_struct *tty, struct file *file)
>  	    current->signal->tty != tty)
>  		return 0;
>  
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
> +
>  	spin_lock_irq(&tty->ctrl_lock);
> +
>  	if (!tty->pgrp)
>  		printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
> -	else if (task_pgrp(current) != tty->pgrp) {
> +	else if (pgrp != tty->pgrp) {
>  		spin_unlock_irq(&tty->ctrl_lock);
>  		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
>  			return -EIO;

I just realized there's a missing rcu_read_unlock() from this early return.

Regards,
Peter Hurley


> -		kill_pgrp(task_pgrp(current), SIGTTIN, 1);
> +		kill_pgrp(pgrp, SIGTTIN, 1);
> +		rcu_read_unlock();
>  		set_thread_flag(TIF_SIGPENDING);
>  		return -ERESTARTSYS;
>  	}
>  	spin_unlock_irq(&tty->ctrl_lock);
> +	rcu_read_unlock();
>  	return 0;
>  }
>  
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..6bdfb98 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -388,33 +388,40 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
>  int tty_check_change(struct tty_struct *tty)
>  {
>  	unsigned long flags;
> +	struct pid *pgrp;
>  	int ret = 0;
>  
>  	if (current->signal->tty != tty)
>  		return 0;
>  
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
> +
>  	spin_lock_irqsave(&tty->ctrl_lock, flags);
>  
>  	if (!tty->pgrp) {
>  		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
>  		goto out_unlock;
>  	}
> -	if (task_pgrp(current) == tty->pgrp)
> +	if (pgrp == tty->pgrp)
>  		goto out_unlock;
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +
>  	if (is_ignored(SIGTTOU))
> -		goto out;
> +		goto out_rcuunlock;
>  	if (is_current_pgrp_orphaned()) {
>  		ret = -EIO;
> -		goto out;
> +		goto out_rcuunlock;
>  	}
> -	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> +	kill_pgrp(pgrp, SIGTTOU, 1);
> +	rcu_read_unlock();
>  	set_thread_flag(TIF_SIGPENDING);
>  	ret = -ERESTARTSYS;
> -out:
>  	return ret;
>  out_unlock:
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +out_rcuunlock:
> +	rcu_read_unlock();
>  	return ret;
>  }
>  
> 

Re: [PATCH v3] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

On Sat, Jul 11, 2015 at 10:05 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> I just realized there's a missing rcu_read_unlock() from this early return.

Nice catch. I'll send a new series out...

-- 
Patrick Donnelly

[PATCH v4] tty: add missing rcu_read_lock for task_pgrp

[Patrick Donnelly]

task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
duration of use.

Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
---
 drivers/tty/n_tty.c  | 15 ++++++++++++---
 drivers/tty/tty_io.c | 17 ++++++++++++-----
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index c9c27f6..de67b2c 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2137,6 +2137,8 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
 
 static int job_control(struct tty_struct *tty, struct file *file)
 {
+	struct pid *pgrp;
+
 	/* Job control check -- must be done at start and after
 	   every sleep (POSIX.1 7.1.1.4). */
 	/* NOTE: not yet done after every sleep pending a thorough
@@ -2146,18 +2148,25 @@ static int job_control(struct tty_struct *tty, struct file *file)
 	    current->signal->tty != tty)
 		return 0;
 
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
+
 	spin_lock_irq(&tty->ctrl_lock);
 	if (!tty->pgrp)
 		printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
-	else if (task_pgrp(current) != tty->pgrp) {
+	else if (pgrp != tty->pgrp) {
 		spin_unlock_irq(&tty->ctrl_lock);
-		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
+		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned()) {
+			rcu_read_unlock();
 			return -EIO;
-		kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+		}
+		kill_pgrp(pgrp, SIGTTIN, 1);
+		rcu_read_unlock();
 		set_thread_flag(TIF_SIGPENDING);
 		return -ERESTARTSYS;
 	}
 	spin_unlock_irq(&tty->ctrl_lock);
+	rcu_read_unlock();
 	return 0;
 }
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 57fc6ee..6bdfb98 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -388,33 +388,40 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
 int tty_check_change(struct tty_struct *tty)
 {
 	unsigned long flags;
+	struct pid *pgrp;
 	int ret = 0;
 
 	if (current->signal->tty != tty)
 		return 0;
 
+	rcu_read_lock();
+	pgrp = task_pgrp(current);
+
 	spin_lock_irqsave(&tty->ctrl_lock, flags);
 
 	if (!tty->pgrp) {
 		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
 		goto out_unlock;
 	}
-	if (task_pgrp(current) == tty->pgrp)
+	if (pgrp == tty->pgrp)
 		goto out_unlock;
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
 	if (is_ignored(SIGTTOU))
-		goto out;
+		goto out_rcuunlock;
 	if (is_current_pgrp_orphaned()) {
 		ret = -EIO;
-		goto out;
+		goto out_rcuunlock;
 	}
-	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	kill_pgrp(pgrp, SIGTTOU, 1);
+	rcu_read_unlock();
 	set_thread_flag(TIF_SIGPENDING);
 	ret = -ERESTARTSYS;
-out:
 	return ret;
 out_unlock:
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+out_rcuunlock:
+	rcu_read_unlock();
 	return ret;
 }
 
-- 
Patrick Donnelly

Re: [PATCH v4] tty: add missing rcu_read_lock for task_pgrp

[Peter Hurley]

On 07/12/2015 06:51 PM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH v4] tty: add missing rcu_read_lock for task_pgrp

[Jiri Slaby]

CC Oleg

On 07/13/2015, 12:51 AM, Patrick Donnelly wrote:
> task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> duration of use.
> 
> Signed-off-by: Patrick Donnelly <batrick@batbytes.com>
> ---
>  drivers/tty/n_tty.c  | 15 ++++++++++++---
>  drivers/tty/tty_io.c | 17 ++++++++++++-----
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index c9c27f6..de67b2c 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2137,6 +2137,8 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
>  
>  static int job_control(struct tty_struct *tty, struct file *file)
>  {
> +	struct pid *pgrp;
> +
>  	/* Job control check -- must be done at start and after
>  	   every sleep (POSIX.1 7.1.1.4). */
>  	/* NOTE: not yet done after every sleep pending a thorough
> @@ -2146,18 +2148,25 @@ static int job_control(struct tty_struct *tty, struct file *file)
>  	    current->signal->tty != tty)
>  		return 0;
>  
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
> +
>  	spin_lock_irq(&tty->ctrl_lock);
>  	if (!tty->pgrp)
>  		printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
> -	else if (task_pgrp(current) != tty->pgrp) {
> +	else if (pgrp != tty->pgrp) {
>  		spin_unlock_irq(&tty->ctrl_lock);
> -		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
> +		if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned()) {
> +			rcu_read_unlock();
>  			return -EIO;
> -		kill_pgrp(task_pgrp(current), SIGTTIN, 1);
> +		}
> +		kill_pgrp(pgrp, SIGTTIN, 1);
> +		rcu_read_unlock();
>  		set_thread_flag(TIF_SIGPENDING);
>  		return -ERESTARTSYS;
>  	}
>  	spin_unlock_irq(&tty->ctrl_lock);
> +	rcu_read_unlock();
>  	return 0;
>  }
>  
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 57fc6ee..6bdfb98 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -388,33 +388,40 @@ EXPORT_SYMBOL_GPL(tty_find_polling_driver);
>  int tty_check_change(struct tty_struct *tty)
>  {
>  	unsigned long flags;
> +	struct pid *pgrp;
>  	int ret = 0;
>  
>  	if (current->signal->tty != tty)
>  		return 0;
>  
> +	rcu_read_lock();
> +	pgrp = task_pgrp(current);
> +
>  	spin_lock_irqsave(&tty->ctrl_lock, flags);
>  
>  	if (!tty->pgrp) {
>  		printk(KERN_WARNING "tty_check_change: tty->pgrp == NULL!\n");
>  		goto out_unlock;
>  	}
> -	if (task_pgrp(current) == tty->pgrp)
> +	if (pgrp == tty->pgrp)
>  		goto out_unlock;
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +
>  	if (is_ignored(SIGTTOU))
> -		goto out;
> +		goto out_rcuunlock;
>  	if (is_current_pgrp_orphaned()) {
>  		ret = -EIO;
> -		goto out;
> +		goto out_rcuunlock;
>  	}
> -	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> +	kill_pgrp(pgrp, SIGTTOU, 1);
> +	rcu_read_unlock();
>  	set_thread_flag(TIF_SIGPENDING);
>  	ret = -ERESTARTSYS;
> -out:
>  	return ret;
>  out_unlock:
>  	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
> +out_rcuunlock:
> +	rcu_read_unlock();
>  	return ret;
>  }
>  
> 

thanks,
-- 
js
suse labs

Re: [PATCH v4] tty: add missing rcu_read_lock for task_pgrp

[Oleg Nesterov]

On 07/20, Jiri Slaby wrote:
>
> CC Oleg
>
> On 07/13/2015, 12:51 AM, Patrick Donnelly wrote:
> > task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
> > is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
> > duration of use.

Look like, this change is right.

But perhaps we should a simple helper for drivers/tty/ which takes rcu
lock, sends a signal to task_pgrp(current), and sets TIF_SIGPENDING?

I won't insist of course, I am fine either way.

Oleg.