Re: [PATCH v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
Christian Riesch writes: > On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård wrote: >> Christian Riesch writes: > [...]>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c >>> index 2e900a9..b09f326 100644 >>> --- a/drivers/tty/n_tty.c >>> +++ b/drivers/tty/n_tty.c >>> @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct >>> *tty) >>> >>> static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata) >>> { >>> - *read_buf_addr(ldata, ldata->read_head++) = c; >>> + *read_buf_addr(ldata, ldata->read_head) = c; >>> + /* increment read_head _after_ placing the character in the buffer */ >>> + ldata->read_head++; >>> } >> >> Is that comment really necessary? > > No, I am pretty sure that removing the comment would not break the code ;-) > > I just thought it would be good to have some kind of reminder here. > Otherwise someone may think: Hey, it would be a good idea to do the > increment right in the first line. And submit a patch for it. The intent all along was to increment after the write. Nobody needs reminding of that. The problem was a misunderstanding of when the post-increment takes effect. As much as we'd like for everybody to have a thorough knowledge of C, a random tty driver doesn't seem the place to educate them. -- Måns Rullgård m...@mansr.com -- 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 v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Wed, Nov 12, 2014 at 12:53 PM, Måns Rullgård wrote: > Christian Riesch writes: > >> On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård wrote: >>> Christian Riesch writes: >> [...]>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 2e900a9..b09f326 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct *tty) static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata) { - *read_buf_addr(ldata, ldata->read_head++) = c; + *read_buf_addr(ldata, ldata->read_head) = c; + /* increment read_head _after_ placing the character in the buffer */ + ldata->read_head++; } >>> >>> Is that comment really necessary? >> >> No, I am pretty sure that removing the comment would not break the code ;-) >> >> I just thought it would be good to have some kind of reminder here. >> Otherwise someone may think: Hey, it would be a good idea to do the >> increment right in the first line. And submit a patch for it. > > The intent all along was to increment after the write. Nobody needs > reminding of that. The problem was a misunderstanding of when the > post-increment takes effect. As much as we'd like for everybody to have > a thorough knowledge of C, a random tty driver doesn't seem the place to > educate them. Ok. I will send a new patch without the comment. Thanks, 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 v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
Christian Riesch writes: > Commit 19e2ad6a09f0c06dbca19c98e5f4584269d913dd ("n_tty: Remove overflow > tests from receive_buf() path") moved the increment of read_head into > the arguments list of read_buf_addr(). Function calls represent a > sequence point in C. Therefore read_head is incremented before the > character c is placed in the buffer. Since the circular read buffer is > a lock-less design since commit 6d76bd2618535c581f1673047b8341fd291abc67 > ("n_tty: Make N_TTY ldisc receive path lockless"), this creates a race > condition that leads to communication errors. > > This patch modifies the code to increment read_head _after_ the data > is placed in the buffer and thus fixes the race for non-SMP machines. > To fix the problem for SMP machines, memory barriers must be added in > a separate patch. > > Signed-off-by: Christian Riesch > Cc: > --- > > This is version 2 of the patch in [1]. > > Changes for v2: > - Rewrote commit message. Since I did not know better, I blamed the compiler > in v1, but actually the code was wrong. See the discussion in [1]. > - Removed memory barriers. For non-SMP machines they are not required, > for SMP machines more brainwork and discussions are needed. > > Best regards, > Christian > > [1] https://lkml.org/lkml/2014/11/6/216 > > drivers/tty/n_tty.c |4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 2e900a9..b09f326 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct *tty) > > static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata) > { > - *read_buf_addr(ldata, ldata->read_head++) = c; > + *read_buf_addr(ldata, ldata->read_head) = c; > + /* increment read_head _after_ placing the character in the buffer */ > + ldata->read_head++; > } Is that comment really necessary? -- Måns Rullgård m...@mansr.com -- 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 v2] n_tty: Fix read_buf race condition, increment read_head after pushing data
On Tue, Nov 11, 2014 at 2:04 PM, Måns Rullgård wrote: > Christian Riesch writes: [...]>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c >> index 2e900a9..b09f326 100644 >> --- a/drivers/tty/n_tty.c >> +++ b/drivers/tty/n_tty.c >> @@ -321,7 +321,9 @@ static void n_tty_check_unthrottle(struct tty_struct >> *tty) >> >> static inline void put_tty_queue(unsigned char c, struct n_tty_data *ldata) >> { >> - *read_buf_addr(ldata, ldata->read_head++) = c; >> + *read_buf_addr(ldata, ldata->read_head) = c; >> + /* increment read_head _after_ placing the character in the buffer */ >> + ldata->read_head++; >> } > > Is that comment really necessary? No, I am pretty sure that removing the comment would not break the code ;-) I just thought it would be good to have some kind of reminder here. Otherwise someone may think: Hey, it would be a good idea to do the increment right in the first line. And submit a patch for it. But I am also ok with removing the comment. So if you like me to post a v3 without the comment, I'll be happy to do that. 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/