n_tty: Clarify copy_from_read_buf()

PATCH New Peter Hurley 1 participant 6 messages Nov 8, 2015 – Nov 27, 2015 TTY layer and serial drivers

lore

Share & tools

lore

Peter Hurley

author

2015-11-08 14:35 UTC #0 lore raw

PATCH 1/1 n_tty: Clarify copy_from_read_buf()

Add a temporary for the computed source address and substitute where appropriate. No functional change.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/tty/n_tty.c +4 -4

index b2b01d5..bc613b8 100644
@@ -2014,11 +2014,11 @@ static int copy_from_read_buf(struct tty_struct *tty,
 	n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail);
 	n = min(*nr, n);
 	if (n) {
-		retval = copy_to_user(*b, read_buf_addr(ldata, tail), n);
+		const unsigned char *from = read_buf_addr(ldata, tail);
+		retval = copy_to_user(*b, from, n);
 		n -= retval;
-		is_eof = n == 1 && read_buf(ldata, tail) == EOF_CHAR(tty);
-		tty_audit_add_data(tty, read_buf_addr(ldata, tail), n,
-				ldata->icanon);
+		is_eof = n == 1 && *from == EOF_CHAR(tty);
+		tty_audit_add_data(tty, from, n, ldata->icanon);
 		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
 		/* Turn single EOF into zero-length read */
 		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-08 14:36 UTC #1 lore raw

PATCH 1/1 n_tty: Uninline tty_copy_to_user()

Merge the multiple tty_copy_to_user() calls into a single copy sequence within tty_copy_to_user().

Signed-off-by Peter Hurley <[email protected]>

---

Requires: "tty: audit: Fix audit source"

 drivers/tty/n_tty.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

drivers/tty/n_tty.c +18 ~~-15~~

index c37c15d..b2b01d5 100644
@@ -162,12 +162,23 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
 	return put_user(x, ptr);
 }
 
-static inline int tty_copy_to_user(struct tty_struct *tty,
-					void __user *to,
-					const void *from,
-					unsigned long n)
+static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
+			    size_t tail, size_t n)
 {
 	struct n_tty_data *ldata = tty->disc_data;
+	size_t size = N_TTY_BUF_SIZE - tail;
+	const void *from = read_buf_addr(ldata, tail);
+	int uncopied;
+
+	if (n > size) {
+		tty_audit_add_data(tty, from, size, ldata->icanon);
+		uncopied = copy_to_user(to, from, size);
+		if (uncopied)
+			return uncopied;
+		to += size;
+		n -= size;
+		from = ldata->read_buf;
+	}
 
 	tty_audit_add_data(tty, from, n, ldata->icanon);
 	return copy_to_user(to, from, n);

@@ -2074,7 +2085,6 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	} else if (eol != size)
 		found = 1;
 
-	size = N_TTY_BUF_SIZE - tail;
 	n = eol - tail;
 	if (n > N_TTY_BUF_SIZE)
 		n += N_TTY_BUF_SIZE;

@@ -2086,17 +2096,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 		eof_push = !n && ldata->read_tail != ldata->line_start;
 	}
 
-	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
-		    __func__, eol, found, n, c, size, more);
-
-	if (n > size) {
-		ret = tty_copy_to_user(tty, *b, read_buf_addr(ldata, tail), size);
-		if (ret)
-			return -EFAULT;
-		ret = tty_copy_to_user(tty, *b + size, ldata->read_buf, n - size);
-	} else
-		ret = tty_copy_to_user(tty, *b, read_buf_addr(ldata, tail), n);
+	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu tail:%zu more:%zu\n",
+		    __func__, eol, found, n, c, tail, more);
 
+	ret = tty_copy_to_user(tty, *b, tail, n);
 	if (ret)
 		return -EFAULT;
 	*b += n;

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-27 19:11 UTC #2 lore raw

v2 cover n_tty_read() helper cleanups

In reply to Peter Hurley

Hi Greg,

This series rebases a couple of already-sent patches on top of the EOF push bug fix, "n_tty: Fix poll() after buffer-limited eof push read", and adds a third to reduce branching (which hopefully clarifies the copy algorithm some).

I will also resend the "Rework tty audit" series, which requires this series.

Regards,

Peter Hurley (3):
n_tty: Uninline tty_copy_to_user()
n_tty: Clarify copy_from_read_buf()
n_tty: Reduce branching in canon_copy_from_read_buf()

 drivers/tty/n_tty.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-27 19:11 UTC #3 lore raw

PATCH v2 1/3 n_tty: Uninline tty_copy_to_user()

