On 2021/04/12 19:44, Greg Kroah-Hartman wrote:
> And trying to "open exclusive only" just does not work, the kernel can
> not enforce that at all, sorry. Any driver that you see trying to do
> that is trivial to work around in userspace, making the kernel code
> pointless.
You mean something like below cannot be used?
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 6a0059e508e3..57200569918a 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -84,14 +84,26 @@ static int tpk_printk(const unsigned char *buf, int count)
return count;
}
+static DEFINE_MUTEX(open_close_lock);
+
/*
* TTY operations open function.
*/
static int tpk_open(struct tty_struct *tty, struct file *filp)
{
- tty->driver_data = &tpk_port;
+ int ret;
+
+ if (mutex_lock_killable(&open_close_lock))
+ return -EINTR;
- return tty_port_open(&tpk_port.port, tty, filp);
+ if (tpk_port.port.count) {
+ ret = -EBUSY;
+ } else {
+ tty->driver_data = &tpk_port;
+ ret = tty_port_open(&tpk_port.port, tty, filp);
+ }
+ mutex_unlock(&open_close_lock);
+ return ret;
}
/*
@@ -102,12 +114,14 @@ static void tpk_close(struct tty_struct *tty, struct file *filp)
struct ttyprintk_port *tpkp = tty->driver_data;
unsigned long flags;
+ mutex_lock(&open_close_lock);
spin_lock_irqsave(&tpkp->spinlock, flags);
/* flush tpk_printk buffer */
tpk_printk(NULL, 0);
spin_unlock_irqrestore(&tpkp->spinlock, flags);
tty_port_close(&tpkp->port, tty, filp);
+ mutex_unlock(&open_close_lock);
}
/*
> Like any tty port, if you have multiple accesses, all bets are off and
> hilarity ensues. Just don't do that and expect things to be working
> well.
Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n ?
On Mon, Apr 12, 2021 at 08:25:27PM +0900, Tetsuo Handa wrote:
> On 2021/04/12 19:44, Greg Kroah-Hartman wrote:
> > And trying to "open exclusive only" just does not work, the kernel can
> > not enforce that at all, sorry. Any driver that you see trying to do
> > that is trivial to work around in userspace, making the kernel code
> > pointless.
>
> You mean something like below cannot be used?
>
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 6a0059e508e3..57200569918a 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -84,14 +84,26 @@ static int tpk_printk(const unsigned char *buf, int count)
> return count;
> }
>
> +static DEFINE_MUTEX(open_close_lock);
Hah, nope, does not work at all!
Think about sending an open file descriptor all around the system, or
through a pipe, your "open only once" check does not prevent that at
all.
> > Like any tty port, if you have multiple accesses, all bets are off and
> > hilarity ensues. Just don't do that and expect things to be working
> > well.
>
> Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
> multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n ?
Why? Can you not hit the same tty code paths from any other tty driver
being open multiple times? Why is ttyprintk somehow "special" here?
thanks,
greg k-h
On 2021/04/12 21:04, Greg Kroah-Hartman wrote:
>> Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
>> multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n ?
>
> Why? Can you not hit the same tty code paths from any other tty driver
> being open multiple times? Why is ttyprintk somehow "special" here?
I found a simplified reproducer. If we call ioctl(TIOCVHANGUP) on /dev/ttyprintk ,
"struct ttyprintk_port tpk_port".port.count cannot be decremented by
tty_port_close() from tpk_close() due to tty_hung_up_p() == true when
close() is called. As a result, tty->count and port count gets out of sync.
Then, when /dev/ttyprintk is opened again and then closed without calling
ioctl(TIOCVHANGUP), this message is printed due to tty_hung_up_p() == false.
---------- Debug print patch start ----------
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 391bada4cedb..42d54368adac 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -113,7 +113,7 @@
#ifdef TTY_DEBUG_HANGUP
# define tty_debug_hangup(tty, f, args...) tty_debug(tty, f, ##args)
#else
-# define tty_debug_hangup(tty, f, args...) do { } while (0)
+# define tty_debug_hangup(tty, f, args...) tty_info(tty, f, ##args)
#endif
#define TTY_PARANOIA_CHECK 1
@@ -628,6 +628,7 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
closecount++;
__tty_fasync(-1, filp, 0); /* can't block */
filp->f_op = &hung_up_tty_fops;
+ tty_warn(tty, "Set hung_up_tty_fops on %px\n", filp);
}
spin_unlock(&tty->files_lock);
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 346d20f4a486..032fc58203ea 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -561,8 +561,10 @@ int tty_port_close_start(struct tty_port *port,
{
unsigned long flags;
- if (tty_hung_up_p(filp))
+ if (tty_hung_up_p(filp)) {
+ tty_warn(tty, "Skipped by hung_up_tty_fops on %px\n", filp);
return 0;
+ }
spin_lock_irqsave(&port->lock, flags);
if (tty->count == 1 && port->count != 1) {
---------- Debug print patch end ----------
---------- Reproducer start ----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <unistd.h>
int main(int argc, char *argv[])
{
int i;
int fd[10];
for (i = 0; i < 10; i++)
fd[i] = open("/dev/ttyprintk", O_WRONLY);
ioctl(fd[0], TIOCVHANGUP);
for (i = 0; i < 10; i++)
close(fd[i]);
close(open("/dev/ttyprintk", O_WRONLY));
return 0;
}
---------- Reproducer end ----------
---------- Console output start ----------
[ 31.885820] ttyprintk ttyprintk: opening (count=1)
[ 31.885859] ttyprintk ttyprintk: opening (count=2)
[ 31.885876] ttyprintk ttyprintk: opening (count=3)
[ 31.885892] ttyprintk ttyprintk: opening (count=4)
[ 31.885907] ttyprintk ttyprintk: opening (count=5)
[ 31.885923] ttyprintk ttyprintk: opening (count=6)
[ 31.885939] ttyprintk ttyprintk: opening (count=7)
[ 31.885956] ttyprintk ttyprintk: opening (count=8)
[ 31.885971] ttyprintk ttyprintk: opening (count=9)
[ 31.885988] ttyprintk ttyprintk: opening (count=10)
[ 31.885999] ttyprintk ttyprintk: vhangup
[ 31.886005] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed1000
[ 31.886009] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed2000
[ 31.886012] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed0c00
[ 31.886016] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed0e00
[ 31.886019] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed2c00
[ 31.886023] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed3a00
[ 31.886026] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed3400
[ 31.886029] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed3c00
[ 31.886033] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed0200
[ 31.886036] ttyprintk ttyprintk: Set hung_up_tty_fops on ffff97c38eed3e00
[ 31.886055] ttyprintk ttyprintk: releasing (count=10)
[ 31.886106] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed3e00
[ 31.886117] ttyprintk ttyprintk: releasing (count=9)
[ 31.886121] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed0200
[ 31.886128] ttyprintk ttyprintk: releasing (count=8)
[ 31.886131] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed3c00
[ 31.886138] ttyprintk ttyprintk: releasing (count=7)
[ 31.886141] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed3400
[ 31.886148] ttyprintk ttyprintk: releasing (count=6)
[ 31.886151] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed3a00
[ 31.886158] ttyprintk ttyprintk: releasing (count=5)
[ 31.886161] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed2c00
[ 31.886168] ttyprintk ttyprintk: releasing (count=4)
[ 31.886171] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed0e00
[ 31.886178] ttyprintk ttyprintk: releasing (count=3)
[ 31.886181] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed0c00
[ 31.886188] ttyprintk ttyprintk: releasing (count=2)
[ 31.886191] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed2000
[ 31.886198] ttyprintk ttyprintk: releasing (count=1)
[ 31.886201] ttyprintk ttyprintk: Skipped by hung_up_tty_fops on ffff97c38eed1000
[ 31.886205] ttyprintk ttyprintk: final close
[ 31.886211] ttyprintk ttyprintk: freeing structure
[ 31.886277] ttyprintk ttyprintk: opening (count=1)
[ 31.886531] ttyprintk ttyprintk: releasing (count=1)
[ 31.886535] ttyprintk ttyprintk: tty_port_close_start: tty->count = 1 port count = 11
[ 31.886543] ttyprintk ttyprintk: final close
[ 31.886552] ttyprintk ttyprintk: freeing structure
---------- Console output end ----------
If I replace /dev/ttyprintk with /dev/ttyS0 in the reproducer shown above,
this message is not printed.
---------- Console output start ----------
[ 10.794802] ttyS ttyS0: opening (count=1)
[ 10.795851] ttyS ttyS0: opening (count=2)
[ 10.795872] ttyS ttyS0: opening (count=3)
[ 10.795890] ttyS ttyS0: opening (count=4)
[ 10.795908] ttyS ttyS0: opening (count=5)
[ 10.795925] ttyS ttyS0: opening (count=6)
[ 10.795943] ttyS ttyS0: opening (count=7)
[ 10.795960] ttyS ttyS0: opening (count=8)
[ 10.795978] ttyS ttyS0: opening (count=9)
[ 10.795995] ttyS ttyS0: opening (count=10)
[ 10.796007] ttyS ttyS0: vhangup
[ 10.796014] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc5600
[ 10.796020] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc6400
[ 10.796025] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc4a00
[ 10.796030] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc6800
[ 10.796035] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc6200
[ 10.796039] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc5a00
[ 10.796044] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc6600
[ 10.796048] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc7600
[ 10.796053] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc5e00
[ 10.796058] ttyS ttyS0: Set hung_up_tty_fops on ffff9cb044dc6000
[ 10.796401] ttyS ttyS0: releasing (count=10)
[ 10.796408] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc6000
[ 10.796434] ttyS ttyS0: releasing (count=9)
[ 10.796439] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc5e00
[ 10.796447] ttyS ttyS0: releasing (count=8)
[ 10.796451] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc7600
[ 10.796459] ttyS ttyS0: releasing (count=7)
[ 10.796463] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc6600
[ 10.796472] ttyS ttyS0: releasing (count=6)
[ 10.796476] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc5a00
[ 10.796484] ttyS ttyS0: releasing (count=5)
[ 10.796488] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc6200
[ 10.796496] ttyS ttyS0: releasing (count=4)
[ 10.796500] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc6800
[ 10.796508] ttyS ttyS0: releasing (count=3)
[ 10.796512] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc4a00
[ 10.796521] ttyS ttyS0: releasing (count=2)
[ 10.796525] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc6400
[ 10.796533] ttyS ttyS0: releasing (count=1)
[ 10.796537] ttyS ttyS0: Skipped by hung_up_tty_fops on ffff9cb044dc5600
[ 10.796542] ttyS ttyS0: final close
[ 10.796548] ttyS ttyS0: freeing structure
[ 10.796634] ttyS ttyS0: opening (count=1)
[ 10.797192] ttyS ttyS0: releasing (count=1)
[ 10.797413] ttyS ttyS0: final close
[ 10.797427] ttyS ttyS0: freeing structure
---------- Console output end ----------
On 2021/04/14 9:45, Tetsuo Handa wrote:
> On 2021/04/12 21:04, Greg Kroah-Hartman wrote:
>>> Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
>>> multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n ?
>>
>> Why? Can you not hit the same tty code paths from any other tty driver
>> being open multiple times? Why is ttyprintk somehow "special" here?
>
> I found a simplified reproducer. If we call ioctl(TIOCVHANGUP) on /dev/ttyprintk ,
> "struct ttyprintk_port tpk_port".port.count cannot be decremented by
> tty_port_close() from tpk_close() due to tty_hung_up_p() == true when
> close() is called. As a result, tty->count and port count gets out of sync.
>
> Then, when /dev/ttyprintk is opened again and then closed without calling
> ioctl(TIOCVHANGUP), this message is printed due to tty_hung_up_p() == false.
>
> If I replace /dev/ttyprintk with /dev/ttyS0 in the reproducer shown above,
> this message is not printed.
>
The difference between /dev/ttyS0 and /dev/ttyprintk is that
the former provides uart_hangup() as "struct tty_operations"->hangup
while the latter does not provide "struct tty_operations"->hangup .
It seems to me that below patch avoids this message, but I'm not familiar
with tty code. Is this fix correct? Don't we need to enforce all drivers
to provide "struct tty_operations"->hangup in order to reset port count ?
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 6a0059e508e3..ff3b9a41b91b 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -158,12 +158,26 @@ static int tpk_ioctl(struct tty_struct *tty,
return 0;
}
+/*
+ * TTY operations hangup function.
+ */
+static void tpk_hangup(struct tty_struct *tty)
+{
+ struct ttyprintk_port *tpkp = tty->driver_data;
+ unsigned long flags;
+
+ spin_lock_irqsave(&tpkp->port.lock, flags);
+ tpkp->port.count = 0;
+ spin_unlock_irqrestore(&tpkp->port.lock, flags);
+}
+
static const struct tty_operations ttyprintk_ops = {
.open = tpk_open,
.close = tpk_close,
.write = tpk_write,
.write_room = tpk_write_room,
.ioctl = tpk_ioctl,
+ .hangup = tpk_hangup,
};
static const struct tty_port_operations null_ops = { };
Dne 14.04.2021 (sre) ob 20:11 +0900 je Tetsuo Handa napisal(a):
> On 2021/04/14 9:45, Tetsuo Handa wrote:
> > On 2021/04/12 21:04, Greg Kroah-Hartman wrote:
> > > > Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
> > > > multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n
> > > > ?
> > >
> > > Why? Can you not hit the same tty code paths from any other tty driver
> > > being open multiple times? Why is ttyprintk somehow "special" here?
> >
> > I found a simplified reproducer. If we call ioctl(TIOCVHANGUP) on
> > /dev/ttyprintk ,
> > "struct ttyprintk_port tpk_port".port.count cannot be decremented by
> > tty_port_close() from tpk_close() due to tty_hung_up_p() == true when
> > close() is called. As a result, tty->count and port count gets out of sync.
> >
> > Then, when /dev/ttyprintk is opened again and then closed without calling
> > ioctl(TIOCVHANGUP), this message is printed due to tty_hung_up_p() == false.
> >
> > If I replace /dev/ttyprintk with /dev/ttyS0 in the reproducer shown above,
> > this message is not printed.
> >
>
> The difference between /dev/ttyS0 and /dev/ttyprintk is that
> the former provides uart_hangup() as "struct tty_operations"->hangup
> while the latter does not provide "struct tty_operations"->hangup .
>
> It seems to me that below patch avoids this message, but I'm not familiar
> with tty code. Is this fix correct? Don't we need to enforce all drivers
> to provide "struct tty_operations"->hangup in order to reset port count ?
>
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 6a0059e508e3..ff3b9a41b91b 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -158,12 +158,26 @@ static int tpk_ioctl(struct tty_struct *tty,
> return 0;
> }
>
> +/*
> + * TTY operations hangup function.
> + */
> +static void tpk_hangup(struct tty_struct *tty)
> +{
> + struct ttyprintk_port *tpkp = tty->driver_data;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tpkp->port.lock, flags);
> + tpkp->port.count = 0;
> + spin_unlock_irqrestore(&tpkp->port.lock, flags);
> +}
> +
> static const struct tty_operations ttyprintk_ops = {
> .open = tpk_open,
> .close = tpk_close,
> .write = tpk_write,
> .write_room = tpk_write_room,
> .ioctl = tpk_ioctl,
> + .hangup = tpk_hangup,
> };
>
> static const struct tty_port_operations null_ops = { };
Wish i could be of more help here, especially around locking.
If this addition is needed, i'd probably do something similar.
However, when comparing the code around other drivers it seems that
'tty_port_hangup()' should be used instead explicit 'port.count'
reset.
best regards, Samo
syzbot is reporting hung task due to flood of
tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
port->count);
message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
decrementing port->count due to tty_hung_up_p() == true.
----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <unistd.h>
int main(int argc, char *argv[])
{
int i;
int fd[10];
for (i = 0; i < 10; i++)
fd[i] = open("/dev/ttyprintk", O_WRONLY);
ioctl(fd[0], TIOCVHANGUP);
for (i = 0; i < 10; i++)
close(fd[i]);
close(open("/dev/ttyprintk", O_WRONLY));
return 0;
}
----------
When TTY hangup happens, port->count needs to be reset via
"struct tty_operations"->hangup callback.
[1] https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
Reported-by: syzbot <[email protected]>
Reported-by: syzbot <[email protected]>
Tested-by: syzbot <[email protected]>
Signed-off-by: Tetsuo Handa <[email protected]>
Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
---
drivers/char/ttyprintk.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 6a0059e508e3..93f5d11c830b 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
return 0;
}
+/*
+ * TTY operations hangup function.
+ */
+static void tpk_hangup(struct tty_struct *tty)
+{
+ struct ttyprintk_port *tpkp = tty->driver_data;
+
+ tty_port_hangup(&tpkp->port);
+}
+
static const struct tty_operations ttyprintk_ops = {
.open = tpk_open,
.close = tpk_close,
.write = tpk_write,
.write_room = tpk_write_room,
.ioctl = tpk_ioctl,
+ .hangup = tpk_hangup,
};
static const struct tty_port_operations null_ops = { };
--
2.18.4
Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
> syzbot is reporting hung task due to flood of
>
> tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
> port->count);
>
> message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
> decrementing port->count due to tty_hung_up_p() == true.
>
> ----------
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> int i;
> int fd[10];
>
> for (i = 0; i < 10; i++)
> fd[i] = open("/dev/ttyprintk", O_WRONLY);
> ioctl(fd[0], TIOCVHANGUP);
> for (i = 0; i < 10; i++)
> close(fd[i]);
> close(open("/dev/ttyprintk", O_WRONLY));
> return 0;
> }
> ----------
>
> When TTY hangup happens, port->count needs to be reset via
> "struct tty_operations"->hangup callback.
>
> [1]
> https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
>
> Reported-by: syzbot <[email protected]>
> Reported-by: syzbot <[email protected]>
> Tested-by: syzbot <[email protected]>
> Signed-off-by: Tetsuo Handa <[email protected]>
> Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
> ---
> drivers/char/ttyprintk.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 6a0059e508e3..93f5d11c830b 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
> return 0;
> }
>
> +/*
> + * TTY operations hangup function.
> + */
> +static void tpk_hangup(struct tty_struct *tty)
> +{
> + struct ttyprintk_port *tpkp = tty->driver_data;
> +
> + tty_port_hangup(&tpkp->port);
> +}
> +
> static const struct tty_operations ttyprintk_ops = {
> .open = tpk_open,
> .close = tpk_close,
> .write = tpk_write,
> .write_room = tpk_write_room,
> .ioctl = tpk_ioctl,
> + .hangup = tpk_hangup,
> };
>
> static const struct tty_port_operations null_ops = { };
Using the supplied test code, i've tested the patch on my desktop running the
5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
tty_port_close_start: tty->count = 1 port count = 11" do not appear any more,
when the test code is run.
I think the patch is ok.
best regards, Samo
On Sun, Apr 18, 2021 at 01:16:05PM +0200, Samo Pogačnik wrote:
> Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
> > syzbot is reporting hung task due to flood of
> >
> > tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
> > port->count);
> >
> > message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
> > decrementing port->count due to tty_hung_up_p() == true.
> >
> > ----------
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <sys/ioctl.h>
> > #include <unistd.h>
> >
> > int main(int argc, char *argv[])
> > {
> > int i;
> > int fd[10];
> >
> > for (i = 0; i < 10; i++)
> > fd[i] = open("/dev/ttyprintk", O_WRONLY);
> > ioctl(fd[0], TIOCVHANGUP);
> > for (i = 0; i < 10; i++)
> > close(fd[i]);
> > close(open("/dev/ttyprintk", O_WRONLY));
> > return 0;
> > }
> > ----------
> >
> > When TTY hangup happens, port->count needs to be reset via
> > "struct tty_operations"->hangup callback.
> >
> > [1]
> > https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
> >
> > Reported-by: syzbot <[email protected]>
> > Reported-by: syzbot <[email protected]>
> > Tested-by: syzbot <[email protected]>
> > Signed-off-by: Tetsuo Handa <[email protected]>
> > Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
> > ---
> > drivers/char/ttyprintk.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > index 6a0059e508e3..93f5d11c830b 100644
> > --- a/drivers/char/ttyprintk.c
> > +++ b/drivers/char/ttyprintk.c
> > @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
> > return 0;
> > }
> >
> > +/*
> > + * TTY operations hangup function.
> > + */
> > +static void tpk_hangup(struct tty_struct *tty)
> > +{
> > + struct ttyprintk_port *tpkp = tty->driver_data;
> > +
> > + tty_port_hangup(&tpkp->port);
> > +}
> > +
> > static const struct tty_operations ttyprintk_ops = {
> > .open = tpk_open,
> > .close = tpk_close,
> > .write = tpk_write,
> > .write_room = tpk_write_room,
> > .ioctl = tpk_ioctl,
> > + .hangup = tpk_hangup,
> > };
> >
> > static const struct tty_port_operations null_ops = { };
>
> Using the supplied test code, i've tested the patch on my desktop running the
> 5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
> tty_port_close_start: tty->count = 1 port count = 11" do not appear any more,
> when the test code is run.
> I think the patch is ok.
Thanks for the review, I'll go queue this up.
greg k-h
On 18. 04. 21, 13:16, Samo Pogačnik wrote:
> Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
>> syzbot is reporting hung task due to flood of
>>
>> tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
>> port->count);
>>
>> message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
>> decrementing port->count due to tty_hung_up_p() == true.
>>
>> ----------
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <sys/ioctl.h>
>> #include <unistd.h>
>>
>> int main(int argc, char *argv[])
>> {
>> int i;
>> int fd[10];
>>
>> for (i = 0; i < 10; i++)
>> fd[i] = open("/dev/ttyprintk", O_WRONLY);
>> ioctl(fd[0], TIOCVHANGUP);
>> for (i = 0; i < 10; i++)
>> close(fd[i]);
>> close(open("/dev/ttyprintk", O_WRONLY));
>> return 0;
>> }
>> ----------
>>
>> When TTY hangup happens, port->count needs to be reset via
>> "struct tty_operations"->hangup callback.
>>
>> [1]
>> https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
>>
>> Reported-by: syzbot <[email protected]>
>> Reported-by: syzbot <[email protected]>
>> Tested-by: syzbot <[email protected]>
>> Signed-off-by: Tetsuo Handa <[email protected]>
>> Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
>> ---
>> drivers/char/ttyprintk.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
>> index 6a0059e508e3..93f5d11c830b 100644
>> --- a/drivers/char/ttyprintk.c
>> +++ b/drivers/char/ttyprintk.c
>> @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
>> return 0;
>> }
>>
>> +/*
>> + * TTY operations hangup function.
>> + */
>> +static void tpk_hangup(struct tty_struct *tty)
>> +{
>> + struct ttyprintk_port *tpkp = tty->driver_data;
>> +
>> + tty_port_hangup(&tpkp->port);
>> +}
>> +
>> static const struct tty_operations ttyprintk_ops = {
>> .open = tpk_open,
>> .close = tpk_close,
>> .write = tpk_write,
>> .write_room = tpk_write_room,
>> .ioctl = tpk_ioctl,
>> + .hangup = tpk_hangup,
>> };
>>
>> static const struct tty_port_operations null_ops = { };
>
> Using the supplied test code, i've tested the patch on my desktop running the
> 5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
> tty_port_close_start: tty->count = 1 port count = 11" do not appear any more,
> when the test code is run.
> I think the patch is ok.
I wonder if the buffer shouldn't be flushed in hangup too? Or better,
the flush moved from tty_ops->close to tty_port->ops->shutdown?
thanks,
--
js
suse labs
Dne 23.04.2021 (pet) ob 06:22 +0200 je Jiri Slaby napisal(a):
> On 18. 04. 21, 13:16, Samo Pogačnik wrote:
> > Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
> > > syzbot is reporting hung task due to flood of
> > >
> > > tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
> > > port->count);
> > >
> > > message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
> > > decrementing port->count due to tty_hung_up_p() == true.
> > >
> > > ----------
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > #include <fcntl.h>
> > > #include <sys/ioctl.h>
> > > #include <unistd.h>
> > >
> > > int main(int argc, char *argv[])
> > > {
> > > int i;
> > > int fd[10];
> > >
> > > for (i = 0; i < 10; i++)
> > > fd[i] = open("/dev/ttyprintk", O_WRONLY);
> > > ioctl(fd[0], TIOCVHANGUP);
> > > for (i = 0; i < 10; i++)
> > > close(fd[i]);
> > > close(open("/dev/ttyprintk", O_WRONLY));
> > > return 0;
> > > }
> > > ----------
> > >
> > > When TTY hangup happens, port->count needs to be reset via
> > > "struct tty_operations"->hangup callback.
> > >
> > > [1]
> > >
https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
> > >
> > > Reported-by: syzbot <[email protected]
> > > >
> > > Reported-by: syzbot <[email protected]
> > > >
> > > Tested-by: syzbot <[email protected]>
> > > Signed-off-by: Tetsuo Handa <[email protected]>
> > > Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
> > > ---
> > > drivers/char/ttyprintk.c | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > > index 6a0059e508e3..93f5d11c830b 100644
> > > --- a/drivers/char/ttyprintk.c
> > > +++ b/drivers/char/ttyprintk.c
> > > @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * TTY operations hangup function.
> > > + */
> > > +static void tpk_hangup(struct tty_struct *tty)
> > > +{
> > > + struct ttyprintk_port *tpkp = tty->driver_data;
> > > +
> > > + tty_port_hangup(&tpkp->port);
> > > +}
> > > +
> > > static const struct tty_operations ttyprintk_ops = {
> > > .open = tpk_open,
> > > .close = tpk_close,
> > > .write = tpk_write,
> > > .write_room = tpk_write_room,
> > > .ioctl = tpk_ioctl,
> > > + .hangup = tpk_hangup,
> > > };
> > >
> > > static const struct tty_port_operations null_ops = { };
> >
> > Using the supplied test code, i've tested the patch on my desktop running
> > the
> > 5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
> > tty_port_close_start: tty->count = 1 port count = 11" do not appear any
> > more,
> > when the test code is run.
> > I think the patch is ok.
>
> I wonder if the buffer shouldn't be flushed in hangup too? Or better,
> the flush moved from tty_ops->close to tty_port->ops->shutdown?
>
> thanks,
Good point. I tried the following additional change, which seems to do the
trick. What do you think?
thanks, Samo
---
drivers/char/ttyprintk.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 93f5d11c8..420222a92 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -100,12 +100,6 @@ static int tpk_open(struct tty_struct *tty, struct file
*filp)
static void tpk_close(struct tty_struct *tty, struct file *filp)
{
struct ttyprintk_port *tpkp = tty->driver_data;
- unsigned long flags;
-
- spin_lock_irqsave(&tpkp->spinlock, flags);
- /* flush tpk_printk buffer */
- tpk_printk(NULL, 0);
- spin_unlock_irqrestore(&tpkp->spinlock, flags);
tty_port_close(&tpkp->port, tty, filp);
}
@@ -168,6 +162,20 @@ static void tpk_hangup(struct tty_struct *tty)
tty_port_hangup(&tpkp->port);
}
+/*
+ * TTY port operations shutdown function.
+ */
+static void tpk_port_shutdown(struct tty_port *tport)
+{
+ struct ttyprintk_port *tpkp =
+ container_of(tport, struct ttyprintk_port, port);
+ unsigned long flags;
+
+ spin_lock_irqsave(&tpkp->spinlock, flags);
+ tpk_flush();
+ spin_unlock_irqrestore(&tpkp->spinlock, flags);
+}
+
static const struct tty_operations ttyprintk_ops = {
.open = tpk_open,
.close = tpk_close,
@@ -177,7 +185,9 @@ static const struct tty_operations ttyprintk_ops = {
.hangup = tpk_hangup,
};
-static const struct tty_port_operations null_ops = { };
+static const struct tty_port_operations tpk_port_ops = {
+ .shutdown = tpk_port_shutdown,
+};
static struct tty_driver *ttyprintk_driver;
@@ -195,7 +205,7 @@ static int __init ttyprintk_init(void)
return PTR_ERR(ttyprintk_driver);
tty_port_init(&tpk_port.port);
- tpk_port.port.ops = &null_ops;
+ tpk_port.port.ops = &tpk_port_ops;
ttyprintk_driver->driver_name = "ttyprintk";
ttyprintk_driver->name = "ttyprintk";
--
2.17.1
On 2021/04/23 18:55, Samo Pogačnik wrote:
>>> Using the supplied test code, i've tested the patch on my desktop running
>>> the
>>> 5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
>>> tty_port_close_start: tty->count = 1 port count = 11" do not appear any
>>> more,
>>> when the test code is run.
>>> I think the patch is ok.
>>
>> I wonder if the buffer shouldn't be flushed in hangup too? Or better,
>> the flush moved from tty_ops->close to tty_port->ops->shutdown?
>>
>> thanks,
>
> Good point. I tried the following additional change, which seems to do the
> trick. What do you think?
>
Shouldn't the tpk_printk buffer be per a "struct file" (i.e. allocated upon
open() and released upon close() in order to allow multiple users) ?
On 23. 04. 21, 11:55, Samo Pogačnik wrote:
> Dne 23.04.2021 (pet) ob 06:22 +0200 je Jiri Slaby napisal(a):
>> On 18. 04. 21, 13:16, Samo Pogačnik wrote:
>>> Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
>>>> syzbot is reporting hung task due to flood of
>>>>
>>>> tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
>>>> port->count);
>>>>
>>>> message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
>>>> decrementing port->count due to tty_hung_up_p() == true.
>>>>
>>>> ----------
>>>> #include <sys/types.h>
>>>> #include <sys/stat.h>
>>>> #include <fcntl.h>
>>>> #include <sys/ioctl.h>
>>>> #include <unistd.h>
>>>>
>>>> int main(int argc, char *argv[])
>>>> {
>>>> int i;
>>>> int fd[10];
>>>>
>>>> for (i = 0; i < 10; i++)
>>>> fd[i] = open("/dev/ttyprintk", O_WRONLY);
>>>> ioctl(fd[0], TIOCVHANGUP);
>>>> for (i = 0; i < 10; i++)
>>>> close(fd[i]);
>>>> close(open("/dev/ttyprintk", O_WRONLY));
>>>> return 0;
>>>> }
>>>> ----------
>>>>
>>>> When TTY hangup happens, port->count needs to be reset via
>>>> "struct tty_operations"->hangup callback.
>>>>
>>>> [1]
>>>>
> https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
>>>>
>>>> Reported-by: syzbot <[email protected]
>>>>>
>>>> Reported-by: syzbot <[email protected]
>>>>>
>>>> Tested-by: syzbot <[email protected]>
>>>> Signed-off-by: Tetsuo Handa <[email protected]>
>>>> Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
>>>> ---
>>>> drivers/char/ttyprintk.c | 11 +++++++++++
>>>> 1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
>>>> index 6a0059e508e3..93f5d11c830b 100644
>>>> --- a/drivers/char/ttyprintk.c
>>>> +++ b/drivers/char/ttyprintk.c
>>>> @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
>>>> return 0;
>>>> }
>>>>
>>>> +/*
>>>> + * TTY operations hangup function.
>>>> + */
>>>> +static void tpk_hangup(struct tty_struct *tty)
>>>> +{
>>>> + struct ttyprintk_port *tpkp = tty->driver_data;
>>>> +
>>>> + tty_port_hangup(&tpkp->port);
>>>> +}
>>>> +
>>>> static const struct tty_operations ttyprintk_ops = {
>>>> .open = tpk_open,
>>>> .close = tpk_close,
>>>> .write = tpk_write,
>>>> .write_room = tpk_write_room,
>>>> .ioctl = tpk_ioctl,
>>>> + .hangup = tpk_hangup,
>>>> };
>>>>
>>>> static const struct tty_port_operations null_ops = { };
>>>
>>> Using the supplied test code, i've tested the patch on my desktop running
>>> the
>>> 5.4 kernel. After applying the patch, the kernel warnings like "ttyprintk:
>>> tty_port_close_start: tty->count = 1 port count = 11" do not appear any
>>> more,
>>> when the test code is run.
>>> I think the patch is ok.
>>
>> I wonder if the buffer shouldn't be flushed in hangup too? Or better,
>> the flush moved from tty_ops->close to tty_port->ops->shutdown?
>>
>> thanks,
>
> Good point. I tried the following additional change, which seems to do the
> trick. What do you think?
>
> thanks, Samo
> ---
> drivers/char/ttyprintk.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 93f5d11c8..420222a92 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -100,12 +100,6 @@ static int tpk_open(struct tty_struct *tty, struct file
> *filp)
> static void tpk_close(struct tty_struct *tty, struct file *filp)
> {
> struct ttyprintk_port *tpkp = tty->driver_data;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&tpkp->spinlock, flags);
> - /* flush tpk_printk buffer */
> - tpk_printk(NULL, 0);
And now, you can drop NULL buf handling from tpk_printk, right?
> - spin_unlock_irqrestore(&tpkp->spinlock, flags);
>
> tty_port_close(&tpkp->port, tty, filp);
--
js
suse labs
Dne 23.04.2021 (pet) ob 12:28 +0200 je Jiri Slaby napisal(a):
> On 23. 04. 21, 11:55, Samo Pogačnik wrote:
> > Dne 23.04.2021 (pet) ob 06:22 +0200 je Jiri Slaby napisal(a):
> > > On 18. 04. 21, 13:16, Samo Pogačnik wrote:
> > > > Dne 15.04.2021 (čet) ob 09:22 +0900 je Tetsuo Handa napisal(a):
> > > > > syzbot is reporting hung task due to flood of
> > > > >
> > > > > tty_warn(tty, "%s: tty->count = 1 port count = %d\n", __func__,
> > > > > port->count);
> > > > >
> > > > > message [1], for ioctl(TIOCVHANGUP) prevents tty_port_close() from
> > > > > decrementing port->count due to tty_hung_up_p() == true.
> > > > >
> > > > > ----------
> > > > > #include <sys/types.h>
> > > > > #include <sys/stat.h>
> > > > > #include <fcntl.h>
> > > > > #include <sys/ioctl.h>
> > > > > #include <unistd.h>
> > > > >
> > > > > int main(int argc, char *argv[])
> > > > > {
> > > > > int i;
> > > > > int fd[10];
> > > > >
> > > > > for (i = 0; i < 10; i++)
> > > > > fd[i] = open("/dev/ttyprintk", O_WRONLY);
> > > > > ioctl(fd[0], TIOCVHANGUP);
> > > > > for (i = 0; i < 10; i++)
> > > > > close(fd[i]);
> > > > > close(open("/dev/ttyprintk", O_WRONLY));
> > > > > return 0;
> > > > > }
> > > > > ----------
> > > > >
> > > > > When TTY hangup happens, port->count needs to be reset via
> > > > > "struct tty_operations"->hangup callback.
> > > > >
> > > > > [1]
> > > > >
> >
> >
https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
> > > > >
> > > > > Reported-by: syzbot <
> > > > > [email protected]
> > > > > >
> > > > >
> > > > > Reported-by: syzbot <
> > > > > [email protected]
> > > > > >
> > > > >
> > > > > Tested-by: syzbot <
> > > > > [email protected]>
> > > > > Signed-off-by: Tetsuo Handa <[email protected]>
> > > > > Fixes: 24b4b67d17c308aa ("add ttyprintk driver")
> > > > > ---
> > > > > drivers/char/ttyprintk.c | 11 +++++++++++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > > > > index 6a0059e508e3..93f5d11c830b 100644
> > > > > --- a/drivers/char/ttyprintk.c
> > > > > +++ b/drivers/char/ttyprintk.c
> > > > > @@ -158,12 +158,23 @@ static int tpk_ioctl(struct tty_struct *tty,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * TTY operations hangup function.
> > > > > + */
> > > > > +static void tpk_hangup(struct tty_struct *tty)
> > > > > +{
> > > > > + struct ttyprintk_port *tpkp = tty->driver_data;
> > > > > +
> > > > > + tty_port_hangup(&tpkp->port);
> > > > > +}
> > > > > +
> > > > > static const struct tty_operations ttyprintk_ops = {
> > > > > .open = tpk_open,
> > > > > .close = tpk_close,
> > > > > .write = tpk_write,
> > > > > .write_room = tpk_write_room,
> > > > > .ioctl = tpk_ioctl,
> > > > > + .hangup = tpk_hangup,
> > > > > };
> > > > >
> > > > > static const struct tty_port_operations null_ops = { };
> > > >
> > > > Using the supplied test code, i've tested the patch on my desktop
> > > > running
> > > > the
> > > > 5.4 kernel. After applying the patch, the kernel warnings like
> > > > "ttyprintk:
> > > > tty_port_close_start: tty->count = 1 port count = 11" do not appear any
> > > > more,
> > > > when the test code is run.
> > > > I think the patch is ok.
> > >
> > > I wonder if the buffer shouldn't be flushed in hangup too? Or better,
> > > the flush moved from tty_ops->close to tty_port->ops->shutdown?
> > >
> > > thanks,
> >
> > Good point. I tried the following additional change, which seems to do the
> > trick. What do you think?
> >
> > thanks, Samo
> > ---
> > drivers/char/ttyprintk.c | 26 ++++++++++++++++++--------
> > 1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> > index 93f5d11c8..420222a92 100644
> > --- a/drivers/char/ttyprintk.c
> > +++ b/drivers/char/ttyprintk.c
> > @@ -100,12 +100,6 @@ static int tpk_open(struct tty_struct *tty, struct file
> > *filp)
> > static void tpk_close(struct tty_struct *tty, struct file *filp)
> > {
> > struct ttyprintk_port *tpkp = tty->driver_data;
> > - unsigned long flags;
> > -
> > - spin_lock_irqsave(&tpkp->spinlock, flags);
> > - /* flush tpk_printk buffer */
> > - tpk_printk(NULL, 0);
>
> And now, you can drop NULL buf handling from tpk_printk, right?
Exactly!
thanks, Samo
---
drivers/char/ttyprintk.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
index 93f5d11c8..6f616cb7c 100644
--- a/drivers/char/ttyprintk.c
+++ b/drivers/char/ttyprintk.c
@@ -54,11 +54,6 @@ static int tpk_printk(const unsigned char *buf, int count)
{
int i = tpk_curr;
- if (buf == NULL) {
- tpk_flush();
- return i;
- }
-
for (i = 0; i < count; i++) {
if (tpk_curr >= TPK_STR_SIZE) {
/* end of tmp buffer reached: cut the message in two */
@@ -100,12 +95,6 @@ static int tpk_open(struct tty_struct *tty, struct file
*filp)
static void tpk_close(struct tty_struct *tty, struct file *filp)
{
struct ttyprintk_port *tpkp = tty->driver_data;
- unsigned long flags;
-
- spin_lock_irqsave(&tpkp->spinlock, flags);
- /* flush tpk_printk buffer */
- tpk_printk(NULL, 0);
- spin_unlock_irqrestore(&tpkp->spinlock, flags);
tty_port_close(&tpkp->port, tty, filp);
}
@@ -168,6 +157,20 @@ static void tpk_hangup(struct tty_struct *tty)
tty_port_hangup(&tpkp->port);
}
+/*
+ * TTY port operations shutdown function.
+ */
+static void tpk_port_shutdown(struct tty_port *tport)
+{
+ struct ttyprintk_port *tpkp =
+ container_of(tport, struct ttyprintk_port, port);
+ unsigned long flags;
+
+ spin_lock_irqsave(&tpkp->spinlock, flags);
+ tpk_flush();
+ spin_unlock_irqrestore(&tpkp->spinlock, flags);
+}
+
static const struct tty_operations ttyprintk_ops = {
.open = tpk_open,
.close = tpk_close,
@@ -177,7 +180,9 @@ static const struct tty_operations ttyprintk_ops = {
.hangup = tpk_hangup,
};
-static const struct tty_port_operations null_ops = { };
+static const struct tty_port_operations tpk_port_ops = {
+ .shutdown = tpk_port_shutdown,
+};
static struct tty_driver *ttyprintk_driver;
@@ -195,7 +200,7 @@ static int __init ttyprintk_init(void)
return PTR_ERR(ttyprintk_driver);
tty_port_init(&tpk_port.port);
- tpk_port.port.ops = &null_ops;
+ tpk_port.port.ops = &tpk_port_ops;
ttyprintk_driver->driver_name = "ttyprintk";
ttyprintk_driver->name = "ttyprintk";
--
2.17.1
Dne 23.04.2021 (pet) ob 19:12 +0900 je Tetsuo Handa napisal(a):
> On 2021/04/23 18:55, Samo Pogačnik wrote:
> > > > Using the supplied test code, i've tested the patch on my desktop
> > > > running
> > > > the
> > > > 5.4 kernel. After applying the patch, the kernel warnings like
> > > > "ttyprintk:
> > > > tty_port_close_start: tty->count = 1 port count = 11" do not appear any
> > > > more,
> > > > when the test code is run.
> > > > I think the patch is ok.
> > >
> > > I wonder if the buffer shouldn't be flushed in hangup too? Or better,
> > > the flush moved from tty_ops->close to tty_port->ops->shutdown?
> > >
> > > thanks,
> >
> > Good point. I tried the following additional change, which seems to do the
> > trick. What do you think?
> >
>
> Shouldn't the tpk_printk buffer be per a "struct file" (i.e. allocated upon
> open() and released upon close() in order to allow multiple users) ?
Final destination of the ttyprintk is printk(), which is a single destination.
The tpk_printk buffer is a common representation of what is yet to be printk-ed
depending on the formatting conditions within the buffer.
At any point the tpk_buffer is potentially multiplexed/interleaved by parts of
required output of any concurrent user, as buffs are being delivered by the
scheduled writes.
As per user buffers look promising with output formatting, the FDs passing
between tasks lead to the same single buffer (Greg already mentioned that). The
other thing which is important to me is the console redirection to ttyprintk,
which automatically brings all concurrent console users to the single open of
ttyprintk.
best regards, Samo
On 2021/04/24 4:47, Samo Pogačnik wrote:
> At any point the tpk_buffer is potentially multiplexed/interleaved by parts of
> required output of any concurrent user, as buffs are being delivered by the
> scheduled writes.
As long as one line is printed by one printk() call, CONFIG_PRINTK_CALLER=y is
helpful enough to distinguish multilplexed/interleaved messages. I consider that
ttyprintk offers additional advantage over printk() for allow buffering one line
of message from userspace.
>
> As per user buffers look promising with output formatting, the FDs passing
> between tasks lead to the same single buffer (Greg already mentioned that).
Those programs which use FD passing know what they are doing. If they still want
one line of message printed via ttyprintk interface, they must do their buffering
before trying to write() to ttyprintk's file descriptor.
Use of per "struct file" buffer gives those programs which does not use
FD passing a guarantee that one line of message from those programs won't be
multilplexed/interleaved (with the aid of CONFIG_PRINTK_CALLER=y).
I consider it an improvement.
> The
> other thing which is important to me is the console redirection to ttyprintk,
> which automatically brings all concurrent console users to the single open of
> ttyprintk.
Gathering whole console output into one FD is similar to FD passing; that is
inevitable. But use of per "struct file" buffer allows /dev/ttyprintk users
from outside of "gather whole console output into one FD" case to avoid
multilplexed/interleaved messages. I tried /dev/ttyprintk like
https://lkml.kernel.org/r/[email protected] .
Dne 24.04.2021 (sob) ob 10:16 +0900 je Tetsuo Handa napisal(a):
> On 2021/04/24 4:47, Samo Pogačnik wrote:
> > At any point the tpk_buffer is potentially multiplexed/interleaved by parts
> > of
> > required output of any concurrent user, as buffs are being delivered by the
> > scheduled writes.
>
> As long as one line is printed by one printk() call, CONFIG_PRINTK_CALLER=y is
> helpful enough to distinguish multilplexed/interleaved messages. I consider
> that
> ttyprintk offers additional advantage over printk() for allow buffering one
> line
> of message from userspace.
>
> >
> > As per user buffers look promising with output formatting, the FDs passing
> > between tasks lead to the same single buffer (Greg already mentioned that).
>
> Those programs which use FD passing know what they are doing. If they still
> want
> one line of message printed via ttyprintk interface, they must do their
> buffering
> before trying to write() to ttyprintk's file descriptor.
>
> Use of per "struct file" buffer gives those programs which does not use
> FD passing a guarantee that one line of message from those programs won't be
> multilplexed/interleaved (with the aid of CONFIG_PRINTK_CALLER=y).
> I consider it an improvement.
>
> >
> > The
> > other thing which is important to me is the console redirection to
> > ttyprintk,
> > which automatically brings all concurrent console users to the single open
> > of
> > ttyprintk.
>
> Gathering whole console output into one FD is similar to FD passing; that is
> inevitable. But use of per "struct file" buffer allows /dev/ttyprintk users
> from outside of "gather whole console output into one FD" case to avoid
> multilplexed/interleaved messages. I tried /dev/ttyprintk like
>
https://lkml.kernel.org/r/[email protected]
> .
>
I think i understand, what would you like to achieve, however implementation
wise there seems to be no reference point available in tty write operation that
would relate to its tty open/close operations (i.e. open() and close() provide
filp while write() does not - probably for a reason) or is there such a relation
available?
On the other hand, my main concern is how to provide a reliable system wide
collection of all console output via ttyprintk console redirection, while normal
operation of system console is preserved (except its output being detoured via
printk and as such logged together with kernel output). Such logging is
particularly useful for after-the-fact inspection of system operation.
That being said i am thinking about how to permanently enable this redirection
as early as possible (i.e. via kernel command line option). I had a working
prototype for that some time ago (never posted). Would anybody like to see such
functionality?
best regards, Samo
On Sat 2021-04-24 11:57:47, Samo Pogačnik wrote:
> Dne 24.04.2021 (sob) ob 10:16 +0900 je Tetsuo Handa napisal(a):
> > On 2021/04/24 4:47, Samo Pogačnik wrote:
> > > At any point the tpk_buffer is potentially multiplexed/interleaved by parts
> > > of
> > > required output of any concurrent user, as buffs are being delivered by the
> > > scheduled writes.
> >
> > As long as one line is printed by one printk() call, CONFIG_PRINTK_CALLER=y is
> > helpful enough to distinguish multilplexed/interleaved messages. I consider
> > that
> > ttyprintk offers additional advantage over printk() for allow buffering one
> > line
> > of message from userspace.
It does not matter how much buffering games you play. As long as you
use printk() to store single lines into the kernel logbuffer they
alway could be interleaved with lines from other processes/CPUs.
> > >
> > > As per user buffers look promising with output formatting, the FDs passing
> > > between tasks lead to the same single buffer (Greg already mentioned that).
> >
> > Those programs which use FD passing know what they are doing. If they still
> > want
> > one line of message printed via ttyprintk interface, they must do their
> > buffering
> > before trying to write() to ttyprintk's file descriptor.
Lines might get interleaved when using printk().
What is special about messages passed via ttyprintk()?
How many processes are using it?
Do they print many lines?
Is it really worth any added complexity?
> > Use of per "struct file" buffer gives those programs which does not use
> > FD passing a guarantee that one line of message from those programs won't be
> > multilplexed/interleaved (with the aid of CONFIG_PRINTK_CALLER=y).
How huge bugffer would be needed?
IMHO, even a 80-bytes big per-task buffer is not acceptable. And even
such a buffer would hold only 1-3 lines.
> I think i understand, what would you like to achieve, however implementation
> wise there seems to be no reference point available in tty write operation that
> would relate to its tty open/close operations (i.e. open() and close() provide
> filp while write() does not - probably for a reason) or is there such a relation
> available?
<
> On the other hand, my main concern is how to provide a reliable system wide
> collection of all console output via ttyprintk console redirection, while normal
> operation of system console is preserved (except its output being detoured via
> printk and as such logged together with kernel output). Such logging is
> particularly useful for after-the-fact inspection of system operation.
I am not sure if I understand the problem. But why does ttyprintk need
any buffer at all. AFAIK, the use-case is to pass any written data into the
kernel logbuffer via printk()?
Why tpk_write() does not call printk() directly?
If you call printk() directly, the caller_id would be from the process
that really wrote the data/message.
> That being said i am thinking about how to permanently enable this redirection
> as early as possible (i.e. via kernel command line option). I had a working
> prototype for that some time ago (never posted). Would anybody like to see such
> functionality?
Please, do not add any complex code if it does not cause real life
problems.
There seems to be only few commits that suggest that anyone is using
this driver and the latest is 7 year old.
+ (2014) acef6660d3aaf18813143
("ttyprintk: make the printk log level configurable")
+ (2014) b24313a82cf24e9017067
("ttyprintk: Allow built as a module")
+ (2014) 7d1c2858c49095ab748f5
("ttyprintk: Fix wrong tty_unregister_driver() call in the error path")
+ (2014) db50d2f65b7c2bcdfb931
("drivers/char: don't use module_init in non-modular ttyprintk.c")
+ (2013) b5325a02aa84c794cf520
("ttyprintk: Fix NULL pointer deref by setting tty_port ops
after initializing port")
+ (2012) ee8b593affdf893012e57
("TTY: ttyprintk, don't touch behind tty->write_buf")
+ (2012) f06fb543c1d0cbd721250
("TTY: ttyprintk, unregister tty driver on failure")
+ (2012) 2f16669d322e05171c9e1
("TTY: remove re-assignments to tty_driver members")
+ (2010) 24b4b67d17c308aaa956b
("add ttyprintk driver")
Best Regards,
Petr
Dne 26.04.2021 (pon) ob 12:00 +0200 je Petr Mladek napisal(a):
> On Sat 2021-04-24 11:57:47, Samo Pogačnik wrote:
> > Dne 24.04.2021 (sob) ob 10:16 +0900 je Tetsuo Handa napisal(a):
> > > On 2021/04/24 4:47, Samo Pogačnik wrote:
> > > > At any point the tpk_buffer is potentially multiplexed/interleaved by
> > > > parts
> > > > of
> > > > required output of any concurrent user, as buffs are being delivered by
> > > > the
> > > > scheduled writes.
> > >
> > > As long as one line is printed by one printk() call,
> > > CONFIG_PRINTK_CALLER=y is
> > > helpful enough to distinguish multilplexed/interleaved messages. I
> > > consider
> > > that
> > > ttyprintk offers additional advantage over printk() for allow buffering
> > > one
> > > line
> > > of message from userspace.
>
> It does not matter how much buffering games you play. As long as you
> use printk() to store single lines into the kernel logbuffer they
> alway could be interleaved with lines from other processes/CPUs.
Exactly. The only purpose of ttyprintk buffering is to mark any begining of
lines occurring within the userspace-string written into ttyprintk TTY. The
marked lines do not originate in the kernel source code, which is not obvious
otherwise (imho this is importannt). Even the CONFIG_PRINTK_CALLER=y does not
give this information, if the task ID printed does not live anymore.
>
> > > >
> > > > As per user buffers look promising with output formatting, the FDs
> > > > passing
> > > > between tasks lead to the same single buffer (Greg already mentioned
> > > > that).
> > >
> > > Those programs which use FD passing know what they are doing. If they
> > > still
> > > want
> > > one line of message printed via ttyprintk interface, they must do their
> > > buffering
> > > before trying to write() to ttyprintk's file descriptor.
>
> Lines might get interleaved when using printk().
> What is special about messages passed via ttyprintk()?
They do not originate in the kernel code.
> How many processes are using it?
In case of redirection any proces, that is writing to console.
> Do they print many lines?
?
> Is it really worth any added complexity?
No.
>
> > On the other hand, my main concern is how to provide a reliable system wide
> > collection of all console output via ttyprintk console redirection, while
> > normal
> > operation of system console is preserved (except its output being detoured
> > via
> > printk and as such logged together with kernel output). Such logging is
> > particularly useful for after-the-fact inspection of system operation.
>
> I am not sure if I understand the problem. But why does ttyprintk need
> any buffer at all. AFAIK, the use-case is to pass any written data into the
> kernel logbuffer via printk()?
(see above - it is not something the kernel is telling you)
>
> Why tpk_write() does not call printk() directly?
(see above)
>
> If you call printk() directly, the caller_id would be from the process
> that really wrote the data/message.
It can be a kernel-code originating message printk-ed on behalf of a user task
or a kernel-code originating message on behalf of a kernel task. Or it may be a
user-code originating message on behalf of its task, when printk-ed via
ttyprintk.
>
> > That being said i am thinking about how to permanently enable this
> > redirection
> > as early as possible (i.e. via kernel command line option). I had a working
> > prototype for that some time ago (never posted). Would anybody like to see
> > such
> > functionality?
>
> Please, do not add any complex code if it does not cause real life
> problems.
>
Noted, thanks.
Best regards, Samo
On Mon 2021-04-26 18:42:05, Samo Pogačnik wrote:
> Dne 26.04.2021 (pon) ob 12:00 +0200 je Petr Mladek napisal(a):
> > It does not matter how much buffering games you play. As long as you
> > use printk() to store single lines into the kernel logbuffer they
> > alway could be interleaved with lines from other processes/CPUs.
>
> Exactly. The only purpose of ttyprintk buffering is to mark any begining of
> lines occurring within the userspace-string written into ttyprintk TTY. The
> marked lines do not originate in the kernel source code, which is not obvious
> otherwise (imho this is importannt). Even the CONFIG_PRINTK_CALLER=y does not
> give this information, if the task ID printed does not live anymore.
I guess that you mean TPK_PREFIX + "[U] ".
> > I am not sure if I understand the problem. But why does ttyprintk need
> > any buffer at all. AFAIK, the use-case is to pass any written data into the
> > kernel logbuffer via printk()?
> (see above - it is not something the kernel is telling you)
But you could do this already in tpk_write(). I mean that you could
parse the given buffer, copy each line to a temporary buffer,
and call printk(TPK_PREFIX "[U] %s\n", tmp_buf).
Why is it postponed to tpk_close()?
IMHO, the printk() in tpk_write() might simplify the logic a bit.
> > Why tpk_write() does not call printk() directly?
> (see above)
> >
> > If you call printk() directly, the caller_id would be from the process
> > that really wrote the data/message.
> It can be a kernel-code originating message printk-ed on behalf of a user task
> or a kernel-code originating message on behalf of a kernel task. Or it may be a
> user-code originating message on behalf of its task, when printk-ed via
> ttyprintk.
Exactly. Now, I am not sure if you think that this good or bad.
IMHO, it is much better to use caller_id of the process/context that
wrote the data/message instead of the process that caused the final
tpk_close().
Anyway, it is not a big deal. We could leave the code as is if it
works for you. My mine intention was to stop the ideas of per-task
buffers and additional complexity. It was just an idea how to
simplify the code instead.
Best Regards,
Petr
Dne 27.04.2021 (tor) ob 12:08 +0200 je Petr Mladek napisal(a):
>
> I guess that you mean TPK_PREFIX + "[U] ".
yes
>
> But you could do this already in tpk_write(). I mean that you could
> parse the given buffer, copy each line to a temporary buffer,
> and call printk(TPK_PREFIX "[U] %s\n", tmp_buf).
>
> Why is it postponed to tpk_close()?
>
> IMHO, the printk() in tpk_write() might simplify the logic a bit.
The string received in tpk_write() has no guaranties, that it represents a
complete output line. It has to be treated as a sub-string of a potentially
multi-line massage produced by the userspace code/process.
The tpk_close() only produces additional output (flush), if the last tpk_write()
string does not end with some end-of-line indication.
>
>
> > >
> > > If you call printk() directly, the caller_id would be from the process
> > > that really wrote the data/message.
> >
> > It can be a kernel-code originating message printk-ed on behalf of a user
> > task
> > or a kernel-code originating message on behalf of a kernel task. Or it may
> > be a
> > user-code originating message on behalf of its task, when printk-ed via
> > ttyprintk.
>
> Exactly. Now, I am not sure if you think that this good or bad.
>
> IMHO, it is much better to use caller_id of the process/context that
> wrote the data/message instead of the process that caused the final
> tpk_close().
>
IMHO, it is good that output provides info about all the above cases and
especially that particular output is not produced by the kernel code itself.
best regards, samo