2023-04-21 14:59:19

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [syzbot] [crypto?] KCSAN: data-race in random_recv_done / virtio_read (3)

On Fri, 21 Apr 2023 at 16:36, syzbot
<[email protected]> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 2faac9a98f01 Merge tag 'keys-fixes-20230321' of git://git...
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=1113f21cc80000
> kernel config: https://syzkaller.appspot.com/x/.config?x=3eb0bb0ae89a5345
> dashboard link: https://syzkaller.appspot.com/bug?extid=726dc8c62c3536431ceb
> compiler: Debian clang version 15.0.7, GNU ld (GNU Binutils for Debian) 2.35.2
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/571c9c5a3db2/disk-2faac9a9.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/a051e3d7c495/vmlinux-2faac9a9.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/ff5ec0d6e37d/bzImage-2faac9a9.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: [email protected]

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.



> ==================================================================
> BUG: KCSAN: data-race in random_recv_done / virtio_read
>
> read to 0xffff8881019054ec of 4 bytes by task 14079 on cpu 0:
> copy_data drivers/char/hw_random/virtio-rng.c:70 [inline]
> virtio_read+0xc3/0x3f0 drivers/char/hw_random/virtio-rng.c:92
> rng_get_data drivers/char/hw_random/core.c:197 [inline]
> rng_dev_read+0x1a7/0x5e0 drivers/char/hw_random/core.c:234
> vfs_read+0x192/0x560 fs/read_write.c:468
> ksys_read+0xeb/0x1a0 fs/read_write.c:613
> __do_sys_read fs/read_write.c:623 [inline]
> __se_sys_read fs/read_write.c:621 [inline]
> __x64_sys_read+0x42/0x50 fs/read_write.c:621
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> write to 0xffff8881019054ec of 4 bytes by interrupt on cpu 1:
> random_recv_done+0x62/0x90 drivers/char/hw_random/virtio-rng.c:45
> vring_interrupt+0x150/0x170 drivers/virtio/virtio_ring.c:2491
> __handle_irq_event_percpu+0x91/0x490 kernel/irq/handle.c:158
> handle_irq_event_percpu kernel/irq/handle.c:193 [inline]
> handle_irq_event+0x64/0xf0 kernel/irq/handle.c:210
> handle_edge_irq+0x17f/0x5a0 kernel/irq/chip.c:819
> generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
> handle_irq arch/x86/kernel/irq.c:231 [inline]
> __common_interrupt+0x64/0x100 arch/x86/kernel/irq.c:250
> common_interrupt+0x49/0xc0 arch/x86/kernel/irq.c:240
> asm_common_interrupt+0x26/0x40 arch/x86/include/asm/idtentry.h:636
>
> value changed: 0x00000000 -> 0x00000040
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 14077 Comm: syz-executor.2 Not tainted 6.3.0-rc3-syzkaller-00016-g2faac9a98f01 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/02/2023
> ==================================================================
> ==================================================================
> BUG: KCSAN: data-race in detach_buf_split / virtqueue_add
>
> read to 0xffff888101a76950 of 4 bytes by task 14131 on cpu 0:
> virtqueue_add_split drivers/virtio/virtio_ring.c:553 [inline]
> virtqueue_add+0x4b9/0x2130 drivers/virtio/virtio_ring.c:2117
> virtqueue_add_inbuf+0x53/0x80 drivers/virtio/virtio_ring.c:2196
> request_entropy drivers/char/hw_random/virtio-rng.c:61 [inline]
> copy_data drivers/char/hw_random/virtio-rng.c:74 [inline]
> virtio_read+0x1c5/0x3f0 drivers/char/hw_random/virtio-rng.c:92
> rng_get_data drivers/char/hw_random/core.c:197 [inline]
> rng_dev_read+0x1a7/0x5e0 drivers/char/hw_random/core.c:234
> vfs_read+0x192/0x560 fs/read_write.c:468
> ksys_read+0xeb/0x1a0 fs/read_write.c:613
> __do_sys_read fs/read_write.c:623 [inline]
> __se_sys_read fs/read_write.c:621 [inline]
> __x64_sys_read+0x42/0x50 fs/read_write.c:621
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> read-write to 0xffff888101a76950 of 4 bytes by interrupt on cpu 1:
> detach_buf_split+0x2fc/0x570 drivers/virtio/virtio_ring.c:757
> virtqueue_get_buf_ctx_split drivers/virtio/virtio_ring.c:835 [inline]
> virtqueue_get_buf_ctx+0x3c8/0x5c0 drivers/virtio/virtio_ring.c:2311
> virtqueue_get_buf+0x1f/0x30 drivers/virtio/virtio_ring.c:2317
> random_recv_done+0x4c/0x90 drivers/char/hw_random/virtio-rng.c:42
> vring_interrupt+0x150/0x170 drivers/virtio/virtio_ring.c:2491
> __handle_irq_event_percpu+0x91/0x490 kernel/irq/handle.c:158
> handle_irq_event_percpu kernel/irq/handle.c:193 [inline]
> handle_irq_event+0x64/0xf0 kernel/irq/handle.c:210
> handle_edge_irq+0x17f/0x5a0 kernel/irq/chip.c:819
> generic_handle_irq_desc include/linux/irqdesc.h:158 [inline]
> handle_irq arch/x86/kernel/irq.c:231 [inline]
> __common_interrupt+0x64/0x100 arch/x86/kernel/irq.c:250
> common_interrupt+0x9e/0xc0 arch/x86/kernel/irq.c:240
> asm_common_interrupt+0x26/0x40 arch/x86/include/asm/idtentry.h:636
> xas_find+0x10a/0x3f0
> find_get_entry mm/filemap.c:2008 [inline]
> filemap_get_folios+0xa4/0x3f0 mm/filemap.c:2174
> mpage_map_and_submit_buffers fs/ext4/inode.c:2358 [inline]
> mpage_map_and_submit_extent fs/ext4/inode.c:2513 [inline]
> ext4_do_writepages+0x1017/0x2140 fs/ext4/inode.c:2876
> ext4_writepages+0x127/0x250 fs/ext4/inode.c:2964
> do_writepages+0x1c5/0x340 mm/page-writeback.c:2551
> filemap_fdatawrite_wbc+0xdb/0xf0 mm/filemap.c:390
> __filemap_fdatawrite_range mm/filemap.c:423 [inline]
> __filemap_fdatawrite mm/filemap.c:429 [inline]
> filemap_flush+0x95/0xc0 mm/filemap.c:456
> ext4_alloc_da_blocks+0x50/0x130 fs/ext4/inode.c:3218
> ext4_release_file+0x5f/0x1c0 fs/ext4/file.c:158
> __fput+0x245/0x570 fs/file_table.c:321
> ____fput+0x15/0x20 fs/file_table.c:349
> task_work_run+0x123/0x160 kernel/task_work.c:179
> exit_task_work include/linux/task_work.h:38 [inline]
> do_exit+0x600/0x1710 kernel/exit.c:869
> do_group_exit+0x101/0x150 kernel/exit.c:1019
> get_signal+0xea9/0xfe0 kernel/signal.c:2859
> arch_do_signal_or_restart+0x89/0x2b0 arch/x86/kernel/signal.c:306
> exit_to_user_mode_loop+0x6d/0xe0 kernel/entry/common.c:168
> exit_to_user_mode_prepare+0x6a/0xa0 kernel/entry/common.c:203
> irqentry_exit_to_user_mode+0x9/0x20 kernel/entry/common.c:309
> irqentry_exit+0x12/0x40 kernel/entry/common.c:412
> exc_general_protection+0x339/0x4c0 arch/x86/kernel/traps.c:728
> asm_exc_general_protection+0x26/0x30 arch/x86/include/asm/idtentry.h:564
>
> value changed: 0x00000001 -> 0x00000000
>
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 14098 Comm: syz-executor.1 Not tainted 6.3.0-rc3-syzkaller-00016-g2faac9a98f01 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/02/2023
> ==================================================================
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at [email protected].
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.


2023-05-03 11:01:41

by Herbert Xu

[permalink] [raw]
Subject: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

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: [email protected]
Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.")
Signed-off-by: Herbert Xu <[email protected]>

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;
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2023-05-03 11:27:28

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

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: [email protected]

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 <[email protected]>
>
> 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;

2023-05-04 08:16:13

by Tudor Ambarus

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data



On 5/4/23 04:55, Herbert Xu wrote:
> On Wed, May 03, 2023 at 12:19:30PM +0100, Tudor Ambarus wrote:
>>
>>> Reported-by: [email protected]
>>
>> 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

2023-05-05 04:16:31

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

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: [email protected]

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 [email protected].

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: [email protected]
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

2023-05-08 05:39:02

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

On Fri, 5 May 2023 at 06:01, Theodore Ts'o <[email protected]> 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: [email protected]
>
> 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 [email protected].
>
> 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: [email protected]
> 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.

2023-05-08 09:07:03

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

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:

[email protected]

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

2023-05-11 15:18:06

by Aleksandr Nogikh

[permalink] [raw]
Subject: Re: [PATCH] hwrng: virtio - Fix race on data_avail and actual data

Hi Ted,

On Mon, May 8, 2023 at 11:06 AM Theodore Ts'o <[email protected]> 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:
>
> [email protected]
>
> 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

--
Aleksandr

>
> Cheers,
>
> - Ted