Message ID | ZFI9bHr1o2Cvdebp@gondor.apana.org.au |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1235480vqo; Wed, 3 May 2023 04:01:56 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6xdCD98MJXxtsMqH7g4PDHJNTVeJfzykb9Ber7VwxBPUOdyUjl1a4roQTx2SEppRF4Zi43 X-Received: by 2002:a17:902:ecc3:b0:1aa:f173:2892 with SMTP id a3-20020a170902ecc300b001aaf1732892mr1904501plh.57.1683111716063; Wed, 03 May 2023 04:01:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683111716; cv=none; d=google.com; s=arc-20160816; b=1K0PcdorjUJAo6rz/qZYn5xNc9xtyfWHu5ta9GBFxrT0DLYpkR8ymfdtoz4ZmY3pvA iCyEruHB6HyubUxyCj9DLwsynaNpdW/bL4j2EBpcjT/t+8j4IndQaEVedQAb3cvnhwAj BZjaAuQKl4Gol5sO8f8q7NnJ2XsQSGclewNTcx0QTc82R/4eGGxFMExNzEAsKoY3eDci 1huBgwpnRfsv+WAo4whwy4CmjUwrU1B7brs3KoxySWyJ5zbeujplF5u46/9lB3JPfdBa 6tOO/01Z6r/9jJj/9853Xbs5AV+3tQbCixl7T1Z3VvfQVwD1ffOs3eZcowTPsdsMfa/B tlQA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=1NR6WOk50sqTpuIxuyibUlMPhQHRyvxLBG2ysBmLsKs=; b=jgjvwY0ELmOlegvAEl85Zl1L2l2h5AFISB25PT4b0UYNv4ytFflgT6jtSJOLn2MO4a LqClT20jXG4NEry61d3/FNf9WIMvgL78DLAISxtt2KffMrgjZkMaZccH+HuHilqcLLRI 4DUhKXalTYXX89oVlTwQ+QyUGUGkZXlGUZkc7ywlCnBWOhkkPYIwLme2yieAo8V7U8yY Oltkvr0WVIX211k2TPijtB8Ng1hfYF84oxDI12qAW3vzomqdprK5iz3UieoV7/OoffPx nZna+X2criOFrgYU6s0yJsmNsexMQTFBMfu+K606mNDxgYrhbMR8HUrBzHi5orM0oczf 1yrw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ha11-20020a17090af3cb00b0024e0cb4a986si5849380pjb.89.2023.05.03.04.01.39; Wed, 03 May 2023 04:01:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229630AbjECKzB (ORCPT <rfc822;heyuhang3455@gmail.com> + 99 others); Wed, 3 May 2023 06:55:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56862 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229569AbjECKy7 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 3 May 2023 06:54:59 -0400 Received: from 167-179-156-38.a7b39c.syd.nbn.aussiebb.net (167-179-156-38.a7b39c.syd.nbn.aussiebb.net [167.179.156.38]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1988849DE; Wed, 3 May 2023 03:54:57 -0700 (PDT) Received: from loth.rohan.me.apana.org.au ([192.168.167.2]) by formenos.hmeau.com with smtp (Exim 4.94.2 #2 (Debian)) id 1puA83-004ibe-9X; Wed, 03 May 2023 18:54:37 +0800 Received: by loth.rohan.me.apana.org.au (sSMTP sendmail emulation); Wed, 03 May 2023 18:54:36 +0800 Date: Wed, 3 May 2023 18:54:36 +0800 From: Herbert Xu <herbert@gondor.apana.org.au> To: Dmitry Vyukov <dvyukov@google.com> Cc: syzbot <syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com>, davem@davemloft.net, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, olivia@selenic.com, syzkaller-bugs@googlegroups.com, Jason Wang <jasowang@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Rusty Russell <rusty@rustcorp.com.au> Subject: [PATCH] hwrng: virtio - Fix race on data_avail and actual data Message-ID: <ZFI9bHr1o2Cvdebp@gondor.apana.org.au> References: <00000000000050327205f9d993b2@google.com> <CACT4Y+awU85RHZjf3+_85AvJOHghoOhH3c9E-70p+a=FrRDYkg@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <CACT4Y+awU85RHZjf3+_85AvJOHghoOhH3c9E-70p+a=FrRDYkg@mail.gmail.com> X-Spam-Status: No, score=2.7 required=5.0 tests=BAYES_00,HELO_DYNAMIC_IPADDR2, RDNS_DYNAMIC,SPF_HELO_NONE,SPF_PASS,TVD_RCVD_IP,T_SCC_BODY_TEXT_LINE, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 X-Spam-Level: ** X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764870550648685851?= X-GMAIL-MSGID: =?utf-8?q?1764870550648685851?= |
Series |
hwrng: virtio - Fix race on data_avail and actual data
|
|
Commit Message
Herbert Xu
May 3, 2023, 10:54 a.m. UTC
On Fri, Apr 21, 2023 at 04:52:13PM +0200, Dmitry Vyukov wrote: > > Here this: > > size = min_t(unsigned int, size, vi->data_avail); > memcpy(buf, vi->data + vi->data_idx, size); > vi->data_idx += size; > vi->data_avail -= size; > > runs concurrently with: > > if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > return; > vi->data_idx = 0; > > I did not fully grasp how/where vi->data is populated, but it looks > like it can lead to use of uninit/stale random data, or even to out of > bounds access, say if vi->data_avail is already updated, but > vi->data_idx is not yet reset to 0. Then concurrent reading will read > not where it's supposed to read. Yes this is a real race. This bug appears to have been around forever. ---8<--- The virtio rng device kicks off a new entropy request whenever the data available reaches zero. When a new request occurs at the end of a read operation, that is, when the result of that request is only needed by the next reader, then there is a race between the writing of the new data and the next reader. This is because there is no synchronisation whatsoever between the writer and the reader. Fix this by writing data_avail with smp_store_release and reading it with smp_load_acquire when we first enter read. The subsequent reads are safe because they're either protected by the first load acquire, or by the completion mechanism. Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Comments
Hi, On 5/3/23 11:54, Herbert Xu wrote: > On Fri, Apr 21, 2023 at 04:52:13PM +0200, Dmitry Vyukov wrote: >> >> Here this: >> >> size = min_t(unsigned int, size, vi->data_avail); >> memcpy(buf, vi->data + vi->data_idx, size); >> vi->data_idx += size; >> vi->data_avail -= size; >> >> runs concurrently with: >> >> if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) >> return; >> vi->data_idx = 0; >> >> I did not fully grasp how/where vi->data is populated, but it looks >> like it can lead to use of uninit/stale random data, or even to out of >> bounds access, say if vi->data_avail is already updated, but >> vi->data_idx is not yet reset to 0. Then concurrent reading will read >> not where it's supposed to read. > > Yes this is a real race. This bug appears to have been around > forever. > > ---8<--- > The virtio rng device kicks off a new entropy request whenever the > data available reaches zero. When a new request occurs at the end > of a read operation, that is, when the result of that request is > only needed by the next reader, then there is a race between the > writing of the new data and the next reader. > > This is because there is no synchronisation whatsoever between the > writer and the reader. > > Fix this by writing data_avail with smp_store_release and reading > it with smp_load_acquire when we first enter read. The subsequent > reads are safe because they're either protected by the first load > acquire, or by the completion mechanism. > > Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb Please add the dashboard link if applying as searching for the syzbot ID rarely gives meaningful results. Cheers, ta > Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c > index f7690e0f92ed..e41a84e6b4b5 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation > */ > > +#include <asm/barrier.h> > #include <linux/err.h> > #include <linux/hw_random.h> > #include <linux/scatterlist.h> > @@ -37,13 +38,13 @@ struct virtrng_info { > static void random_recv_done(struct virtqueue *vq) > { > struct virtrng_info *vi = vq->vdev->priv; > + unsigned int len; > > /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */ > - if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > + if (!virtqueue_get_buf(vi->vq, &len)) > return; > > - vi->data_idx = 0; > - > + smp_store_release(&vi->data_avail, len); > complete(&vi->have_data); > } > > @@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) > struct scatterlist sg; > > reinit_completion(&vi->have_data); > - vi->data_avail = 0; > vi->data_idx = 0; > > sg_init_one(&sg, vi->data, sizeof(vi->data)); > @@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) > read = 0; > > /* copy available data */ > - if (vi->data_avail) { > + if (smp_load_acquire(&vi->data_avail)) { > chunk = copy_data(vi, buf, size); > size -= chunk; > read += chunk;
On Wed, May 03, 2023 at 06:54:36PM +0800, Herbert Xu wrote: > On Fri, Apr 21, 2023 at 04:52:13PM +0200, Dmitry Vyukov wrote: > > > > Here this: > > > > size = min_t(unsigned int, size, vi->data_avail); > > memcpy(buf, vi->data + vi->data_idx, size); > > vi->data_idx += size; > > vi->data_avail -= size; > > > > runs concurrently with: > > > > if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > > return; > > vi->data_idx = 0; > > > > I did not fully grasp how/where vi->data is populated, but it looks > > like it can lead to use of uninit/stale random data, or even to out of > > bounds access, say if vi->data_avail is already updated, but > > vi->data_idx is not yet reset to 0. Then concurrent reading will read > > not where it's supposed to read. > > Yes this is a real race. This bug appears to have been around > forever. > > ---8<--- > The virtio rng device kicks off a new entropy request whenever the > data available reaches zero. When a new request occurs at the end > of a read operation, that is, when the result of that request is > only needed by the next reader, then there is a race between the > writing of the new data and the next reader. > > This is because there is no synchronisation whatsoever between the > writer and the reader. > > Fix this by writing data_avail with smp_store_release and reading > it with smp_load_acquire when we first enter read. The subsequent > reads are safe because they're either protected by the first load > acquire, or by the completion mechanism. > > Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com > Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.") > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c > index f7690e0f92ed..e41a84e6b4b5 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation > */ > > +#include <asm/barrier.h> > #include <linux/err.h> > #include <linux/hw_random.h> > #include <linux/scatterlist.h> > @@ -37,13 +38,13 @@ struct virtrng_info { > static void random_recv_done(struct virtqueue *vq) > { > struct virtrng_info *vi = vq->vdev->priv; > + unsigned int len; > > /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */ > - if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > + if (!virtqueue_get_buf(vi->vq, &len)) > return; > > - vi->data_idx = 0; > - On the surface of it, it looks like you removed this store which isn't described in the commit log. I do not, offhand, remember why we stored 0 in data_idx here when we also zero it in request_entropy. It was added with commit 5c8e933050044d6dd2a000f9a5756ae73cbe7c44 Author: Laurent Vivier <lvivier@redhat.com> Date: Thu Oct 28 12:11:10 2021 +0200 hwrng: virtio - don't waste entropy if we don't use all the entropy available in the buffer, keep it and use it later. Signed-off-by: Laurent Vivier <lvivier@redhat.com> Link: https://lore.kernel.org/r/20211028101111.128049-4-lvivier@redhat.com Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > + smp_store_release(&vi->data_avail, len); > complete(&vi->have_data); > } > > @@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) > struct scatterlist sg; > > reinit_completion(&vi->have_data); > - vi->data_avail = 0; > vi->data_idx = 0; > > sg_init_one(&sg, vi->data, sizeof(vi->data)); > @@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) > read = 0; > > /* copy available data */ > - if (vi->data_avail) { > + if (smp_load_acquire(&vi->data_avail)) { > chunk = copy_data(vi, buf, size); > size -= chunk; > read += chunk; > -- > Email: Herbert Xu <herbert@gondor.apana.org.au> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Wed, May 03, 2023 at 12:19:30PM +0100, Tudor Ambarus wrote: > > > Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > > Please add the dashboard link if applying as searching for the syzbot ID > rarely gives meaningful results. The syzbot ID is already present in the in the Reported-by tag. There is no reason to clutter up the commit message with redundant information. Cheers,
On 5/4/23 04:55, Herbert Xu wrote: > On Wed, May 03, 2023 at 12:19:30PM +0100, Tudor Ambarus wrote: >> >>> Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com >> >> Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb >> >> Please add the dashboard link if applying as searching for the syzbot ID >> rarely gives meaningful results. > > The syzbot ID is already present in the in the Reported-by tag. > There is no reason to clutter up the commit message with redundant > information. > As you prefer. Theodore Ts'o encourages to add a dashboard link, here's his reasoning: https://github.com/google/syzkaller/issues/3393#issuecomment-1347476434 Cheers, ta
On Thu, May 04, 2023 at 09:10:43AM +0100, Tudor Ambarus wrote: > > The syzbot ID is already present in the in the Reported-by tag. > > There is no reason to clutter up the commit message with redundant > > information. > > As you prefer. Theodore Ts'o encourages to add a dashboard link, here's > his reasoning: > https://github.com/google/syzkaller/issues/3393#issuecomment-1347476434 The reason why I've requested having both the Link and Reported-by is because you don't know the secret incantation: s;Reported-by: syzbot\+\([0-9a-z]+\)@syzkaller.appspotmail.com;https://syzkaller.appspotmail.com/extid?=\1; ... you can't easily get from a "Reported-by:" e-mail address to a URL link that will actually get you to the syzkaller page. What I used to do was to go to https://groups.google.com/g/syzkaller-bugs and then enter into the Google Groups searech box: Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com which is a ***super*** clunky way to get to the syzkaller page. What would be nice is if there was an easy way that didn't rely on kernel developers knowing the internal URL structure of Syzbot to be able to enter the Reported-by link on some convenient web page, perhaps in a search box found in the front page of https://syzkaller.appspot.com, and be able to find the syzbot report web page that way. Since that doesn't exist today, I include both the Reported-by: and Link: in my commit descriptions, out of consideration to the reviewer who might want to be able to find the Syzbot page and don't know the secret trick to calculate the URL from the Reported-by: e-mail address. Another gotcha with Syzbot is that there are two id's, the "extid" and the "id" which makes thing ***super*** confusing. For example, both of these URL's go the same Syzbot report: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb https://syzkaller.appspot.com/bug?id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7 The Reported-by e-mail address uses the extid. So for example, this case, it would be syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com. However, all of the links in the Syzbot web pages use the id form of the URL. So if you were browsing the syzbot reports assigned to the crypto subsystem via https://syzkaller.appspot.com/upstream/s/crypto, you would find the id-style link, and then the commit fixing the bug might have something like this: Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7 In that case, there is no (obvious) relationship between the hex string found in the Reported-by line and the Link line. One additional unfortunate fallout from syzbot having an "extid" and "id", is that depending on how the syzbot entry initially found by the contributor sending in a patch to address a syzbot report, either URL can be found in mailing list archives. So if you search for "extid=726dc8c62c3536431ceb" you won't find references to "id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7" even though they are both referring to same Syzbot report. <<< sigh >>>> As they say, the hardest problem to solve in the C.S. world is naming, and syzbot has two names for every single syzbot report, and both are exposed to the poor user. :-( - Ted
On Fri, 5 May 2023 at 06:01, Theodore Ts'o <tytso@mit.edu> wrote: > > On Thu, May 04, 2023 at 09:10:43AM +0100, Tudor Ambarus wrote: > > > The syzbot ID is already present in the in the Reported-by tag. > > > There is no reason to clutter up the commit message with redundant > > > information. > > > > As you prefer. Theodore Ts'o encourages to add a dashboard link, here's > > his reasoning: > > https://github.com/google/syzkaller/issues/3393#issuecomment-1347476434 > > The reason why I've requested having both the Link and Reported-by is > because you don't know the secret incantation: > > s;Reported-by: syzbot\+\([0-9a-z]+\)@syzkaller.appspotmail.com;https://syzkaller.appspotmail.com/extid?=\1; > > ... you can't easily get from a "Reported-by:" e-mail address to a URL > link that will actually get you to the syzkaller page. What I used to > do was to go to https://groups.google.com/g/syzkaller-bugs and then > enter into the Google Groups searech box: > > Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com > > which is a ***super*** clunky way to get to the syzkaller page. What > would be nice is if there was an easy way that didn't rely on kernel > developers knowing the internal URL structure of Syzbot to be able to > enter the Reported-by link on some convenient web page, perhaps in a > search box found in the front page of https://syzkaller.appspot.com, > and be able to find the syzbot report web page that way. > > Since that doesn't exist today, I include both the Reported-by: and > Link: in my commit descriptions, out of consideration to the reviewer > who might want to be able to find the Syzbot page and don't know the > secret trick to calculate the URL from the Reported-by: e-mail > address. > > > Another gotcha with Syzbot is that there are two id's, the "extid" and > the "id" which makes thing ***super*** confusing. For example, both > of these URL's go the same Syzbot report: > > https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > https://syzkaller.appspot.com/bug?id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7 > > The Reported-by e-mail address uses the extid. So for example, this > case, it would be syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com. > > However, all of the links in the Syzbot web pages use the id form of > the URL. So if you were browsing the syzbot reports assigned to the > crypto subsystem via https://syzkaller.appspot.com/upstream/s/crypto, > you would find the id-style link, and then the commit fixing the bug > might have something like this: > > Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7 > > In that case, there is no (obvious) relationship between the hex > string found in the Reported-by line and the Link line. > > > One additional unfortunate fallout from syzbot having an "extid" and > "id", is that depending on how the syzbot entry initially found by the > contributor sending in a patch to address a syzbot report, either URL > can be found in mailing list archives. So if you search for > "extid=726dc8c62c3536431ceb" you won't find references to > "id=eec08eb3763c9ec749fd565e70cfe6e485af7ed7" even though they are > both referring to same Syzbot report. > > <<< sigh >>>> As they say, the hardest problem to solve in the > C.S. world is naming, and syzbot has two names for every single syzbot > report, and both are exposed to the poor user. :-( A link like this may work for syzbot instead of the Reported-by tag (may work out of the box, but need to double check if we start to use this): Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb Or similarly this may work: Reported-by: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb I think the parsing code mostly looks for the hash. This was proposed, but people said that they need links to lore and don't want links to syzkaller dashboard. So this was rejected at the time.
On Mon, May 08, 2023 at 07:33:39AM +0200, Dmitry Vyukov wrote: > A link like this may work for syzbot instead of the Reported-by tag > (may work out of the box, but need to double check if we start to use > this): > > Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > > Or similarly this may work: > > Reported-by: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > I think the parsing code mostly looks for the hash. > > This was proposed, but people said that they need links to lore and > don't want links to syzkaller dashboard. So this was rejected at the > time. I think the "Reported-by: " line should continue to contain an e-mail, since that way "git send-email" will automatically include a Cc: to the mailing list address so that the syzbot page for the report will contain a link to the page. What *would* be useful would be a search box on the top-level https://syzkaller.appspot.com where you could either enter an e-mail address like: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com or the syzbot report title e.g.: KCSAN: data-race in random_recv_done / virtio_read (3) or just a function name: sys_quotactl_fd The search box could just push the text to google.com with "site:syzkaller.appspot.com", which should mostly do the right thing. Also, it would also be nice if all of the URL links on the syzkaller.appspot.com used the id form of the URL. That is, to use https://syzkaller.appspot.com/bug?extid=6c73bd34311ee489dbf5 instead of: https://syzkaller.appspot.com/bug?id=32c54626e170a6b327ca2c8ae4c1aea666a8c20b The extid form of the URL is shorter, and having a consistency so that the primary URL is the extid would reduce confusion. The web site will need to continue to support the id form of the URL since there are quite a few of those URL's in mailing list archives and git commit descriptions. It also would be useful if there was a way to translate from the extid hash to the id hash, so that it's possible to search for the extid and id forms of the URL --- since the URL aliasing means that for a developer trying to do code archeology and web searches, that we need to search for both URL forms for past syzbot reports. (But if we can avoid the aliasing confusion moving forward, that would be **really** nice.) Cheers, - Ted
Hi Ted, On Mon, May 8, 2023 at 11:06 AM Theodore Ts'o <tytso@mit.edu> wrote: > > On Mon, May 08, 2023 at 07:33:39AM +0200, Dmitry Vyukov wrote: > > A link like this may work for syzbot instead of the Reported-by tag > > (may work out of the box, but need to double check if we start to use > > this): > > > > Link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > > > > Or similarly this may work: > > > > Reported-by: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb > > I think the parsing code mostly looks for the hash. > > > > This was proposed, but people said that they need links to lore and > > don't want links to syzkaller dashboard. So this was rejected at the > > time. > > I think the "Reported-by: " line should continue to contain an e-mail, > since that way "git send-email" will automatically include a Cc: to > the mailing list address so that the syzbot page for the report will > contain a link to the page. > > What *would* be useful would be a search box on the top-level > https://syzkaller.appspot.com where you could either enter an e-mail > address like: > > syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com > > or the syzbot report title e.g.: > > KCSAN: data-race in random_recv_done / virtio_read (3) > > or just a function name: > > sys_quotactl_fd > > The search box could just push the text to google.com with > "site:syzkaller.appspot.com", which should mostly do the right thing. Thanks for the suggestion! I've filed https://github.com/google/syzkaller/issues/3892 > > Also, it would also be nice if all of the URL links on the > syzkaller.appspot.com used the id form of the URL. That is, to use > > https://syzkaller.appspot.com/bug?extid=6c73bd34311ee489dbf5 > > instead of: > > https://syzkaller.appspot.com/bug?id=32c54626e170a6b327ca2c8ae4c1aea666a8c20b > > The extid form of the URL is shorter, and having a consistency so that > the primary URL is the extid would reduce confusion. The web site > will need to continue to support the id form of the URL since there > are quite a few of those URL's in mailing list archives and git commit > descriptions. > > It also would be useful if there was a way to translate from the extid > hash to the id hash, so that it's possible to search for the extid and > id forms of the URL --- since the URL aliasing means that for a > developer trying to do code archeology and web searches, that we need > to search for both URL forms for past syzbot reports. (But if we can > avoid the aliasing confusion moving forward, that would be **really** > nice.) I've just sent a PR [1] so that URLs from bug lists on the web dashboard use the extid= instead of the id= parameter. Hopefully this will reduce the confusion. [1] https://github.com/google/syzkaller/pull/3891
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index f7690e0f92ed..e41a84e6b4b5 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -4,6 +4,7 @@ * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation */ +#include <asm/barrier.h> #include <linux/err.h> #include <linux/hw_random.h> #include <linux/scatterlist.h> @@ -37,13 +38,13 @@ struct virtrng_info { static void random_recv_done(struct virtqueue *vq) { struct virtrng_info *vi = vq->vdev->priv; + unsigned int len; /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */ - if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) + if (!virtqueue_get_buf(vi->vq, &len)) return; - vi->data_idx = 0; - + smp_store_release(&vi->data_avail, len); complete(&vi->have_data); } @@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) struct scatterlist sg; reinit_completion(&vi->have_data); - vi->data_avail = 0; vi->data_idx = 0; sg_init_one(&sg, vi->data, sizeof(vi->data)); @@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) read = 0; /* copy available data */ - if (vi->data_avail) { + if (smp_load_acquire(&vi->data_avail)) { chunk = copy_data(vi, buf, size); size -= chunk; read += chunk;