* [PATCH 0/3] tty: Fix tiocspgrp() related races
[not found] <20230831143645.2298799-1-earl_chew@yahoo.com>
@ 2023-09-01 1:50 ` Earl Chew
2023-09-28 13:06 ` [PATCH v2 " Earl Chew
` (3 more replies)
2023-09-01 1:50 ` [PATCH 1/3] tty: Fix __tty_check_change() and tiocspgrp() race Earl Chew
` (2 subsequent siblings)
3 siblings, 4 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-01 1:50 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
[ Reposting to linux-kernel@vger.kernel.org ]
This set of patches improves tiocspgrp() use cases, and was
motivated by noticing rare job control SIGTTIN/SIGTTOU races.
Over time, users probably have encountered these races during
interactive use, but quite possibly have simply worked through
them.
The first patch addresses a race that occurs when a job control
shell runs a fg command to move a background job to the foreground.
The shell issues tcsetpgrp() followed by killpg(SIGCONT). In
rare cases this coincides with the new foreground job receiving
a SIGTTIN which gives the appearance that fg stops the job.
The first reproducer below can detect this case:
while ./first ; do : ; done
WIFSTOPPED 21
The second patch addresses a race that occurs when a job
control application issues several tcsetpgrp() calls in
parallel. In rare cases, the callers will only partially serialise.
After the first successfully sets the foreground process group,
the remaining should receive SIGTTOU because they are now
background processes that are attempting to change the
foreground process group. Instead, they successfully reconfigure
the foreground process group of the controlling terminal.
The second reproducer below can detect this case:
while ./second ; do : ; done
Died at line 86: Exited 2 Stopped 0
The third patch relocates the sampling of task_pgrp() in
__tty_check_change_locked() to place it inside the locked
region, immediately before the value is used. This improves
the consistency of the sampled value, and follows the example
set by __proc_set_tty().
/* FIRST RACE */
#include <errno.h>
#include <fcntl.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <time.h>
#include <unistd.h>
#include <sys/wait.h>
#define DIE(...) die(__LINE__, __VA_ARGS__)
void die(unsigned lineno, const char *fmt, ...)
{
va_list argp;
va_start(argp, fmt);
vfprintf(stderr, fmt, argp);
fputc('\n', stderr);
exit(1);
}
void delay(unsigned range, unsigned scale)
{
unsigned rnd = random() % range;
struct timespec delay = {
.tv_nsec = rnd * scale
};
nanosleep(&delay, 0);
}
int main()
{
const char *ttyname = ctermid(0);
if (!ttyname) DIE("ctermid");
int ttyfd = open(ttyname, O_RDONLY | O_NONBLOCK);
if (-1 == ttyfd) DIE(ttyname);
typedef void (*sighandler_t)(int);
sighandler_t sighandler = signal(SIGINT, SIG_DFL);
if (SIG_ERR == sighandler) DIE("signal");
sigset_t sigmask;
if (sigprocmask(SIG_SETMASK, 0, &sigmask)) DIE("sigprocmask");
if (sigismember(&sigmask, SIGTTIN)) DIE("SIGTTIN");
pid_t child = fork();
if (!child) {
srandom(getpid());
if (setpgid(0, 0)) DIE("setpgid");
delay(10000, 1);
char buf[1];
int rd = read(ttyfd, buf, 0);
if (rd) DIE("read");
} else {
srandom(getpid());
if (setpgid(child, child)) DIE("setpgid");
delay(10000, 1);
if (tcsetpgrp(ttyfd, child)) DIE("tcsetpgrp");
if (killpg(child, SIGCONT)) DIE("killpg");
int status;
pid_t waited = waitpid(child, &status, WUNTRACED);
if (child != waited) DIE("waitpid");
kill(child, SIGKILL);
if (WIFSTOPPED(status)) DIE("WIFSTOPPED %d", WSTOPSIG(status));
if (!WIFEXITED(status)) DIE("WEXITED");
if (WEXITSTATUS(status)) DIE("WEXITSTATUS");
}
return 0;
}
/* FIRST RACE */
/* SECOND RACE */
#include <errno.h>
#include <fcntl.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <signal.h>
#include <unistd.h>
#include <sys/wait.h>
#define DIE(...) die(__LINE__, __VA_ARGS__)
void die(unsigned lineno, const char *fmt, ...)
{
va_list argp;
fprintf(stderr, "Died at line %u", lineno);
if (fmt) {
fprintf(stderr, ": ");
va_start(argp, fmt);
vfprintf(stderr, fmt, argp);
}
fputc('\n', stderr);
exit(1);
}
int main()
{
const char *ttyname = ctermid(0);
if (!ttyname) DIE(0);
int ttyfd = open(ttyname, O_RDONLY | O_NONBLOCK);
if (-1 == ttyfd) DIE(ttyname);
sigset_t sigmask;
if (sigprocmask(SIG_SETMASK, 0, &sigmask)) DIE(0);
if (sigismember(&sigmask, SIGTTOU)) DIE(0);
pid_t parentPid = getpid();
pid_t pgid[2] = { };
pid_t child[2] = { };
for (int ix = 0; ix < 2; ++ix) {
pgid[ix] = fork();
if (!pgid[ix]) {
if (setpgid(0, 0)) DIE(0);
exit(0);
}
if (setpgid(pgid[ix], pgid[ix])) DIE(0);
child[ix] = fork();
if (!child[ix]) {
if (kill(getpid(), SIGSTOP)) DIE(0);
printf("Child %d foreground %d\n", getpid(), pgid[ix]);
if (tcsetpgrp(ttyfd, pgid[ix])) DIE(0);
exit(0);
}
if (-1 == child[ix]) DIE(0);
if (child[ix] != waitpid(child[ix], 0, WUNTRACED)) DIE(0);
}
if (SIG_ERR == signal(SIGTTOU, SIG_IGN)) DIE(0);
if (kill(-parentPid, SIGCONT)) DIE(0);
int status;
int stopped = 0;
int exited = 0;
for (int ix = 0; ix < 2; ++ix) {
pid_t waited = waitpid(child[ix], &status, WUNTRACED);
if (waited != child[ix]) DIE("Child %d", child[ix]);
if (WIFEXITED(status))
++exited;
if (WIFSTOPPED(status))
++stopped;
}
for (int ix = 0; ix < 2; ++ix) {
if (kill(pgid[ix], SIGKILL) && ESRCH != errno) DIE(0);
if (kill(child[ix], SIGKILL) && ESRCH != errno) DIE(0);
}
if (2 == exited)
DIE("Exited %d Stopped %d", exited, stopped);
return 0;
}
/* SECOND RACE */
Earl Chew (3):
tty: Fix __tty_check_change() and tiocspgrp() race
tty: Serialise racing tiocspgrp() callers
tty: Move task_pgrp() after tty->ctrl.lock for consistency
drivers/tty/tty_jobctrl.c | 44 ++++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 12 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 0/3] tty: Fix tiocspgrp() related races
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
@ 2023-09-28 13:06 ` Earl Chew
2023-09-28 13:38 ` Greg KH
2023-09-28 13:06 ` [PATCH v2 1/3] tty: Fix __tty_check_change() and tiocspgrp() race Earl Chew
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Earl Chew @ 2023-09-28 13:06 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
This is a resubmission to correct errors and warnings
called out by the kernel test robot.
Earl Chew (3):
tty: Fix __tty_check_change() and tiocspgrp() race
tty: Serialise racing tiocspgrp() callers
tty: Move task_pgrp() after tty->ctrl.lock for consistency
drivers/tty/tty_jobctrl.c | 47 ++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 13 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/3] tty: Fix tiocspgrp() related races
2023-09-28 13:06 ` [PATCH v2 " Earl Chew
@ 2023-09-28 13:38 ` Greg KH
0 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2023-09-28 13:38 UTC (permalink / raw)
To: Earl Chew; +Cc: linux-kernel, jirislaby, peter
On Thu, Sep 28, 2023 at 06:06:56AM -0700, Earl Chew wrote:
> This is a resubmission to correct errors and warnings
> called out by the kernel test robot.
>
> Earl Chew (3):
> tty: Fix __tty_check_change() and tiocspgrp() race
> tty: Serialise racing tiocspgrp() callers
> tty: Move task_pgrp() after tty->ctrl.lock for consistency
>
> drivers/tty/tty_jobctrl.c | 47 ++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 13 deletions(-)
>
> --
> 2.39.1
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] tty: Fix __tty_check_change() and tiocspgrp() race
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
2023-09-28 13:06 ` [PATCH v2 " Earl Chew
@ 2023-09-28 13:06 ` Earl Chew
2023-09-28 13:06 ` [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
2023-09-28 13:06 ` [PATCH v2 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency Earl Chew
3 siblings, 0 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-28 13:06 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
Restore the use of tty->ctrl.lock to cover both checking
current->task_pgrp() and sending kill_pgrp(). This coverage
was lost for SIGTTIN as part of commit 2812d9e9fd94
(tty: Combine SIGTTOU/SIGTTIN handling). In rare cases,
job control shells using tcsetpgrp() and SIGCONT to move
a background job to the foreground could find the new
foreground job stopping almost immediately due to SIGTTIN
or SIGTTOU.
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index 0d04287da..aba721a3c 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -44,7 +44,6 @@ int __tty_check_change(struct tty_struct *tty, int sig)
spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
if (tty_pgrp && pgrp != tty_pgrp) {
if (is_ignored(sig)) {
@@ -58,6 +57,7 @@ int __tty_check_change(struct tty_struct *tty, int sig)
ret = -ERESTARTSYS;
}
}
+ spin_unlock_irqrestore(&tty->ctrl.lock, flags);
rcu_read_unlock();
if (!tty_pgrp)
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
2023-09-28 13:06 ` [PATCH v2 " Earl Chew
2023-09-28 13:06 ` [PATCH v2 1/3] tty: Fix __tty_check_change() and tiocspgrp() race Earl Chew
@ 2023-09-28 13:06 ` Earl Chew
2023-09-29 6:07 ` kernel test robot
2023-09-28 13:06 ` [PATCH v2 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency Earl Chew
3 siblings, 1 reply; 13+ messages in thread
From: Earl Chew @ 2023-09-28 13:06 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew, kernel test robot
Only lock tty->ctrl.lock once when processing requests
in tiocspgrp() to serialise concurrent changes. Since
the introduction of tty->ctrl.lock in commit 47f86834bbd4
("redo locking of tty->pgrp"), tiocspgrp() has acquired the
lock twice: first to check the process group, and next to
change the process group. In the rare case of multiple
callers, all can pass the process group check before each
taking turns to update the process group.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309012304.un7EAdgl-lkp@intel.com/
Closes: https://lore.kernel.org/r/202309011252.ItlD27Mg-lkp@intel.com/
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 43 +++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index aba721a3c..da028aadf 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -20,9 +20,10 @@ static int is_ignored(int sig)
}
/**
- * __tty_check_change - check for POSIX terminal changes
+ * __tty_check_change_locked - check for POSIX terminal changes
* @tty: tty to check
* @sig: signal to send
+ * @ctrl_lock: &ctrl.lock if already acquired
*
* If we try to write to, or set the state of, a terminal and we're
* not in the foreground, send a SIGTTOU. If the signal is blocked or
@@ -30,19 +31,24 @@ static int is_ignored(int sig)
*
* Locking: ctrl.lock
*/
-int __tty_check_change(struct tty_struct *tty, int sig)
+static int __tty_check_change_locked(struct tty_struct *tty, int sig,
+ spinlock_t *ctrl_lock)
{
unsigned long flags;
struct pid *pgrp, *tty_pgrp;
int ret = 0;
+ BUG_ON(ctrl_lock && (
+ ctrl_lock != &tty->ctrl.lock || !spin_is_locked(ctrl_lock)));
+
if (current->signal->tty != tty)
return 0;
rcu_read_lock();
pgrp = task_pgrp(current);
- spin_lock_irqsave(&tty->ctrl.lock, flags);
+ if (!ctrl_lock)
+ spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
if (tty_pgrp && pgrp != tty_pgrp) {
@@ -57,7 +63,8 @@ int __tty_check_change(struct tty_struct *tty, int sig)
ret = -ERESTARTSYS;
}
}
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
+ if (!ctrl_lock)
+ spin_unlock_irqrestore(&tty->ctrl.lock, flags);
rcu_read_unlock();
if (!tty_pgrp)
@@ -66,9 +73,19 @@ int __tty_check_change(struct tty_struct *tty, int sig)
return ret;
}
+static int tty_check_change_locked(struct tty_struct *tty, spinlock_t *locked)
+{
+ return __tty_check_change_locked(tty, SIGTTOU, locked);
+}
+
+int __tty_check_change(struct tty_struct *tty, int sig)
+{
+ return __tty_check_change_locked(tty, sig, 0);
+}
+
int tty_check_change(struct tty_struct *tty)
{
- return __tty_check_change(tty, SIGTTOU);
+ return tty_check_change_locked(tty, 0);
}
EXPORT_SYMBOL(tty_check_change);
@@ -489,12 +506,7 @@ 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);
-
- if (retval == -EIO)
- return -ENOTTY;
- if (retval)
- return retval;
+ int retval;
if (get_user(pgrp_nr, p))
return -EFAULT;
@@ -502,6 +514,15 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
return -EINVAL;
spin_lock_irq(&real_tty->ctrl.lock);
+ retval = tty_check_change_locked(real_tty, &real_tty->ctrl.lock);
+
+ if (retval == -EIO) {
+ retval = -ENOTTY;
+ goto out_unlock_ctrl;
+ }
+ if (retval)
+ goto out_unlock_ctrl;
+
if (!current->signal->tty ||
(current->signal->tty != real_tty) ||
(real_tty->ctrl.session != task_session(current))) {
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers
2023-09-28 13:06 ` [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
@ 2023-09-29 6:07 ` kernel test robot
0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-09-29 6:07 UTC (permalink / raw)
To: Earl Chew, linux-kernel, gregkh, jirislaby
Cc: oe-kbuild-all, peter, earl.chew, kernel test robot
Hi Earl,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.6-rc3 next-20230928]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Earl-Chew/tty-Fix-__tty_check_change-and-tiocspgrp-race/20230929-010931
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230928130658.4045344-3-earl.chew%40yahoo.ca
patch subject: [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers
config: i386-randconfig-062-20230929 (https://download.01.org/0day-ci/archive/20230929/202309291311.hbeO1nmm-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/202309291311.hbeO1nmm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309291311.hbeO1nmm-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/tty/tty_jobctrl.c:83:52: sparse: sparse: Using plain integer as NULL pointer
drivers/tty/tty_jobctrl.c:88:45: sparse: sparse: Using plain integer as NULL pointer
drivers/tty/tty_jobctrl.c:97:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:97:9: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:97:9: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:100:34: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:100:34: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:100:34: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:141:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:141:31: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:141:31: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:143:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:143:33: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:143:33: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:152:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:152:31: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:152:31: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:173:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:173:33: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:173:33: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:182:9: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:182:9: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:182:9: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:184:40: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:184:40: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:184:40: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:222:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:222:41: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:222:41: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:232:51: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:232:51: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:232:51: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:244:43: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:244:43: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:244:43: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:308:39: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:308:39: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:308:39: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:311:41: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:311:41: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:311:41: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:340:31: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:340:31: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:340:31: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:343:33: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected struct spinlock [usertype] *lock @@ got struct spinlock [noderef] __rcu * @@
drivers/tty/tty_jobctrl.c:343:33: sparse: expected struct spinlock [usertype] *lock
drivers/tty/tty_jobctrl.c:343:33: sparse: got struct spinlock [noderef] __rcu *
drivers/tty/tty_jobctrl.c:19:41: sparse: sparse: dereference of noderef expression
drivers/tty/tty_jobctrl.c: note: in included file (through include/linux/rculist.h, include/linux/sched/signal.h):
include/linux/rcupdate.h:778:9: sparse: sparse: context imbalance in '__tty_check_change_locked' - different lock contexts for basic block
vim +83 drivers/tty/tty_jobctrl.c
80
81 int __tty_check_change(struct tty_struct *tty, int sig)
82 {
> 83 return __tty_check_change_locked(tty, sig, 0);
84 }
85
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
` (2 preceding siblings ...)
2023-09-28 13:06 ` [PATCH v2 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
@ 2023-09-28 13:06 ` Earl Chew
3 siblings, 0 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-28 13:06 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
Refactor __tty_check_change_locked() for consistency
with __proc_set_tty() which calls task_pgrp() after
tty->ctrl.lock is acquired.
In addition, spin_lock_irqsave() can block, while
task_pgrp() cannot block. Fetching the process
group immediately before it is used reduces the
window for inconsistency, and improves clarity.
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index da028aadf..e82ffd875 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -44,13 +44,13 @@ static int __tty_check_change_locked(struct tty_struct *tty, int sig,
if (current->signal->tty != tty)
return 0;
- rcu_read_lock();
- pgrp = task_pgrp(current);
-
if (!ctrl_lock)
spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
+ rcu_read_lock();
+ pgrp = task_pgrp(current);
+
if (tty_pgrp && pgrp != tty_pgrp) {
if (is_ignored(sig)) {
if (sig == SIGTTIN)
@@ -63,9 +63,9 @@ static int __tty_check_change_locked(struct tty_struct *tty, int sig,
ret = -ERESTARTSYS;
}
}
+ rcu_read_unlock();
if (!ctrl_lock)
spin_unlock_irqrestore(&tty->ctrl.lock, flags);
- rcu_read_unlock();
if (!tty_pgrp)
tty_warn(tty, "sig=%d, tty->pgrp == NULL!\n", sig);
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/3] tty: Fix __tty_check_change() and tiocspgrp() race
[not found] <20230831143645.2298799-1-earl_chew@yahoo.com>
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
@ 2023-09-01 1:50 ` Earl Chew
2023-09-01 1:50 ` [PATCH 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
2023-09-01 1:50 ` [PATCH 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency Earl Chew
3 siblings, 0 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-01 1:50 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
Restore the use of tty->ctrl.lock to cover both checking
current->task_pgrp() and sending kill_pgrp(). This coverage
was lost for SIGTTIN as part of commit 2812d9e9fd94
(tty: Combine SIGTTOU/SIGTTIN handling). In rare cases,
job control shells using tcsetpgrp() and SIGCONT to move
a background job to the foreground could find the new
foreground job stopping almost immediately due to SIGTTIN
or SIGTTOU.
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index 0d04287da..aba721a3c 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -44,7 +44,6 @@ int __tty_check_change(struct tty_struct *tty, int sig)
spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
if (tty_pgrp && pgrp != tty_pgrp) {
if (is_ignored(sig)) {
@@ -58,6 +57,7 @@ int __tty_check_change(struct tty_struct *tty, int sig)
ret = -ERESTARTSYS;
}
}
+ spin_unlock_irqrestore(&tty->ctrl.lock, flags);
rcu_read_unlock();
if (!tty_pgrp)
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
[not found] <20230831143645.2298799-1-earl_chew@yahoo.com>
2023-09-01 1:50 ` [PATCH 0/3] tty: Fix tiocspgrp() related races Earl Chew
2023-09-01 1:50 ` [PATCH 1/3] tty: Fix __tty_check_change() and tiocspgrp() race Earl Chew
@ 2023-09-01 1:50 ` Earl Chew
2023-09-01 16:09 ` kernel test robot
` (2 more replies)
2023-09-01 1:50 ` [PATCH 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency Earl Chew
3 siblings, 3 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-01 1:50 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
Only lock tty->ctrl.lock once when processing requests
in tiocspgrp() to serialise concurrent changes. Since
the introduction of tty->ctrl.lock in commit 47f86834bbd4
(redo locking of tty->pgrp), tiocspgrp() has acquired the
lock twice: first to check the process group, and next to
change the process group. In the rare case of multiple
callers, all can pass the process group check before each
taking turns to update the process group.
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 40 +++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index aba721a3c..462fdf7b2 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -30,19 +30,24 @@ static int is_ignored(int sig)
*
* Locking: ctrl.lock
*/
-int __tty_check_change(struct tty_struct *tty, int sig)
+static int __tty_check_change_locked(struct tty_struct *tty, int sig,
+ spinlock_t *ctrl_lock)
{
unsigned long flags;
struct pid *pgrp, *tty_pgrp;
int ret = 0;
+ BUG_ON(ctrl_lock && (
+ ctrl_lock != &tty->ctrl.lock || !spin_is_locked(ctrl_lock)));
+
if (current->signal->tty != tty)
return 0;
rcu_read_lock();
pgrp = task_pgrp(current);
- spin_lock_irqsave(&tty->ctrl.lock, flags);
+ if (!ctrl_lock)
+ spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
if (tty_pgrp && pgrp != tty_pgrp) {
@@ -57,7 +62,8 @@ int __tty_check_change(struct tty_struct *tty, int sig)
ret = -ERESTARTSYS;
}
}
- spin_unlock_irqrestore(&tty->ctrl.lock, flags);
+ if (!ctrl_lock)
+ spin_unlock_irqrestore(&tty->ctrl.lock, flags);
rcu_read_unlock();
if (!tty_pgrp)
@@ -66,9 +72,19 @@ int __tty_check_change(struct tty_struct *tty, int sig)
return ret;
}
+static int tty_check_change_locked(struct tty_struct *tty, spinlock_t *locked)
+{
+ return __tty_check_change_locked(tty, SIGTTOU, locked);
+}
+
+int __tty_check_change(struct tty_struct *tty, int sig)
+{
+ return __tty_check_change_locked(tty, sig, 0);
+}
+
int tty_check_change(struct tty_struct *tty)
{
- return __tty_check_change(tty, SIGTTOU);
+ return tty_check_change_locked(tty, 0);
}
EXPORT_SYMBOL(tty_check_change);
@@ -489,12 +505,7 @@ 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);
-
- if (retval == -EIO)
- return -ENOTTY;
- if (retval)
- return retval;
+ int retval;
if (get_user(pgrp_nr, p))
return -EFAULT;
@@ -502,6 +513,15 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
return -EINVAL;
spin_lock_irq(&real_tty->ctrl.lock);
+ retval = tty_check_change_locked(real_tty, &real_tty->ctrl.lock);
+
+ if (retval == -EIO) {
+ retval = -ENOTTY;
+ goto out_unlock_ctrl;
+ }
+ if (retval)
+ goto out_unlock_ctrl;
+
if (!current->signal->tty ||
(current->signal->tty != real_tty) ||
(real_tty->ctrl.session != task_session(current))) {
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
2023-09-01 1:50 ` [PATCH 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
@ 2023-09-01 16:09 ` kernel test robot
2023-09-01 17:14 ` kernel test robot
2023-09-28 21:00 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-09-01 16:09 UTC (permalink / raw)
To: Earl Chew, linux-kernel, gregkh, jirislaby
Cc: oe-kbuild-all, peter, earl.chew
Hi Earl,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.5 next-20230831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Earl-Chew/tty-Serialise-racing-tiocspgrp-callers/20230901-095411
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230901015030.2469062-3-earl.chew%40yahoo.ca
patch subject: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230901/202309012304.un7EAdgl-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230901/202309012304.un7EAdgl-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309012304.un7EAdgl-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/tty/tty_jobctrl.c:35: warning: Function parameter or member 'ctrl_lock' not described in '__tty_check_change_locked'
>> drivers/tty/tty_jobctrl.c:35: warning: expecting prototype for __tty_check_change(). Prototype was for __tty_check_change_locked() instead
vim +35 drivers/tty/tty_jobctrl.c
a1235b3eb10086b Nicolas Pitre 2017-04-12 21
a1235b3eb10086b Nicolas Pitre 2017-04-12 22 /**
6ef6785d781e9ce Lee Jones 2021-05-20 23 * __tty_check_change - check for POSIX terminal changes
a1235b3eb10086b Nicolas Pitre 2017-04-12 24 * @tty: tty to check
bc38fe241bc320f Lee Jones 2020-11-04 25 * @sig: signal to send
a1235b3eb10086b Nicolas Pitre 2017-04-12 26 *
a1235b3eb10086b Nicolas Pitre 2017-04-12 27 * If we try to write to, or set the state of, a terminal and we're
a1235b3eb10086b Nicolas Pitre 2017-04-12 28 * not in the foreground, send a SIGTTOU. If the signal is blocked or
a1235b3eb10086b Nicolas Pitre 2017-04-12 29 * ignored, go ahead and perform the operation. (POSIX 7.2)
a1235b3eb10086b Nicolas Pitre 2017-04-12 30 *
64d608db38ffc0c Jiri Slaby 2021-05-05 31 * Locking: ctrl.lock
a1235b3eb10086b Nicolas Pitre 2017-04-12 32 */
9aa37b12858f4bd Earl Chew 2023-08-31 33 static int __tty_check_change_locked(struct tty_struct *tty, int sig,
9aa37b12858f4bd Earl Chew 2023-08-31 34 spinlock_t *ctrl_lock)
a1235b3eb10086b Nicolas Pitre 2017-04-12 @35 {
a1235b3eb10086b Nicolas Pitre 2017-04-12 36 unsigned long flags;
a1235b3eb10086b Nicolas Pitre 2017-04-12 37 struct pid *pgrp, *tty_pgrp;
a1235b3eb10086b Nicolas Pitre 2017-04-12 38 int ret = 0;
a1235b3eb10086b Nicolas Pitre 2017-04-12 39
9aa37b12858f4bd Earl Chew 2023-08-31 40 BUG_ON(ctrl_lock && (
9aa37b12858f4bd Earl Chew 2023-08-31 41 ctrl_lock != &tty->ctrl.lock || !spin_is_locked(ctrl_lock)));
9aa37b12858f4bd Earl Chew 2023-08-31 42
a1235b3eb10086b Nicolas Pitre 2017-04-12 43 if (current->signal->tty != tty)
a1235b3eb10086b Nicolas Pitre 2017-04-12 44 return 0;
a1235b3eb10086b Nicolas Pitre 2017-04-12 45
a1235b3eb10086b Nicolas Pitre 2017-04-12 46 rcu_read_lock();
a1235b3eb10086b Nicolas Pitre 2017-04-12 47 pgrp = task_pgrp(current);
a1235b3eb10086b Nicolas Pitre 2017-04-12 48
9aa37b12858f4bd Earl Chew 2023-08-31 49 if (!ctrl_lock)
64d608db38ffc0c Jiri Slaby 2021-05-05 50 spin_lock_irqsave(&tty->ctrl.lock, flags);
64d608db38ffc0c Jiri Slaby 2021-05-05 51 tty_pgrp = tty->ctrl.pgrp;
a1235b3eb10086b Nicolas Pitre 2017-04-12 52
cf90c06f8115016 David Emett 2019-03-10 53 if (tty_pgrp && pgrp != tty_pgrp) {
a1235b3eb10086b Nicolas Pitre 2017-04-12 54 if (is_ignored(sig)) {
a1235b3eb10086b Nicolas Pitre 2017-04-12 55 if (sig == SIGTTIN)
a1235b3eb10086b Nicolas Pitre 2017-04-12 56 ret = -EIO;
a1235b3eb10086b Nicolas Pitre 2017-04-12 57 } else if (is_current_pgrp_orphaned())
a1235b3eb10086b Nicolas Pitre 2017-04-12 58 ret = -EIO;
a1235b3eb10086b Nicolas Pitre 2017-04-12 59 else {
a1235b3eb10086b Nicolas Pitre 2017-04-12 60 kill_pgrp(pgrp, sig, 1);
a1235b3eb10086b Nicolas Pitre 2017-04-12 61 set_thread_flag(TIF_SIGPENDING);
a1235b3eb10086b Nicolas Pitre 2017-04-12 62 ret = -ERESTARTSYS;
a1235b3eb10086b Nicolas Pitre 2017-04-12 63 }
a1235b3eb10086b Nicolas Pitre 2017-04-12 64 }
9aa37b12858f4bd Earl Chew 2023-08-31 65 if (!ctrl_lock)
03beb0c7227a52e Earl Chew 2023-08-31 66 spin_unlock_irqrestore(&tty->ctrl.lock, flags);
a1235b3eb10086b Nicolas Pitre 2017-04-12 67 rcu_read_unlock();
a1235b3eb10086b Nicolas Pitre 2017-04-12 68
a1235b3eb10086b Nicolas Pitre 2017-04-12 69 if (!tty_pgrp)
a1235b3eb10086b Nicolas Pitre 2017-04-12 70 tty_warn(tty, "sig=%d, tty->pgrp == NULL!\n", sig);
a1235b3eb10086b Nicolas Pitre 2017-04-12 71
a1235b3eb10086b Nicolas Pitre 2017-04-12 72 return ret;
a1235b3eb10086b Nicolas Pitre 2017-04-12 73 }
a1235b3eb10086b Nicolas Pitre 2017-04-12 74
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
2023-09-01 1:50 ` [PATCH 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
2023-09-01 16:09 ` kernel test robot
@ 2023-09-01 17:14 ` kernel test robot
2023-09-28 21:00 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-09-01 17:14 UTC (permalink / raw)
To: Earl Chew, linux-kernel, gregkh, jirislaby
Cc: llvm, oe-kbuild-all, peter, earl.chew
Hi Earl,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.5 next-20230831]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Earl-Chew/tty-Serialise-racing-tiocspgrp-callers/20230901-095411
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230901015030.2469062-3-earl.chew%40yahoo.ca
patch subject: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
config: mips-randconfig-r025-20230901 (https://download.01.org/0day-ci/archive/20230902/202309020016.GL35iYlP-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230902/202309020016.GL35iYlP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309020016.GL35iYlP-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/tty/tty_jobctrl.c:35: warning: Function parameter or member 'ctrl_lock' not described in '__tty_check_change_locked'
>> drivers/tty/tty_jobctrl.c:35: warning: expecting prototype for __tty_check_change(). Prototype was for __tty_check_change_locked() instead
vim +35 drivers/tty/tty_jobctrl.c
a1235b3eb10086b Nicolas Pitre 2017-04-12 21
a1235b3eb10086b Nicolas Pitre 2017-04-12 22 /**
6ef6785d781e9ce Lee Jones 2021-05-20 23 * __tty_check_change - check for POSIX terminal changes
a1235b3eb10086b Nicolas Pitre 2017-04-12 24 * @tty: tty to check
bc38fe241bc320f Lee Jones 2020-11-04 25 * @sig: signal to send
a1235b3eb10086b Nicolas Pitre 2017-04-12 26 *
a1235b3eb10086b Nicolas Pitre 2017-04-12 27 * If we try to write to, or set the state of, a terminal and we're
a1235b3eb10086b Nicolas Pitre 2017-04-12 28 * not in the foreground, send a SIGTTOU. If the signal is blocked or
a1235b3eb10086b Nicolas Pitre 2017-04-12 29 * ignored, go ahead and perform the operation. (POSIX 7.2)
a1235b3eb10086b Nicolas Pitre 2017-04-12 30 *
64d608db38ffc0c Jiri Slaby 2021-05-05 31 * Locking: ctrl.lock
a1235b3eb10086b Nicolas Pitre 2017-04-12 32 */
9aa37b12858f4bd Earl Chew 2023-08-31 33 static int __tty_check_change_locked(struct tty_struct *tty, int sig,
9aa37b12858f4bd Earl Chew 2023-08-31 34 spinlock_t *ctrl_lock)
a1235b3eb10086b Nicolas Pitre 2017-04-12 @35 {
a1235b3eb10086b Nicolas Pitre 2017-04-12 36 unsigned long flags;
a1235b3eb10086b Nicolas Pitre 2017-04-12 37 struct pid *pgrp, *tty_pgrp;
a1235b3eb10086b Nicolas Pitre 2017-04-12 38 int ret = 0;
a1235b3eb10086b Nicolas Pitre 2017-04-12 39
9aa37b12858f4bd Earl Chew 2023-08-31 40 BUG_ON(ctrl_lock && (
9aa37b12858f4bd Earl Chew 2023-08-31 41 ctrl_lock != &tty->ctrl.lock || !spin_is_locked(ctrl_lock)));
9aa37b12858f4bd Earl Chew 2023-08-31 42
a1235b3eb10086b Nicolas Pitre 2017-04-12 43 if (current->signal->tty != tty)
a1235b3eb10086b Nicolas Pitre 2017-04-12 44 return 0;
a1235b3eb10086b Nicolas Pitre 2017-04-12 45
a1235b3eb10086b Nicolas Pitre 2017-04-12 46 rcu_read_lock();
a1235b3eb10086b Nicolas Pitre 2017-04-12 47 pgrp = task_pgrp(current);
a1235b3eb10086b Nicolas Pitre 2017-04-12 48
9aa37b12858f4bd Earl Chew 2023-08-31 49 if (!ctrl_lock)
64d608db38ffc0c Jiri Slaby 2021-05-05 50 spin_lock_irqsave(&tty->ctrl.lock, flags);
64d608db38ffc0c Jiri Slaby 2021-05-05 51 tty_pgrp = tty->ctrl.pgrp;
a1235b3eb10086b Nicolas Pitre 2017-04-12 52
cf90c06f8115016 David Emett 2019-03-10 53 if (tty_pgrp && pgrp != tty_pgrp) {
a1235b3eb10086b Nicolas Pitre 2017-04-12 54 if (is_ignored(sig)) {
a1235b3eb10086b Nicolas Pitre 2017-04-12 55 if (sig == SIGTTIN)
a1235b3eb10086b Nicolas Pitre 2017-04-12 56 ret = -EIO;
a1235b3eb10086b Nicolas Pitre 2017-04-12 57 } else if (is_current_pgrp_orphaned())
a1235b3eb10086b Nicolas Pitre 2017-04-12 58 ret = -EIO;
a1235b3eb10086b Nicolas Pitre 2017-04-12 59 else {
a1235b3eb10086b Nicolas Pitre 2017-04-12 60 kill_pgrp(pgrp, sig, 1);
a1235b3eb10086b Nicolas Pitre 2017-04-12 61 set_thread_flag(TIF_SIGPENDING);
a1235b3eb10086b Nicolas Pitre 2017-04-12 62 ret = -ERESTARTSYS;
a1235b3eb10086b Nicolas Pitre 2017-04-12 63 }
a1235b3eb10086b Nicolas Pitre 2017-04-12 64 }
9aa37b12858f4bd Earl Chew 2023-08-31 65 if (!ctrl_lock)
03beb0c7227a52e Earl Chew 2023-08-31 66 spin_unlock_irqrestore(&tty->ctrl.lock, flags);
a1235b3eb10086b Nicolas Pitre 2017-04-12 67 rcu_read_unlock();
a1235b3eb10086b Nicolas Pitre 2017-04-12 68
a1235b3eb10086b Nicolas Pitre 2017-04-12 69 if (!tty_pgrp)
a1235b3eb10086b Nicolas Pitre 2017-04-12 70 tty_warn(tty, "sig=%d, tty->pgrp == NULL!\n", sig);
a1235b3eb10086b Nicolas Pitre 2017-04-12 71
a1235b3eb10086b Nicolas Pitre 2017-04-12 72 return ret;
a1235b3eb10086b Nicolas Pitre 2017-04-12 73 }
a1235b3eb10086b Nicolas Pitre 2017-04-12 74
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
2023-09-01 1:50 ` [PATCH 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
2023-09-01 16:09 ` kernel test robot
2023-09-01 17:14 ` kernel test robot
@ 2023-09-28 21:00 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2023-09-28 21:00 UTC (permalink / raw)
To: Earl Chew, linux-kernel, gregkh, jirislaby
Cc: oe-kbuild-all, peter, earl.chew
Hi Earl,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus linus/master v6.6-rc3 next-20230928]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Earl-Chew/tty-Serialise-racing-tiocspgrp-callers/20230929-010844
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link: https://lore.kernel.org/r/20230901015030.2469062-3-earl.chew%40yahoo.ca
patch subject: [PATCH 2/3] tty: Serialise racing tiocspgrp() callers
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230929/202309290422.8TLHGj3t-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/202309290422.8TLHGj3t-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309290422.8TLHGj3t-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/tty/tty_jobctrl.c:35: warning: Function parameter or member 'ctrl_lock' not described in '__tty_check_change_locked'
>> drivers/tty/tty_jobctrl.c:35: warning: expecting prototype for __tty_check_change(). Prototype was for __tty_check_change_locked() instead
vim +35 drivers/tty/tty_jobctrl.c
a1235b3eb10086 Nicolas Pitre 2017-04-12 21
a1235b3eb10086 Nicolas Pitre 2017-04-12 22 /**
6ef6785d781e9c Lee Jones 2021-05-20 23 * __tty_check_change - check for POSIX terminal changes
a1235b3eb10086 Nicolas Pitre 2017-04-12 24 * @tty: tty to check
bc38fe241bc320 Lee Jones 2020-11-04 25 * @sig: signal to send
a1235b3eb10086 Nicolas Pitre 2017-04-12 26 *
a1235b3eb10086 Nicolas Pitre 2017-04-12 27 * If we try to write to, or set the state of, a terminal and we're
a1235b3eb10086 Nicolas Pitre 2017-04-12 28 * not in the foreground, send a SIGTTOU. If the signal is blocked or
a1235b3eb10086 Nicolas Pitre 2017-04-12 29 * ignored, go ahead and perform the operation. (POSIX 7.2)
a1235b3eb10086 Nicolas Pitre 2017-04-12 30 *
64d608db38ffc0 Jiri Slaby 2021-05-05 31 * Locking: ctrl.lock
a1235b3eb10086 Nicolas Pitre 2017-04-12 32 */
99d6f12e388e7a Earl Chew 2023-08-31 33 static int __tty_check_change_locked(struct tty_struct *tty, int sig,
99d6f12e388e7a Earl Chew 2023-08-31 34 spinlock_t *ctrl_lock)
a1235b3eb10086 Nicolas Pitre 2017-04-12 @35 {
a1235b3eb10086 Nicolas Pitre 2017-04-12 36 unsigned long flags;
a1235b3eb10086 Nicolas Pitre 2017-04-12 37 struct pid *pgrp, *tty_pgrp;
a1235b3eb10086 Nicolas Pitre 2017-04-12 38 int ret = 0;
a1235b3eb10086 Nicolas Pitre 2017-04-12 39
99d6f12e388e7a Earl Chew 2023-08-31 40 BUG_ON(ctrl_lock && (
99d6f12e388e7a Earl Chew 2023-08-31 41 ctrl_lock != &tty->ctrl.lock || !spin_is_locked(ctrl_lock)));
99d6f12e388e7a Earl Chew 2023-08-31 42
a1235b3eb10086 Nicolas Pitre 2017-04-12 43 if (current->signal->tty != tty)
a1235b3eb10086 Nicolas Pitre 2017-04-12 44 return 0;
a1235b3eb10086 Nicolas Pitre 2017-04-12 45
a1235b3eb10086 Nicolas Pitre 2017-04-12 46 rcu_read_lock();
a1235b3eb10086 Nicolas Pitre 2017-04-12 47 pgrp = task_pgrp(current);
a1235b3eb10086 Nicolas Pitre 2017-04-12 48
99d6f12e388e7a Earl Chew 2023-08-31 49 if (!ctrl_lock)
64d608db38ffc0 Jiri Slaby 2021-05-05 50 spin_lock_irqsave(&tty->ctrl.lock, flags);
64d608db38ffc0 Jiri Slaby 2021-05-05 51 tty_pgrp = tty->ctrl.pgrp;
a1235b3eb10086 Nicolas Pitre 2017-04-12 52
cf90c06f811501 David Emett 2019-03-10 53 if (tty_pgrp && pgrp != tty_pgrp) {
a1235b3eb10086 Nicolas Pitre 2017-04-12 54 if (is_ignored(sig)) {
a1235b3eb10086 Nicolas Pitre 2017-04-12 55 if (sig == SIGTTIN)
a1235b3eb10086 Nicolas Pitre 2017-04-12 56 ret = -EIO;
a1235b3eb10086 Nicolas Pitre 2017-04-12 57 } else if (is_current_pgrp_orphaned())
a1235b3eb10086 Nicolas Pitre 2017-04-12 58 ret = -EIO;
a1235b3eb10086 Nicolas Pitre 2017-04-12 59 else {
a1235b3eb10086 Nicolas Pitre 2017-04-12 60 kill_pgrp(pgrp, sig, 1);
a1235b3eb10086 Nicolas Pitre 2017-04-12 61 set_thread_flag(TIF_SIGPENDING);
a1235b3eb10086 Nicolas Pitre 2017-04-12 62 ret = -ERESTARTSYS;
a1235b3eb10086 Nicolas Pitre 2017-04-12 63 }
a1235b3eb10086 Nicolas Pitre 2017-04-12 64 }
99d6f12e388e7a Earl Chew 2023-08-31 65 if (!ctrl_lock)
95d3590fbe7aed Earl Chew 2023-08-31 66 spin_unlock_irqrestore(&tty->ctrl.lock, flags);
a1235b3eb10086 Nicolas Pitre 2017-04-12 67 rcu_read_unlock();
a1235b3eb10086 Nicolas Pitre 2017-04-12 68
a1235b3eb10086 Nicolas Pitre 2017-04-12 69 if (!tty_pgrp)
a1235b3eb10086 Nicolas Pitre 2017-04-12 70 tty_warn(tty, "sig=%d, tty->pgrp == NULL!\n", sig);
a1235b3eb10086 Nicolas Pitre 2017-04-12 71
a1235b3eb10086 Nicolas Pitre 2017-04-12 72 return ret;
a1235b3eb10086 Nicolas Pitre 2017-04-12 73 }
a1235b3eb10086 Nicolas Pitre 2017-04-12 74
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] tty: Move task_pgrp() after tty->ctrl.lock for consistency
[not found] <20230831143645.2298799-1-earl_chew@yahoo.com>
` (2 preceding siblings ...)
2023-09-01 1:50 ` [PATCH 2/3] tty: Serialise racing tiocspgrp() callers Earl Chew
@ 2023-09-01 1:50 ` Earl Chew
3 siblings, 0 replies; 13+ messages in thread
From: Earl Chew @ 2023-09-01 1:50 UTC (permalink / raw)
To: linux-kernel, gregkh, jirislaby; +Cc: peter, earl.chew
Refactor __tty_check_change_locked() for consistency
with __proc_set_tty() which calls task_pgrp() after
tty->ctrl.lock is acquired.
In addition, spin_lock_irqsave() can block, while
task_pgrp() cannot block. Fetching the process
group immediately before it is used reduces the
window for inconsistency, and improves clarity.
Signed-off-by: Earl Chew <earl.chew@yahoo.ca>
---
drivers/tty/tty_jobctrl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index 462fdf7b2..b4d37cad9 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -43,13 +43,13 @@ static int __tty_check_change_locked(struct tty_struct *tty, int sig,
if (current->signal->tty != tty)
return 0;
- rcu_read_lock();
- pgrp = task_pgrp(current);
-
if (!ctrl_lock)
spin_lock_irqsave(&tty->ctrl.lock, flags);
tty_pgrp = tty->ctrl.pgrp;
+ rcu_read_lock();
+ pgrp = task_pgrp(current);
+
if (tty_pgrp && pgrp != tty_pgrp) {
if (is_ignored(sig)) {
if (sig == SIGTTIN)
@@ -62,9 +62,9 @@ static int __tty_check_change_locked(struct tty_struct *tty, int sig,
ret = -ERESTARTSYS;
}
}
+ rcu_read_unlock();
if (!ctrl_lock)
spin_unlock_irqrestore(&tty->ctrl.lock, flags);
- rcu_read_unlock();
if (!tty_pgrp)
tty_warn(tty, "sig=%d, tty->pgrp == NULL!\n", sig);
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread