Re: [PATCH] tty: fix data race in tty_buffer_flush
On Thu, Sep 17, 2015 at 12:28:11PM +0200, Dmitry Vyukov wrote: > One stupid question. There is a number of branches: > remotes/origin/master > remotes/origin/tty-linus > remotes/origin/tty-next > remotes/origin/tty-testing > > What branch should I base my patches on? I used master. Right now, if you look, they all point to the same thing, so it doesn't matter :) tty-linus is for patches to go to Linus this kernel release. tty-next is for patches to go to Linus the next kernel release. Both tty-linus and tty-next show up in linux-next and are never rebased. tty-testing is for patches that are being considered for tty-next, it does not show up in linux-next and sometimes is rebased if things go wrong with the patches. Hope this helps, greg k-h -- 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] tty: fix data race in tty_buffer_flush
I've resent the 3 patches with version information. On Thu, Sep 17, 2015 at 12:28 PM, Dmitry Vyukov wrote: > One stupid question. There is a number of branches: > remotes/origin/master > remotes/origin/tty-linus > remotes/origin/tty-next > remotes/origin/tty-testing > > What branch should I base my patches on? I used master. > > > > > On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukov wrote: >> Sorry for the delay. It is starred in my inbox and I plan to resend >> the patches. Just did not have time yet. >> >> >> On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley >> wrote: >>> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: tty_buffer_flush frees not acquired buffers. As the result, for example, read of b->size in tty_buffer_free can return garbage value which will lead to a huge buffer hanging in the freelist. This is just the benignest manifestation of freeing of a not acquired object. If the object is passed to kfree, heap can be corrupted. Acquire visibility over the buffer before freeing it. The data race was found with KernelThreadSanitizer (KTSAN). >>> >>> Dmitry, >>> >>> Looks good. Thanks for splitting this from the other fix. >>> >>> Like Greg said, the patch needs revision info to help maintainers, et al >>> track which is most current/relevant/etc. >>> >>> Typical format for $subject is something like: >>> >>> [PATCH v4] tty: fix data race in tty_buffer_flush >>> >>> >>> Notes re: changes from other patch versions are added below the >>> git changelog separator (example below). >>> Signed-off-by: Dmitry Vyukov --- >>> >>> v4: Corrected commit log and patch revision notes >>> >>> v3: Added code comment re: paired smp barrier >>> >>> v2: Split from 'tty: fix data races on tty_buffer.commit' >>> drivers/tty/tty_buffer.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 5a3fa89..a2a8cd0 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld) atomic_inc(>priority); mutex_lock(>lock); - while ((next = buf->head->next) != NULL) { + /* paired w/ release in __tty_buffer_request_room; ensures there are +* no pending memory accesses to the freed buffer +*/ + while ((next = smp_load_acquire(>head->next)) != NULL) { tty_buffer_free(port, buf->head); buf->head = next; } -- 2.5.0.457.gab17608 >> >> >> >> -- >> Dmitry Vyukov, Software Engineer, dvyu...@google.com >> Google Germany GmbH, Dienerstraße 12, 80331, München >> Geschäftsführer: Graham Law, Christine Elizabeth Flores >> Registergericht und -nummer: Hamburg, HRB 86891 >> Sitz der Gesellschaft: Hamburg >> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat >> sind, leiten Sie diese bitte nicht weiter, informieren Sie den >> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. >> This e-mail is confidential. If you are not the right addressee please >> do not forward it, please inform the sender, and please erase this >> e-mail including any attachments. Thanks. > > > > -- > Dmitry Vyukov, Software Engineer, dvyu...@google.com > Google Germany GmbH, Dienerstraße 12, 80331, München > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat > sind, leiten Sie diese bitte nicht weiter, informieren Sie den > Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. > This e-mail is confidential. If you are not the right addressee please > do not forward it, please inform the sender, and please erase this > e-mail including any attachments. Thanks. -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
One stupid question. There is a number of branches: remotes/origin/master remotes/origin/tty-linus remotes/origin/tty-next remotes/origin/tty-testing What branch should I base my patches on? I used master. On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukov wrote: > Sorry for the delay. It is starred in my inbox and I plan to resend > the patches. Just did not have time yet. > > > On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley > wrote: >> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: >>> tty_buffer_flush frees not acquired buffers. >>> As the result, for example, read of b->size in tty_buffer_free >>> can return garbage value which will lead to a huge buffer >>> hanging in the freelist. This is just the benignest >>> manifestation of freeing of a not acquired object. >>> If the object is passed to kfree, heap can be corrupted. >>> >>> Acquire visibility over the buffer before freeing it. >>> >>> The data race was found with KernelThreadSanitizer (KTSAN). >> >> Dmitry, >> >> Looks good. Thanks for splitting this from the other fix. >> >> Like Greg said, the patch needs revision info to help maintainers, et al >> track which is most current/relevant/etc. >> >> Typical format for $subject is something like: >> >> [PATCH v4] tty: fix data race in tty_buffer_flush >> >> >> Notes re: changes from other patch versions are added below the >> git changelog separator (example below). >> >>> Signed-off-by: Dmitry Vyukov >>> --- >> >> v4: Corrected commit log and patch revision notes >> >> v3: Added code comment re: paired smp barrier >> >> v2: Split from 'tty: fix data races on tty_buffer.commit' >> >>> drivers/tty/tty_buffer.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c >>> index 5a3fa89..a2a8cd0 100644 >>> --- a/drivers/tty/tty_buffer.c >>> +++ b/drivers/tty/tty_buffer.c >>> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct >>> tty_ldisc *ld) >>> atomic_inc(>priority); >>> >>> mutex_lock(>lock); >>> - while ((next = buf->head->next) != NULL) { >>> + /* paired w/ release in __tty_buffer_request_room; ensures there are >>> +* no pending memory accesses to the freed buffer >>> +*/ >>> + while ((next = smp_load_acquire(>head->next)) != NULL) { >>> tty_buffer_free(port, buf->head); >>> buf->head = next; >>> } >>> -- >>> 2.5.0.457.gab17608 >>> > > > > -- > Dmitry Vyukov, Software Engineer, dvyu...@google.com > Google Germany GmbH, Dienerstraße 12, 80331, München > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat > sind, leiten Sie diese bitte nicht weiter, informieren Sie den > Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. > This e-mail is confidential. If you are not the right addressee please > do not forward it, please inform the sender, and please erase this > e-mail including any attachments. Thanks. -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
One stupid question. There is a number of branches: remotes/origin/master remotes/origin/tty-linus remotes/origin/tty-next remotes/origin/tty-testing What branch should I base my patches on? I used master. On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukovwrote: > Sorry for the delay. It is starred in my inbox and I plan to resend > the patches. Just did not have time yet. > > > On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley > wrote: >> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: >>> tty_buffer_flush frees not acquired buffers. >>> As the result, for example, read of b->size in tty_buffer_free >>> can return garbage value which will lead to a huge buffer >>> hanging in the freelist. This is just the benignest >>> manifestation of freeing of a not acquired object. >>> If the object is passed to kfree, heap can be corrupted. >>> >>> Acquire visibility over the buffer before freeing it. >>> >>> The data race was found with KernelThreadSanitizer (KTSAN). >> >> Dmitry, >> >> Looks good. Thanks for splitting this from the other fix. >> >> Like Greg said, the patch needs revision info to help maintainers, et al >> track which is most current/relevant/etc. >> >> Typical format for $subject is something like: >> >> [PATCH v4] tty: fix data race in tty_buffer_flush >> >> >> Notes re: changes from other patch versions are added below the >> git changelog separator (example below). >> >>> Signed-off-by: Dmitry Vyukov >>> --- >> >> v4: Corrected commit log and patch revision notes >> >> v3: Added code comment re: paired smp barrier >> >> v2: Split from 'tty: fix data races on tty_buffer.commit' >> >>> drivers/tty/tty_buffer.c | 5 - >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c >>> index 5a3fa89..a2a8cd0 100644 >>> --- a/drivers/tty/tty_buffer.c >>> +++ b/drivers/tty/tty_buffer.c >>> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct >>> tty_ldisc *ld) >>> atomic_inc(>priority); >>> >>> mutex_lock(>lock); >>> - while ((next = buf->head->next) != NULL) { >>> + /* paired w/ release in __tty_buffer_request_room; ensures there are >>> +* no pending memory accesses to the freed buffer >>> +*/ >>> + while ((next = smp_load_acquire(>head->next)) != NULL) { >>> tty_buffer_free(port, buf->head); >>> buf->head = next; >>> } >>> -- >>> 2.5.0.457.gab17608 >>> > > > > -- > Dmitry Vyukov, Software Engineer, dvyu...@google.com > Google Germany GmbH, Dienerstraße 12, 80331, München > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat > sind, leiten Sie diese bitte nicht weiter, informieren Sie den > Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. > This e-mail is confidential. If you are not the right addressee please > do not forward it, please inform the sender, and please erase this > e-mail including any attachments. Thanks. -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
I've resent the 3 patches with version information. On Thu, Sep 17, 2015 at 12:28 PM, Dmitry Vyukovwrote: > One stupid question. There is a number of branches: > remotes/origin/master > remotes/origin/tty-linus > remotes/origin/tty-next > remotes/origin/tty-testing > > What branch should I base my patches on? I used master. > > > > > On Wed, Sep 16, 2015 at 8:20 PM, Dmitry Vyukov wrote: >> Sorry for the delay. It is starred in my inbox and I plan to resend >> the patches. Just did not have time yet. >> >> >> On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley >> wrote: >>> On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: tty_buffer_flush frees not acquired buffers. As the result, for example, read of b->size in tty_buffer_free can return garbage value which will lead to a huge buffer hanging in the freelist. This is just the benignest manifestation of freeing of a not acquired object. If the object is passed to kfree, heap can be corrupted. Acquire visibility over the buffer before freeing it. The data race was found with KernelThreadSanitizer (KTSAN). >>> >>> Dmitry, >>> >>> Looks good. Thanks for splitting this from the other fix. >>> >>> Like Greg said, the patch needs revision info to help maintainers, et al >>> track which is most current/relevant/etc. >>> >>> Typical format for $subject is something like: >>> >>> [PATCH v4] tty: fix data race in tty_buffer_flush >>> >>> >>> Notes re: changes from other patch versions are added below the >>> git changelog separator (example below). >>> Signed-off-by: Dmitry Vyukov --- >>> >>> v4: Corrected commit log and patch revision notes >>> >>> v3: Added code comment re: paired smp barrier >>> >>> v2: Split from 'tty: fix data races on tty_buffer.commit' >>> drivers/tty/tty_buffer.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 5a3fa89..a2a8cd0 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld) atomic_inc(>priority); mutex_lock(>lock); - while ((next = buf->head->next) != NULL) { + /* paired w/ release in __tty_buffer_request_room; ensures there are +* no pending memory accesses to the freed buffer +*/ + while ((next = smp_load_acquire(>head->next)) != NULL) { tty_buffer_free(port, buf->head); buf->head = next; } -- 2.5.0.457.gab17608 >> >> >> >> -- >> Dmitry Vyukov, Software Engineer, dvyu...@google.com >> Google Germany GmbH, Dienerstraße 12, 80331, München >> Geschäftsführer: Graham Law, Christine Elizabeth Flores >> Registergericht und -nummer: Hamburg, HRB 86891 >> Sitz der Gesellschaft: Hamburg >> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat >> sind, leiten Sie diese bitte nicht weiter, informieren Sie den >> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. >> This e-mail is confidential. If you are not the right addressee please >> do not forward it, please inform the sender, and please erase this >> e-mail including any attachments. Thanks. > > > > -- > Dmitry Vyukov, Software Engineer, dvyu...@google.com > Google Germany GmbH, Dienerstraße 12, 80331, München > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat > sind, leiten Sie diese bitte nicht weiter, informieren Sie den > Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. > This e-mail is confidential. If you are not the right addressee please > do not forward it, please inform the sender, and please erase this > e-mail including any attachments. Thanks. -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
On Thu, Sep 17, 2015 at 12:28:11PM +0200, Dmitry Vyukov wrote: > One stupid question. There is a number of branches: > remotes/origin/master > remotes/origin/tty-linus > remotes/origin/tty-next > remotes/origin/tty-testing > > What branch should I base my patches on? I used master. Right now, if you look, they all point to the same thing, so it doesn't matter :) tty-linus is for patches to go to Linus this kernel release. tty-next is for patches to go to Linus the next kernel release. Both tty-linus and tty-next show up in linux-next and are never rebased. tty-testing is for patches that are being considered for tty-next, it does not show up in linux-next and sometimes is rebased if things go wrong with the patches. Hope this helps, greg k-h -- 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] tty: fix data race in tty_buffer_flush
Sorry for the delay. It is starred in my inbox and I plan to resend the patches. Just did not have time yet. On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurley wrote: > On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: >> tty_buffer_flush frees not acquired buffers. >> As the result, for example, read of b->size in tty_buffer_free >> can return garbage value which will lead to a huge buffer >> hanging in the freelist. This is just the benignest >> manifestation of freeing of a not acquired object. >> If the object is passed to kfree, heap can be corrupted. >> >> Acquire visibility over the buffer before freeing it. >> >> The data race was found with KernelThreadSanitizer (KTSAN). > > Dmitry, > > Looks good. Thanks for splitting this from the other fix. > > Like Greg said, the patch needs revision info to help maintainers, et al > track which is most current/relevant/etc. > > Typical format for $subject is something like: > > [PATCH v4] tty: fix data race in tty_buffer_flush > > > Notes re: changes from other patch versions are added below the > git changelog separator (example below). > >> Signed-off-by: Dmitry Vyukov >> --- > > v4: Corrected commit log and patch revision notes > > v3: Added code comment re: paired smp barrier > > v2: Split from 'tty: fix data races on tty_buffer.commit' > >> drivers/tty/tty_buffer.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c >> index 5a3fa89..a2a8cd0 100644 >> --- a/drivers/tty/tty_buffer.c >> +++ b/drivers/tty/tty_buffer.c >> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct >> tty_ldisc *ld) >> atomic_inc(>priority); >> >> mutex_lock(>lock); >> - while ((next = buf->head->next) != NULL) { >> + /* paired w/ release in __tty_buffer_request_room; ensures there are >> +* no pending memory accesses to the freed buffer >> +*/ >> + while ((next = smp_load_acquire(>head->next)) != NULL) { >> tty_buffer_free(port, buf->head); >> buf->head = next; >> } >> -- >> 2.5.0.457.gab17608 >> -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
Sorry for the delay. It is starred in my inbox and I plan to resend the patches. Just did not have time yet. On Wed, Sep 16, 2015 at 3:38 AM, Peter Hurleywrote: > On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: >> tty_buffer_flush frees not acquired buffers. >> As the result, for example, read of b->size in tty_buffer_free >> can return garbage value which will lead to a huge buffer >> hanging in the freelist. This is just the benignest >> manifestation of freeing of a not acquired object. >> If the object is passed to kfree, heap can be corrupted. >> >> Acquire visibility over the buffer before freeing it. >> >> The data race was found with KernelThreadSanitizer (KTSAN). > > Dmitry, > > Looks good. Thanks for splitting this from the other fix. > > Like Greg said, the patch needs revision info to help maintainers, et al > track which is most current/relevant/etc. > > Typical format for $subject is something like: > > [PATCH v4] tty: fix data race in tty_buffer_flush > > > Notes re: changes from other patch versions are added below the > git changelog separator (example below). > >> Signed-off-by: Dmitry Vyukov >> --- > > v4: Corrected commit log and patch revision notes > > v3: Added code comment re: paired smp barrier > > v2: Split from 'tty: fix data races on tty_buffer.commit' > >> drivers/tty/tty_buffer.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c >> index 5a3fa89..a2a8cd0 100644 >> --- a/drivers/tty/tty_buffer.c >> +++ b/drivers/tty/tty_buffer.c >> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct >> tty_ldisc *ld) >> atomic_inc(>priority); >> >> mutex_lock(>lock); >> - while ((next = buf->head->next) != NULL) { >> + /* paired w/ release in __tty_buffer_request_room; ensures there are >> +* no pending memory accesses to the freed buffer >> +*/ >> + while ((next = smp_load_acquire(>head->next)) != NULL) { >> tty_buffer_free(port, buf->head); >> buf->head = next; >> } >> -- >> 2.5.0.457.gab17608 >> -- Dmitry Vyukov, Software Engineer, dvyu...@google.com Google Germany GmbH, Dienerstraße 12, 80331, München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks. -- 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] tty: fix data race in tty_buffer_flush
On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov wrote: > tty_buffer_flush frees not acquired buffers. > As the result, for example, read of b->size in tty_buffer_free > can return garbage value which will lead to a huge buffer > hanging in the freelist. This is just the benignest > manifestation of freeing of a not acquired object. > If the object is passed to kfree, heap can be corrupted. > > Acquire visibility over the buffer before freeing it. > > The data race was found with KernelThreadSanitizer (KTSAN). Dmitry, Looks good. Thanks for splitting this from the other fix. Like Greg said, the patch needs revision info to help maintainers, et al track which is most current/relevant/etc. Typical format for $subject is something like: [PATCH v4] tty: fix data race in tty_buffer_flush Notes re: changes from other patch versions are added below the git changelog separator (example below). > Signed-off-by: Dmitry Vyukov > --- v4: Corrected commit log and patch revision notes v3: Added code comment re: paired smp barrier v2: Split from 'tty: fix data races on tty_buffer.commit' > drivers/tty/tty_buffer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c > index 5a3fa89..a2a8cd0 100644 > --- a/drivers/tty/tty_buffer.c > +++ b/drivers/tty/tty_buffer.c > @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct > tty_ldisc *ld) > atomic_inc(>priority); > > mutex_lock(>lock); > - while ((next = buf->head->next) != NULL) { > + /* paired w/ release in __tty_buffer_request_room; ensures there are > +* no pending memory accesses to the freed buffer > +*/ > + while ((next = smp_load_acquire(>head->next)) != NULL) { > tty_buffer_free(port, buf->head); > buf->head = next; > } > -- > 2.5.0.457.gab17608 > -- 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] tty: fix data race in tty_buffer_flush
On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukovwrote: > tty_buffer_flush frees not acquired buffers. > As the result, for example, read of b->size in tty_buffer_free > can return garbage value which will lead to a huge buffer > hanging in the freelist. This is just the benignest > manifestation of freeing of a not acquired object. > If the object is passed to kfree, heap can be corrupted. > > Acquire visibility over the buffer before freeing it. > > The data race was found with KernelThreadSanitizer (KTSAN). Dmitry, Looks good. Thanks for splitting this from the other fix. Like Greg said, the patch needs revision info to help maintainers, et al track which is most current/relevant/etc. Typical format for $subject is something like: [PATCH v4] tty: fix data race in tty_buffer_flush Notes re: changes from other patch versions are added below the git changelog separator (example below). > Signed-off-by: Dmitry Vyukov > --- v4: Corrected commit log and patch revision notes v3: Added code comment re: paired smp barrier v2: Split from 'tty: fix data races on tty_buffer.commit' > drivers/tty/tty_buffer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c > index 5a3fa89..a2a8cd0 100644 > --- a/drivers/tty/tty_buffer.c > +++ b/drivers/tty/tty_buffer.c > @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct > tty_ldisc *ld) > atomic_inc(>priority); > > mutex_lock(>lock); > - while ((next = buf->head->next) != NULL) { > + /* paired w/ release in __tty_buffer_request_room; ensures there are > +* no pending memory accesses to the freed buffer > +*/ > + while ((next = smp_load_acquire(>head->next)) != NULL) { > tty_buffer_free(port, buf->head); > buf->head = next; > } > -- > 2.5.0.457.gab17608 > -- 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] tty: fix data race in tty_buffer_flush
On Tue, Sep 08, 2015 at 02:48:26PM +0200, Dmitry Vyukov wrote: > tty_buffer_flush frees not acquired buffers. > As the result, for example, read of b->size in tty_buffer_free > can return garbage value which will lead to a huge buffer > hanging in the freelist. This is just the benignest > manifestation of freeing of a not acquired object. > If the object is passed to kfree, heap can be corrupted. > > Acquire visibility over the buffer before freeing it. > > The data race was found with KernelThreadSanitizer (KTSAN). > > Signed-off-by: Dmitry Vyukov > --- > drivers/tty/tty_buffer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) What changed from the last version? Why should I take this one and not the other? What order do these go in? Please please please version your patches, and number them in a series so that I know what to apply, and in what order, and what changed between versions. Please read Documentation/SubmittingPatches for all the details about this. I've now dropped all of your patches from my queue, please resend them all properly. thanks, greg k-h -- 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] tty: fix data race in tty_buffer_flush
On Tue, Sep 08, 2015 at 02:48:26PM +0200, Dmitry Vyukov wrote: > tty_buffer_flush frees not acquired buffers. > As the result, for example, read of b->size in tty_buffer_free > can return garbage value which will lead to a huge buffer > hanging in the freelist. This is just the benignest > manifestation of freeing of a not acquired object. > If the object is passed to kfree, heap can be corrupted. > > Acquire visibility over the buffer before freeing it. > > The data race was found with KernelThreadSanitizer (KTSAN). > > Signed-off-by: Dmitry Vyukov> --- > drivers/tty/tty_buffer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) What changed from the last version? Why should I take this one and not the other? What order do these go in? Please please please version your patches, and number them in a series so that I know what to apply, and in what order, and what changed between versions. Please read Documentation/SubmittingPatches for all the details about this. I've now dropped all of your patches from my queue, please resend them all properly. thanks, greg k-h -- 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/