Signed-off-by: Patrick Donnelly <[email protected]>
---
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
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 <[email protected]>
---
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
On Sat, Jun 27, 2015 at 05:17:02PM -0400, Patrick Donnelly wrote:
> Signed-off-by: Patrick Donnelly <[email protected]>
No changelog text? Sorry, it's required.
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 <[email protected]>
> ---
> 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?
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 <[email protected]>
> ---
> 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...
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Sat, Jun 27, 2015 at 7:26 PM, Greg Kroah-Hartman
<[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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;
> }
>
>
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 <[email protected]>
> ---
> 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;
>
On Sun, Jun 28, 2015 at 11:23 AM, Peter Hurley <[email protected]> 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
On 06/28/2015 01:20 PM, Patrick Donnelly wrote:
> On Sun, Jun 28, 2015 at 11:23 AM, Peter Hurley <[email protected]> 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
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 <[email protected]>
> ---
> 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;
> }
>
>
On Sun, Jun 28, 2015 at 12:07 PM, Peter Hurley <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hi Peter,
On Sun, Jun 28, 2015 at 3:27 PM, Peter Hurley <[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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;
> }
>
>
On Sat, Jul 11, 2015 at 10:05 PM, Peter Hurley <[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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
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.