From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
To: Jiri Slaby <jslaby@suse.cz>, Paul Fulghum <paulkf@microgate.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
gregkh@linuxfoundation.org, Alan Cox <alan@lxorguk.ukuu.org.uk>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning
Date: Thu, 3 Jan 2019 20:32:29 +0900 [thread overview]
Message-ID: <98eec651-d1ab-48e5-309b-7e748981cfdd@i-love.sakura.ne.jp> (raw)
In-Reply-To: <ba6de468-d36d-cd33-9a13-520102b676c6@suse.cz>
On 2019/01/03 18:09, Jiri Slaby wrote:
> On 02. 01. 19, 16:04, Tetsuo Handa wrote:
>> + if (wait_event_interruptible(tty->read_wait,
>> + (ret = -EIO, test_bit(TTY_OTHER_CLOSED, &tty->flags)) ||
>> + (ret = 0, tty_hung_up_p(file)) ||
>> + (rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list)) != NULL ||
>> + (ret = -EAGAIN, tty_io_nonblock(tty, file))))
>> + return -EINTR;
>
> Oh, that looks really ugly. Could you move all this to a function
> returning a bool and taking &ret and &rbuf as parameters?
>
OK. Something like this?
From 725c55be437b6ce3b578a045cc7ddeeb2bbeb4b3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 3 Jan 2019 20:29:49 +0900
Subject: [PATCH] tty/n_hdlc: Use wait_event_interruptible() and same sanity
checks.
We can use wait_event_interruptible() in order to make it easier to
understand. Also, since the reason of using different level/frequency of
sanity checks for read and write is unclear while nowadays we have rich
fuzzing/sanitizing tools, let's use same sanity checks for read and write.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/tty/n_hdlc.c | 154 ++++++++++++++++++++-------------------------------
1 file changed, 61 insertions(+), 93 deletions(-)
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 8223d02..c497ef1 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -548,6 +548,27 @@ static void n_hdlc_tty_receive(struct tty_struct *tty, const __u8 *data,
} /* end of n_hdlc_tty_receive() */
+static bool __n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
+ struct n_hdlc *n_hdlc, int *ret,
+ struct n_hdlc_buf **rbuf)
+{
+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
+ *ret = -EIO;
+ return true;
+ }
+ if (tty_hung_up_p(file))
+ return true;
+ *rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
+ if (*rbuf)
+ return true;
+ /* no data */
+ if (tty_io_nonblock(tty, file)) {
+ *ret = -EAGAIN;
+ return true;
+ }
+ return false;
+}
+
/**
* n_hdlc_tty_read - Called to retrieve one frame of data (if available)
* @tty - pointer to tty instance data
@@ -562,14 +583,13 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
{
struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
int ret = 0;
- struct n_hdlc_buf *rbuf;
- DECLARE_WAITQUEUE(wait, current);
+ struct n_hdlc_buf *rbuf = NULL;
if (debuglevel >= DEBUG_LEVEL_INFO)
printk("%s(%d)n_hdlc_tty_read() called\n",__FILE__,__LINE__);
/* Validate the pointers */
- if (!n_hdlc)
+ if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC)
return -EIO;
/* verify user access to buffer */
@@ -579,60 +599,41 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
return -EFAULT;
}
- add_wait_queue(&tty->read_wait, &wait);
-
- for (;;) {
- if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
- ret = -EIO;
- break;
- }
- if (tty_hung_up_p(file))
- break;
-
- set_current_state(TASK_INTERRUPTIBLE);
-
- rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
- if (rbuf) {
- if (rbuf->count > nr) {
- /* too large for caller's buffer */
- ret = -EOVERFLOW;
- } else {
- __set_current_state(TASK_RUNNING);
- if (copy_to_user(buf, rbuf->buf, rbuf->count))
- ret = -EFAULT;
- else
- ret = rbuf->count;
- }
-
- if (n_hdlc->rx_free_buf_list.count >
- DEFAULT_RX_BUF_COUNT)
- kfree(rbuf);
- else
- n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
- break;
- }
-
- /* no data */
- if (tty_io_nonblock(tty, file)) {
- ret = -EAGAIN;
- break;
- }
-
- schedule();
-
- if (signal_pending(current)) {
- ret = -EINTR;
- break;
- }
+ if (wait_event_interruptible(tty->read_wait,
+ __n_hdlc_tty_read(tty, file, n_hdlc, &ret,
+ &rbuf)))
+ return -EINTR;
+ if (rbuf) {
+ if (rbuf->count > nr)
+ /* too large for caller's buffer */
+ ret = -EOVERFLOW;
+ else if (copy_to_user(buf, rbuf->buf, rbuf->count))
+ ret = -EFAULT;
+ else
+ ret = rbuf->count;
+ if (n_hdlc->rx_free_buf_list.count > DEFAULT_RX_BUF_COUNT)
+ kfree(rbuf);
+ else
+ n_hdlc_buf_put(&n_hdlc->rx_free_buf_list, rbuf);
}
-
- remove_wait_queue(&tty->read_wait, &wait);
- __set_current_state(TASK_RUNNING);
-
return ret;
} /* end of n_hdlc_tty_read() */
+static bool __n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
+ struct n_hdlc *n_hdlc, int *error,
+ struct n_hdlc_buf **tbuf)
+{
+ *tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
+ if (*tbuf)
+ return true;
+ if (tty_io_nonblock(tty, file)) {
+ *error = -EAGAIN;
+ return true;
+ }
+ return false;
+}
+
/**
* n_hdlc_tty_write - write a single frame of data to device
* @tty - pointer to associated tty device instance data
@@ -645,20 +646,16 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
const unsigned char *data, size_t count)
{
- struct n_hdlc *n_hdlc = tty2n_hdlc (tty);
+ struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
int error = 0;
- DECLARE_WAITQUEUE(wait, current);
struct n_hdlc_buf *tbuf;
if (debuglevel >= DEBUG_LEVEL_INFO)
printk("%s(%d)n_hdlc_tty_write() called count=%zd\n",
__FILE__,__LINE__,count);
-
- /* Verify pointers */
- if (!n_hdlc)
- return -EIO;
- if (n_hdlc->magic != HDLC_MAGIC)
+ /* Validate the pointers */
+ if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC)
return -EIO;
/* verify frame size */
@@ -670,40 +667,12 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
maxframe );
count = maxframe;
}
-
- add_wait_queue(&tty->write_wait, &wait);
-
- for (;;) {
- set_current_state(TASK_INTERRUPTIBLE);
-
- tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
- if (tbuf)
- break;
-
- if (tty_io_nonblock(tty, file)) {
- error = -EAGAIN;
- break;
- }
- schedule();
-
- n_hdlc = tty2n_hdlc (tty);
- if (!n_hdlc || n_hdlc->magic != HDLC_MAGIC ||
- tty != n_hdlc->tty) {
- printk("n_hdlc_tty_write: %p invalid after wait!\n", n_hdlc);
- error = -EIO;
- break;
- }
-
- if (signal_pending(current)) {
- error = -EINTR;
- break;
- }
- }
-
- __set_current_state(TASK_RUNNING);
- remove_wait_queue(&tty->write_wait, &wait);
- if (!error) {
+ if (wait_event_interruptible(tty->write_wait,
+ __n_hdlc_tty_write(tty, file, n_hdlc,
+ &error, &tbuf)))
+ return -EINTR;
+ if (tbuf) {
/* Retrieve the user's buffer */
memcpy(tbuf->buf, data, count);
@@ -712,7 +681,6 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
n_hdlc_buf_put(&n_hdlc->tx_buf_list,tbuf);
n_hdlc_send_frames(n_hdlc,tty);
}
-
return error;
} /* end of n_hdlc_tty_write() */
--
1.8.3.1
next prev parent reply other threads:[~2019-01-03 11:32 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-29 0:41 WARNING in __might_sleep (2) syzbot
2018-12-29 11:48 ` [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning Tetsuo Handa
2019-01-01 3:13 ` Paul Fulghum
2019-01-01 20:28 ` [PATCH] tty/n_hdlc: fix __might_sleep warning Paul Fulghum
2019-01-10 11:38 ` Tetsuo Handa
2019-01-10 12:25 ` Arnd Bergmann
[not found] ` <FEBFE826-8D27-4A0B-86A5-BA559921CADC@microgate.com>
2019-01-02 15:04 ` [PATCH] tty/n_hdlc: fix sleep in !TASK_RUNNING state warning Tetsuo Handa
2019-01-02 20:55 ` Paul Fulghum
2019-01-03 9:09 ` Jiri Slaby
2019-01-03 11:32 ` Tetsuo Handa [this message]
2019-01-03 15:57 ` Paul Fulghum
2019-01-04 10:23 ` Tetsuo Handa
2019-01-04 13:57 ` Paul Fulghum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98eec651-d1ab-48e5-309b-7e748981cfdd@i-love.sakura.ne.jp \
--to=penguin-kernel@i-love.sakura.ne.jp \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=jslaby@suse.cz \
--cc=linux-kernel@vger.kernel.org \
--cc=paulkf@microgate.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).