Add missing rcu_read_lock for task_pgrp.

PATCH Superseded Patrick Donnelly 5 participants 23 messages Jun 27, 2015 – Jul 20, 2015 TTY layer and serial drivers

lore

Share & tools

lore

Patrick Donnelly

author

2015-06-27 21:17 UTC #0 lore raw

PATCH 1/2 Add missing rcu_read_lock for task_pgrp.

Signed-off-by Patrick Donnelly <[email protected]>

---
 drivers/tty/tty_io.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

drivers/tty/tty_io.c +13 -6

index 57fc6ee..401d05e 100644
@@ -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;
 }

Signature · 1 line

Patrick Donnelly

Patrick Donnelly

author

2015-06-27 21:17 UTC #1 lore raw

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

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(-)

drivers/tty/tty_io.c +7 -1

index 401d05e..c20a2fb 100644
@@ -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;

Signature · 1 line

Patrick Donnelly

Patrick Donnelly

author

2015-06-28 00:53 UTC #3 lore raw

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

Quoted 21 lines · Greg Kroah-Hartman

Quoted 20 lines · 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(-)
drivers/tty/tty_io.c +1 -1
index 401d05e..c20a2fb 100644
@@ -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;
 

Quoted 3 lines

drivers/tty/tty_io.c static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t · line 2566
Quoted 1 line
drivers/tty/tty_io.c +1 -0
+     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.

Signature · 1 line

Patrick Donnelly

Greg Kroah-Hartman

maintainer

2015-06-27 23:27 UTC #4 lore raw

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

In reply to Patrick Donnelly

Quoted 26 lines · 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(-)

drivers/tty/tty_io.c +3 -1

index 401d05e..c20a2fb 100644
@@ -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)

drivers/tty/tty_io.c static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t · line 2591

Quoted 10 lines

drivers/tty/tty_io.c +4 -0

@@ -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...

Greg Kroah-Hartman

maintainer

2015-06-27 23:26 UTC #5 lore raw

In reply to Patrick Donnelly

Quoted 1 line · Patrick Donnelly

Signed-off-by Patrick Donnelly <[email protected]>

No changelog text? Sorry, it's required.

Patrick Donnelly

author

2015-06-28 00:51 UTC #6 lore raw

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

In reply to 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/tty_io.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

drivers/tty/tty_io.c +15 -9

index 57fc6ee..fbb55db 100644
@@ -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;
 }

Signature · 1 line

Patrick Donnelly

Patrick Donnelly

author

2015-06-28 00:51 UTC #7 lore raw

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

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(+)

drivers/tty/tty_io.c +3 -0

index fbb55db..01b4769 100644
@@ -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;

Signature · 1 line

Patrick Donnelly

Peter Hurley

2015-06-28 16:07 UTC #8 lore raw

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

Quoted 13 lines · 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(+)
drivers/tty/tty_io.c +0 -0
index fbb55db..01b4769 100644

drivers/tty/tty_io.c static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t · line 2584

Quoted 6 lines

drivers/tty/tty_io.c +3 -0

@@ -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

Quoted 4 lines

drivers/tty/tty_io.c +0 -0

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

Patrick Donnelly

author

2015-06-29 01:27 UTC #9 lore raw

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

Quoted 26 lines · Peter Hurley

Quoted 13 lines · 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(+)
drivers/tty/tty_io.c +0 -0
index fbb55db..01b4769 100644
drivers/tty/tty_io.c static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t · line 2584
Quoted 6 lines
drivers/tty/tty_io.c +3 -0
@@ -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()

Quoted 11 lines

"* 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
Quoted 4 lines
drivers/tty/tty_io.c +0 -0
      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.

Signature · 1 line

Patrick Donnelly

Peter Hurley

2015-06-28 15:23 UTC #10 lore raw

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

In reply to Patrick Donnelly

Quoted 3 lines · 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.

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

Quoted 59 lines

Signed-off-by Patrick Donnelly <[email protected]>

---
 drivers/tty/tty_io.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

drivers/tty/tty_io.c +15 -9

index 57fc6ee..fbb55db 100644
@@ -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

author

2015-06-28 17:20 UTC #11 lore raw

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

Quoted 6 lines · Peter Hurley

Quoted 3 lines · 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.
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.

Signature · 1 line

Patrick Donnelly