Merge the multiple tty_copy_to_user() calls into a single copy sequence within tty_copy_to_user().

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

drivers/tty/n_tty.c +18 ~~-15~~

index b4bc8ea..9a96f26 100644
@@ -163,12 +163,23 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
 	return put_user(x, ptr);
 }
 
-static inline int tty_copy_to_user(struct tty_struct *tty,
-					void __user *to,
-					const void *from,
-					unsigned long n)
+static int tty_copy_to_user(struct tty_struct *tty, void __user *to,
+			    size_t tail, size_t n)
 {
 	struct n_tty_data *ldata = tty->disc_data;
+	size_t size = N_TTY_BUF_SIZE - tail;
+	const void *from = read_buf_addr(ldata, tail);
+	int uncopied;
+
+	if (n > size) {
+		tty_audit_add_data(tty, from, size, ldata->icanon);
+		uncopied = copy_to_user(to, from, size);
+		if (uncopied)
+			return uncopied;
+		to += size;
+		n -= size;
+		from = ldata->read_buf;
+	}
 
 	tty_audit_add_data(tty, from, n, ldata->icanon);
 	return copy_to_user(to, from, n);

@@ -2073,7 +2084,6 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	} else if (eol != size)
 		found = 1;
 
-	size = N_TTY_BUF_SIZE - tail;
 	n = eol - tail;
 	if (n > N_TTY_BUF_SIZE)
 		n += N_TTY_BUF_SIZE;

@@ -2084,17 +2094,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 		n = c;
 	}
 
-	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
-		    __func__, eol, found, n, c, size, more);
-
-	if (n > size) {
-		ret = tty_copy_to_user(tty, *b, read_buf_addr(ldata, tail), size);
-		if (ret)
-			return -EFAULT;
-		ret = tty_copy_to_user(tty, *b + size, ldata->read_buf, n - size);
-	} else
-		ret = tty_copy_to_user(tty, *b, read_buf_addr(ldata, tail), n);
+	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu tail:%zu more:%zu\n",
+		    __func__, eol, found, n, c, tail, more);
 
+	ret = tty_copy_to_user(tty, *b, tail, n);
 	if (ret)
 		return -EFAULT;
 	*b += n;

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-27 19:11 UTC #4 lore raw

PATCH v2 2/3 n_tty: Clarify copy_from_read_buf()

In reply to Peter Hurley

Add a temporary for the computed source address and substitute where appropriate. No functional change.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/tty/n_tty.c +4 -4

index 9a96f26..eab606a 100644
@@ -2013,11 +2013,11 @@ static int copy_from_read_buf(struct tty_struct *tty,
 	n = min(head - ldata->read_tail, N_TTY_BUF_SIZE - tail);
 	n = min(*nr, n);
 	if (n) {
-		retval = copy_to_user(*b, read_buf_addr(ldata, tail), n);
+		const unsigned char *from = read_buf_addr(ldata, tail);
+		retval = copy_to_user(*b, from, n);
 		n -= retval;
-		is_eof = n == 1 && read_buf(ldata, tail) == EOF_CHAR(tty);
-		tty_audit_add_data(tty, read_buf_addr(ldata, tail), n,
-				ldata->icanon);
+		is_eof = n == 1 && *from == EOF_CHAR(tty);
+		tty_audit_add_data(tty, from, n, ldata->icanon);
 		smp_store_release(&ldata->read_tail, ldata->read_tail + n);
 		/* Turn single EOF into zero-length read */
 		if (L_EXTPROC(tty) && ldata->icanon && is_eof &&

Signature · 1 line

2.6.3

Peter Hurley

author

2015-11-27 19:11 UTC #5 lore raw

PATCH v2 3/3 n_tty: Reduce branching in canon_copy_from_read_buf()

In reply to Peter Hurley

Instead of compare-and-set, just compute 'found'.

Signed-off-by Peter Hurley <[email protected]>

---
 drivers/tty/n_tty.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/tty/n_tty.c +3 -4

index eab606a..9b0b610 100644
@@ -2079,10 +2079,9 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	if (eol == N_TTY_BUF_SIZE && more) {
 		/* scan wrapped without finding set bit */
 		eol = find_next_bit(ldata->read_flags, more, 0);
-		if (eol != more)
-			found = 1;
-	} else if (eol != size)
-		found = 1;
+		found = eol != more;
+	} else
+		found = eol != size;
 
 	n = eol - tail;
 	if (n > N_TTY_BUF_SIZE)

Signature · 1 line

2.6.3