Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
[ reviewing my own patch :) ]
On 12/30/2014 07:05 AM, Peter Hurley wrote:
> Add commit_head buffer index, which the producer-side publishes
> after input processing. This ensures the consumer-side observes
> correctly-ordered writes in raw mode (ie., the buffer data is
> written before the buffer index is advanced).
>
> Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
> on the relevant buffer index; read_tail from the producer-side
> and canon_head/commit_head from the consumer-side, or both in shared
> paths such as receive_room().
>
> Based on work by Christian Riesch
>
> NB: Exclusive access is still guaranteed with termios_rwsem write
> lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
> circumstance, commit_head is equivalent to read_head.
>
> Cc: Christian Riesch
> Cc: # v3.14.x (will need backport to v3.12.x)
> Signed-off-by: Peter Hurley
> ---
> drivers/tty/n_tty.c | 80
> ++---
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index d2b4967..a618b10 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -90,6 +90,7 @@
> struct n_tty_data {
> /* producer-published */
> size_t read_head;
> + size_t commit_head; /* == read_head when not receiving */
commit_head is now the observable producer-side buffer index when
termios is !icanon || L_EXTPROC mode.
> size_t canon_head;
> size_t echo_head;
> size_t echo_commit;
> @@ -127,11 +128,6 @@ struct n_tty_data {
> struct mutex output_lock;
> };
>
> -static inline size_t read_cnt(struct n_tty_data *ldata)
> -{
> - return ldata->read_head - ldata->read_tail;
> -}
> -
Can keep read_cnt(). Will continue to be used in several places. See
other comments.
> static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
> {
> return ldata->read_buf[i & (N_TTY_BUF_SIZE - 1)];
> @@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
> unsigned char x,
> static int receive_room(struct tty_struct *tty)
> {
> struct n_tty_data *ldata = tty->disc_data;
> + size_t head = ACCESS_ONCE(ldata->commit_head);
> + size_t tail = ACCESS_ONCE(ldata->read_tail);
Producer-side receive_room() needs to be special-cased for the
call from the n_tty_receive_buf_common() i/o loop, because the
read_tail access needs to be:
size_t tail = smp_load_acquire(ldata->read_tail);
Paired with the consumer-side smp_store_release(read_tail), will
guarantee that the consumer has finished loading from the
read_buf before the producer may overwrite that data.
The producer-side receive_room() doesn't need the ACCESS_ONCE()s because
the commit_head and canon_head are only modified by itself (as the
producer).
The consumer-side receive_room() doesn't need the ACCESS_ONCE()s either.
At least one compiler barrier is passed through on each loop in
n_tty_read(), and at least once before starting the loop.
> int left;
>
> if (I_PARMRK(tty)) {
> - /* Multiply read_cnt by 3, since each byte might take up to
> + /* Multiply count by 3, since each byte might take up to
>* three times as many spaces when PARMRK is set (depending on
>* its flags, e.g. parity error). */
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
> } else
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
>
> /*
>* If we are doing input canonicalization, and there are no
> @@ -181,7 +179,7 @@ static int receive_room(struct tty_struct *tty)
>* characters will be beeped.
>*/
> if (left <= 0)
> - left = ldata->icanon && ldata->canon_head == ldata->read_tail;
> + left = ldata->icanon && ACCESS_ONCE(ldata->canon_head) == tail;
>
> return left;
> }
> @@ -224,9 +222,9 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
> ssize_t n = 0;
>
> if (!ldata->icanon)
> - n = read_cnt(ldata);
> + n = ACCESS_ONCE(ldata->commit_head) - ldata->read_tail;
> else
> - n = ldata->canon_head - ldata->read_tail;
> + n = ACCESS_ONCE(ldata->canon_head) - ldata->read_tail;
Existing compiler barriers in the consumer-side i/o loop make
these ACCESS_ONCE()s unnecessary (as with receive_room() and
input_available_p()).
> return n;
> }
>
> @@ -313,10 +311,6 @@ static void n_tty_check_unthrottle(struct tty_struct
> *tty)
> *
> * n_tty_receive_buf()/producer path:
> * caller holds non-exclusive termios_rwsem
> - * modifies read_head
> - *
> - * read_head is only considered 'published' if canonical mode is
> - * not active.
> */
>
> static inline void put_tty_queue(unsigned char c,
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
[ reviewing my own patch :) ]
On 12/30/2014 07:05 AM, Peter Hurley wrote:
Add commit_head buffer index, which the producer-side publishes
after input processing. This ensures the consumer-side observes
correctly-ordered writes in raw mode (ie., the buffer data is
written before the buffer index is advanced).
Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
on the relevant buffer index; read_tail from the producer-side
and canon_head/commit_head from the consumer-side, or both in shared
paths such as receive_room().
Based on work by Christian Riesch christian.rie...@omicron.at
NB: Exclusive access is still guaranteed with termios_rwsem write
lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
circumstance, commit_head is equivalent to read_head.
Cc: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org # v3.14.x (will need backport to v3.12.x)
Signed-off-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_tty.c | 80
++---
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d2b4967..a618b10 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -90,6 +90,7 @@
struct n_tty_data {
/* producer-published */
size_t read_head;
+ size_t commit_head; /* == read_head when not receiving */
commit_head is now the observable producer-side buffer index when
termios is !icanon || L_EXTPROC mode.
size_t canon_head;
size_t echo_head;
size_t echo_commit;
@@ -127,11 +128,6 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
-{
- return ldata-read_head - ldata-read_tail;
-}
-
Can keep read_cnt(). Will continue to be used in several places. See
other comments.
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
{
return ldata-read_buf[i (N_TTY_BUF_SIZE - 1)];
@@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
Producer-side receive_room() needs to be special-cased for the
call from the n_tty_receive_buf_common() i/o loop, because the
read_tail access needs to be:
size_t tail = smp_load_acquire(ldata-read_tail);
Paired with the consumer-side smp_store_release(read_tail), will
guarantee that the consumer has finished loading from the
read_buf before the producer may overwrite that data.
The producer-side receive_room() doesn't need the ACCESS_ONCE()s because
the commit_head and canon_head are only modified by itself (as the
producer).
The consumer-side receive_room() doesn't need the ACCESS_ONCE()s either.
At least one compiler barrier is passed through on each loop in
n_tty_read(), and at least once before starting the loop.
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
/*
* If we are doing input canonicalization, and there are no
@@ -181,7 +179,7 @@ static int receive_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left = 0)
- left = ldata-icanon ldata-canon_head == ldata-read_tail;
+ left = ldata-icanon ACCESS_ONCE(ldata-canon_head) == tail;
return left;
}
@@ -224,9 +222,9 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
ssize_t n = 0;
if (!ldata-icanon)
- n = read_cnt(ldata);
+ n = ACCESS_ONCE(ldata-commit_head) - ldata-read_tail;
else
- n = ldata-canon_head - ldata-read_tail;
+ n = ACCESS_ONCE(ldata-canon_head) - ldata-read_tail;
Existing compiler barriers in the consumer-side i/o loop make
these ACCESS_ONCE()s unnecessary (as with receive_room() and
input_available_p()).
return n;
}
@@ -313,10 +311,6 @@ static void n_tty_check_unthrottle(struct tty_struct
*tty)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * modifies read_head
- *
- * read_head is only considered 'published' if canonical mode is
- * not active.
*/
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
@@
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On 01/01/2015 06:00 AM, Christian Riesch wrote: > Peter, > > Thank you for this patch! Unfortunately I had not much time for this > since my last patch submission, so I am happy that someone continued > this work. > > I have a few comments/questions, please see below. > > On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley > wrote: >> Add commit_head buffer index, which the producer-side publishes >> after input processing. This ensures the consumer-side observes >> correctly-ordered writes in raw mode > > I understand that the commit_head reduces the number of memory > barriers and makes some things easier. But what is so special about > raw mode that requires the introduction of commit_head? commit_head is simply the read_head after each received buffer is processed by the input worker. In this context, I meant 'raw mode' as any non-canonical mode, ie., any mode in which copy_from_read_buf() is used by the reader. I'll replace 'raw' with 'non-canonical' instead. >> (ie., the buffer data is >> written before the buffer index is advanced). >> >> Further, remove read_cnt() and expand inline, using ACCESS_ONCE() > > "ACCESS_ONCE() and memory barriers"? This portion of the changelog refers only to read_cnt(), for which memory barriers are not required. But, while writing the explanatory fragment [1], I realized that input_available_p() must load the most recent relevant head index (either canon_head or commit_head based on the mode) because it may conclude there is no more input _and_ then observe an end-of-file condition. So I need to fix this up to do the smp_load_acquire() in input_available_p() and ACCESS_ONCE() in the *_copy_from_read_buf(). Regards, Peter Hurley [1] Strictly speaking, the ACCESS_ONCE()'s are optimizations. Neither the producer nor consumer require the most recent 'opposed' index; if the compiler elided the opposed index load, instead reusing an existing load -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On 01/01/2015 06:00 AM, Christian Riesch wrote: Peter, Thank you for this patch! Unfortunately I had not much time for this since my last patch submission, so I am happy that someone continued this work. I have a few comments/questions, please see below. On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley pe...@hurleysoftware.com wrote: Add commit_head buffer index, which the producer-side publishes after input processing. This ensures the consumer-side observes correctly-ordered writes in raw mode I understand that the commit_head reduces the number of memory barriers and makes some things easier. But what is so special about raw mode that requires the introduction of commit_head? commit_head is simply the read_head after each received buffer is processed by the input worker. In this context, I meant 'raw mode' as any non-canonical mode, ie., any mode in which copy_from_read_buf() is used by the reader. I'll replace 'raw' with 'non-canonical' instead. (ie., the buffer data is written before the buffer index is advanced). Further, remove read_cnt() and expand inline, using ACCESS_ONCE() ACCESS_ONCE() and memory barriers? This portion of the changelog refers only to read_cnt(), for which memory barriers are not required. But, while writing the explanatory fragment [1], I realized that input_available_p() must load the most recent relevant head index (either canon_head or commit_head based on the mode) because it may conclude there is no more input _and_ then observe an end-of-file condition. So I need to fix this up to do the smp_load_acquire() in input_available_p() and ACCESS_ONCE() in the *_copy_from_read_buf(). Regards, Peter Hurley [1] Strictly speaking, the ACCESS_ONCE()'s are optimizations. Neither the producer nor consumer require the most recent 'opposed' index; if the compiler elided the opposed index load, instead reusing an existing load -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Hi Christian,
On 01/01/2015 08:55 AM, Christian Riesch wrote:
> On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch >> @@ -164,15
> +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
> unsigned char x,
>>> static int receive_room(struct tty_struct *tty)
>>> {
>>> struct n_tty_data *ldata = tty->disc_data;
>>> + size_t head = ACCESS_ONCE(ldata->commit_head);
>>> + size_t tail = ACCESS_ONCE(ldata->read_tail);
>>> int left;
>>>
>>> if (I_PARMRK(tty)) {
>>> - /* Multiply read_cnt by 3, since each byte might take up to
>>> + /* Multiply count by 3, since each byte might take up to
>>> * three times as many spaces when PARMRK is set (depending
>>> on
>>> * its flags, e.g. parity error). */
>>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
>>> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
>>> } else
>>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
>>> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
>>
>> Actually, less room may be available, if read_head != commit_head.
>> Could this cause problems? I guess yes, at least in
>> n_tty_receive_buf_common, where this could lead to a buffer overflow,
>> right?
>
> Sorry, should not be a problem, at least not for
> n_tty_receive_buf_common, since this is producer path, right?
Yeah, that's what I was in the process of writing just now.
BTW, I did see your note about the I_PARMRK computation being
overly conservative; I'll address that in a separate patch
on top of this.
> But how about the other calls of receive_room()?
Those are all either consumer-side or exclusive, ie., when both
producer and consumer are prevented from running by the termios_rwsem
write lock (eg., n_tty_set_termios()).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch >> @@ -164,15
+160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
>> static int receive_room(struct tty_struct *tty)
>> {
>> struct n_tty_data *ldata = tty->disc_data;
>> + size_t head = ACCESS_ONCE(ldata->commit_head);
>> + size_t tail = ACCESS_ONCE(ldata->read_tail);
>> int left;
>>
>> if (I_PARMRK(tty)) {
>> - /* Multiply read_cnt by 3, since each byte might take up to
>> + /* Multiply count by 3, since each byte might take up to
>> * three times as many spaces when PARMRK is set (depending
>> on
>> * its flags, e.g. parity error). */
>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
>> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
>> } else
>> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
>> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
>
> Actually, less room may be available, if read_head != commit_head.
> Could this cause problems? I guess yes, at least in
> n_tty_receive_buf_common, where this could lead to a buffer overflow,
> right?
Sorry, should not be a problem, at least not for
n_tty_receive_buf_common, since this is producer path, right? But how
about the other calls of receive_room()?
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Peter,
Thank you for this patch! Unfortunately I had not much time for this
since my last patch submission, so I am happy that someone continued
this work.
I have a few comments/questions, please see below.
On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley wrote:
> Add commit_head buffer index, which the producer-side publishes
> after input processing. This ensures the consumer-side observes
> correctly-ordered writes in raw mode
I understand that the commit_head reduces the number of memory
barriers and makes some things easier. But what is so special about
raw mode that requires the introduction of commit_head?
> (ie., the buffer data is
> written before the buffer index is advanced).
>
> Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
"ACCESS_ONCE() and memory barriers"?
> on the relevant buffer index; read_tail from the producer-side
> and canon_head/commit_head from the consumer-side, or both in shared
> paths such as receive_room().
>
> Based on work by Christian Riesch
>
> NB: Exclusive access is still guaranteed with termios_rwsem write
> lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
> circumstance, commit_head is equivalent to read_head.
>
> Cc: Christian Riesch
> Cc: # v3.14.x (will need backport to v3.12.x)
> Signed-off-by: Peter Hurley
> ---
> drivers/tty/n_tty.c | 80
> ++---
> 1 file changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index d2b4967..a618b10 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -90,6 +90,7 @@
> struct n_tty_data {
> /* producer-published */
> size_t read_head;
> + size_t commit_head; /* == read_head when not receiving */
> size_t canon_head;
> size_t echo_head;
> size_t echo_commit;
> @@ -127,11 +128,6 @@ struct n_tty_data {
> struct mutex output_lock;
> };
>
> -static inline size_t read_cnt(struct n_tty_data *ldata)
> -{
> - return ldata->read_head - ldata->read_tail;
> -}
> -
> static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
> {
> return ldata->read_buf[i & (N_TTY_BUF_SIZE - 1)];
> @@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
> unsigned char x,
> static int receive_room(struct tty_struct *tty)
> {
> struct n_tty_data *ldata = tty->disc_data;
> + size_t head = ACCESS_ONCE(ldata->commit_head);
> + size_t tail = ACCESS_ONCE(ldata->read_tail);
> int left;
>
> if (I_PARMRK(tty)) {
> - /* Multiply read_cnt by 3, since each byte might take up to
> + /* Multiply count by 3, since each byte might take up to
> * three times as many spaces when PARMRK is set (depending on
> * its flags, e.g. parity error). */
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
> } else
> - left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
> + left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Best regards,
Christian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Peter,
Thank you for this patch! Unfortunately I had not much time for this
since my last patch submission, so I am happy that someone continued
this work.
I have a few comments/questions, please see below.
On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley pe...@hurleysoftware.com wrote:
Add commit_head buffer index, which the producer-side publishes
after input processing. This ensures the consumer-side observes
correctly-ordered writes in raw mode
I understand that the commit_head reduces the number of memory
barriers and makes some things easier. But what is so special about
raw mode that requires the introduction of commit_head?
(ie., the buffer data is
written before the buffer index is advanced).
Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
ACCESS_ONCE() and memory barriers?
on the relevant buffer index; read_tail from the producer-side
and canon_head/commit_head from the consumer-side, or both in shared
paths such as receive_room().
Based on work by Christian Riesch christian.rie...@omicron.at
NB: Exclusive access is still guaranteed with termios_rwsem write
lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
circumstance, commit_head is equivalent to read_head.
Cc: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org # v3.14.x (will need backport to v3.12.x)
Signed-off-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_tty.c | 80
++---
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d2b4967..a618b10 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -90,6 +90,7 @@
struct n_tty_data {
/* producer-published */
size_t read_head;
+ size_t commit_head; /* == read_head when not receiving */
size_t canon_head;
size_t echo_head;
size_t echo_commit;
@@ -127,11 +128,6 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
-{
- return ldata-read_head - ldata-read_tail;
-}
-
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
{
return ldata-read_buf[i (N_TTY_BUF_SIZE - 1)];
@@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Best regards,
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
Hi Christian,
On 01/01/2015 08:55 AM, Christian Riesch wrote:
On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch @@ -164,15
+160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending
on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Sorry, should not be a problem, at least not for
n_tty_receive_buf_common, since this is producer path, right?
Yeah, that's what I was in the process of writing just now.
BTW, I did see your note about the I_PARMRK computation being
overly conservative; I'll address that in a separate patch
on top of this.
But how about the other calls of receive_room()?
Those are all either consumer-side or exclusive, ie., when both
producer and consumer are prevented from running by the termios_rwsem
write lock (eg., n_tty_set_termios()).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer
On Thu, Jan 1, 2015 at 12:00 PM, Christian Riesch @@ -164,15
+160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending
on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
Actually, less room may be available, if read_head != commit_head.
Could this cause problems? I guess yes, at least in
n_tty_receive_buf_common, where this could lead to a buffer overflow,
right?
Sorry, should not be a problem, at least not for
n_tty_receive_buf_common, since this is producer path, right? But how
about the other calls of receive_room()?
Christian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] n_tty: Fix unordered accesses to lockless read buffer
Add commit_head buffer index, which the producer-side publishes
after input processing. This ensures the consumer-side observes
correctly-ordered writes in raw mode (ie., the buffer data is
written before the buffer index is advanced).
Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
on the relevant buffer index; read_tail from the producer-side
and canon_head/commit_head from the consumer-side, or both in shared
paths such as receive_room().
Based on work by Christian Riesch
NB: Exclusive access is still guaranteed with termios_rwsem write
lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
circumstance, commit_head is equivalent to read_head.
Cc: Christian Riesch
Cc: # v3.14.x (will need backport to v3.12.x)
Signed-off-by: Peter Hurley
---
drivers/tty/n_tty.c | 80 ++---
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d2b4967..a618b10 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -90,6 +90,7 @@
struct n_tty_data {
/* producer-published */
size_t read_head;
+ size_t commit_head; /* == read_head when not receiving */
size_t canon_head;
size_t echo_head;
size_t echo_commit;
@@ -127,11 +128,6 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
-{
- return ldata->read_head - ldata->read_tail;
-}
-
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
{
return ldata->read_buf[i & (N_TTY_BUF_SIZE - 1)];
@@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty->disc_data;
+ size_t head = ACCESS_ONCE(ldata->commit_head);
+ size_t tail = ACCESS_ONCE(ldata->read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
/*
* If we are doing input canonicalization, and there are no
@@ -181,7 +179,7 @@ static int receive_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left <= 0)
- left = ldata->icanon && ldata->canon_head == ldata->read_tail;
+ left = ldata->icanon && ACCESS_ONCE(ldata->canon_head) == tail;
return left;
}
@@ -224,9 +222,9 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
ssize_t n = 0;
if (!ldata->icanon)
- n = read_cnt(ldata);
+ n = ACCESS_ONCE(ldata->commit_head) - ldata->read_tail;
else
- n = ldata->canon_head - ldata->read_tail;
+ n = ACCESS_ONCE(ldata->canon_head) - ldata->read_tail;
return n;
}
@@ -313,10 +311,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * modifies read_head
- *
- * read_head is only considered 'published' if canonical mode is
- * not active.
*/
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
@@ -340,6 +334,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata)
{
ldata->read_head = ldata->canon_head = ldata->read_tail = 0;
ldata->echo_head = ldata->echo_tail = ldata->echo_commit = 0;
+ ldata->commit_head = 0;
ldata->echo_mark = 0;
ldata->line_start = 0;
@@ -987,10 +982,6 @@ static inline void finish_erasing(struct n_tty_data *ldata)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * modifies read_head
- *
- * Modifying the read_head is not considered a publish in this context
- * because canonical mode is active -- only canon_head publishes
*/
static void eraser(unsigned char c, struct tty_struct *tty)
@@ -1139,7 +1130,7 @@ static void isig(int sig, struct tty_struct *tty)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * publishes read_head via put_tty_queue()
+ * publishes read_head as commit_head
*
* Note: may get exclusive termios_rwsem if flushing input buffer
*/
@@ -1166,6 +1157,8 @@ static void n_tty_receive_break(struct tty_struct *tty)
put_tty_queue('\0', ldata);
}
[PATCH] n_tty: Fix unordered accesses to lockless read buffer
Add commit_head buffer index, which the producer-side publishes
after input processing. This ensures the consumer-side observes
correctly-ordered writes in raw mode (ie., the buffer data is
written before the buffer index is advanced).
Further, remove read_cnt() and expand inline, using ACCESS_ONCE()
on the relevant buffer index; read_tail from the producer-side
and canon_head/commit_head from the consumer-side, or both in shared
paths such as receive_room().
Based on work by Christian Riesch christian.rie...@omicron.at
NB: Exclusive access is still guaranteed with termios_rwsem write
lock, eg. by n_tty_set_termios() and in n_tty_ioctl(); in this
circumstance, commit_head is equivalent to read_head.
Cc: Christian Riesch christian.rie...@omicron.at
Cc: sta...@vger.kernel.org # v3.14.x (will need backport to v3.12.x)
Signed-off-by: Peter Hurley pe...@hurleysoftware.com
---
drivers/tty/n_tty.c | 80 ++---
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d2b4967..a618b10 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -90,6 +90,7 @@
struct n_tty_data {
/* producer-published */
size_t read_head;
+ size_t commit_head; /* == read_head when not receiving */
size_t canon_head;
size_t echo_head;
size_t echo_commit;
@@ -127,11 +128,6 @@ struct n_tty_data {
struct mutex output_lock;
};
-static inline size_t read_cnt(struct n_tty_data *ldata)
-{
- return ldata-read_head - ldata-read_tail;
-}
-
static inline unsigned char read_buf(struct n_tty_data *ldata, size_t i)
{
return ldata-read_buf[i (N_TTY_BUF_SIZE - 1)];
@@ -164,15 +160,17 @@ static inline int tty_put_user(struct tty_struct *tty,
unsigned char x,
static int receive_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty-disc_data;
+ size_t head = ACCESS_ONCE(ldata-commit_head);
+ size_t tail = ACCESS_ONCE(ldata-read_tail);
int left;
if (I_PARMRK(tty)) {
- /* Multiply read_cnt by 3, since each byte might take up to
+ /* Multiply count by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
* its flags, e.g. parity error). */
- left = N_TTY_BUF_SIZE - read_cnt(ldata) * 3 - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) * 3 - 1;
} else
- left = N_TTY_BUF_SIZE - read_cnt(ldata) - 1;
+ left = N_TTY_BUF_SIZE - (head - tail) - 1;
/*
* If we are doing input canonicalization, and there are no
@@ -181,7 +179,7 @@ static int receive_room(struct tty_struct *tty)
* characters will be beeped.
*/
if (left = 0)
- left = ldata-icanon ldata-canon_head == ldata-read_tail;
+ left = ldata-icanon ACCESS_ONCE(ldata-canon_head) == tail;
return left;
}
@@ -224,9 +222,9 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
ssize_t n = 0;
if (!ldata-icanon)
- n = read_cnt(ldata);
+ n = ACCESS_ONCE(ldata-commit_head) - ldata-read_tail;
else
- n = ldata-canon_head - ldata-read_tail;
+ n = ACCESS_ONCE(ldata-canon_head) - ldata-read_tail;
return n;
}
@@ -313,10 +311,6 @@ static void n_tty_check_unthrottle(struct tty_struct *tty)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * modifies read_head
- *
- * read_head is only considered 'published' if canonical mode is
- * not active.
*/
static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
@@ -340,6 +334,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata)
{
ldata-read_head = ldata-canon_head = ldata-read_tail = 0;
ldata-echo_head = ldata-echo_tail = ldata-echo_commit = 0;
+ ldata-commit_head = 0;
ldata-echo_mark = 0;
ldata-line_start = 0;
@@ -987,10 +982,6 @@ static inline void finish_erasing(struct n_tty_data *ldata)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * modifies read_head
- *
- * Modifying the read_head is not considered a publish in this context
- * because canonical mode is active -- only canon_head publishes
*/
static void eraser(unsigned char c, struct tty_struct *tty)
@@ -1139,7 +1130,7 @@ static void isig(int sig, struct tty_struct *tty)
*
* n_tty_receive_buf()/producer path:
* caller holds non-exclusive termios_rwsem
- * publishes read_head via put_tty_queue()
+ * publishes read_head as commit_head
*
* Note: may get exclusive termios_rwsem if flushing input buffer
*/
@@ -1166,6 +1157,8 @@ static void n_tty_receive_break(struct tty_struct