Peter Hurley

2015-06-28 19:21 UTC #12 lore raw

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

Quoted 12 lines · Patrick Donnelly

Quoted 6 lines · Peter Hurley
Quoted 3 lines · 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.
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:

Quoted 6 lines

* 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

Peter Hurley

2015-06-28 19:27 UTC #13 lore raw

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

In reply to Patrick Donnelly

Hi Patrick,

Quoted 22 lines · 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/tty_io.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)
drivers/tty/tty_io.c +1 -0
index 57fc6ee..fbb55db 100644
@@ -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;

drivers/tty/tty_io.c EXPORT_SYMBOL_GPL(tty_find_polling_driver); · line 403

Quoted 10 lines

drivers/tty/tty_io.c +4 -2

 
-	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

Quoted 31 lines

drivers/tty/tty_io.c +10 -7

 	}
-	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

author

2015-06-29 23:38 UTC #14 lore raw

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

Hi Peter,

Quoted 1 line · Peter Hurley

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.

Quoted 2 lines

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");

Signature · 1 line

Patrick Donnelly

Patrick Donnelly

author

2015-06-30 00:00 UTC #15 lore raw

PATCH v3 1/1 tty: add missing rcu_read_lock for task_pgrp

In reply to 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(-)

drivers/tty/n_tty.c +10 -2

index c9c27f6..0d631f8 100644
@@ -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;
 }

drivers/tty/tty_io.c +12 -5

index 57fc6ee..6bdfb98 100644
@@ -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;
 }

Signature · 1 line

Patrick Donnelly

Peter Hurley

Reviewed-by on v3 1/1

2015-07-09 02:07 UTC #16 lore raw

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

Quoted 3 lines · 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.

Reviewed-by Peter Hurley <[email protected]>

Peter Hurley

2015-07-12 02:05 UTC #17 lore raw

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

In reply to Patrick Donnelly

Quoted 29 lines · 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(-)

drivers/tty/n_tty.c +4 -0

index c9c27f6..0d631f8 100644
@@ -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);

drivers/tty/n_tty.c static int job_control(struct tty_struct *tty, struct file *file) · line 2161

Quoted 10 lines

drivers/tty/n_tty.c +3 -1

+
 	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

Quoted 62 lines

drivers/tty/n_tty.c +3 -1

-		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;
 }

drivers/tty/tty_io.c +12 -5

index 57fc6ee..6bdfb98 100644
@@ -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

author

2015-07-12 22:42 UTC #18 lore raw

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

Quoted 1 line · Peter Hurley

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

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

Signature · 1 line

Patrick Donnelly

Patrick Donnelly

author

2015-07-12 22:53 UTC #19 lore raw

PATCH v4 1/1 tty: add missing rcu_read_lock for task_pgrp

In reply to 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(-)

drivers/tty/n_tty.c +12 -3

index c9c27f6..de67b2c 100644
@@ -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;
 }

drivers/tty/tty_io.c +12 -5

index 57fc6ee..6bdfb98 100644
@@ -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;
 }

Signature · 1 line

Patrick Donnelly

Peter Hurley

Reviewed-by on v4 1/1

2015-07-13 00:36 UTC #20 lore raw

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

Quoted 3 lines · 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.

Reviewed-by Peter Hurley <[email protected]>

Jiri Slaby

2015-07-20 09:26 UTC #21 lore raw

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

In reply to Patrick Donnelly

CC Oleg

Quoted 93 lines · 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(-)

drivers/tty/n_tty.c +12 -3

index c9c27f6..de67b2c 100644
@@ -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;
 }

drivers/tty/tty_io.c +10 -4

index 57fc6ee..6bdfb98 100644
@@ -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);

drivers/tty/tty_io.c EXPORT_SYMBOL_GPL(tty_find_polling_driver); · line 427

Quoted 10 lines

drivers/tty/tty_io.c +2 -1

 	ret = -ERESTARTSYS;
-out:
 	return ret;
 out_unlock:
 	spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+out_rcuunlock:
+	rcu_read_unlock();
 	return ret;
 }

thanks,

Signature · 2 lines

js
suse labs

Oleg Nesterov

2015-07-20 19:42 UTC #22 lore raw

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

Quoted 7 lines · Jiri Slaby

CC Oleg
Quoted 3 lines · 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.

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.