Re: [PATCH 0/3] Revert "virtio_net: rx enable premapped mode by default"
For the record, I got a chance to test these changes and confirmed that they resolved the issue for me when applied on 6.11-rc7. Tested-by: Darren Kenny Thanks, Darren. PS - I'll try get to looking at the other potential fix when I have time. On Tuesday, 2024-09-10 at 08:12:06 -04, Michael S. Tsirkin wrote: > On Fri, Sep 06, 2024 at 08:31:34PM +0800, Xuan Zhuo wrote: >> Regression: >> http://lore.kernel.org/all/8b20cc28-45a9-4643-8e87-ba164a540...@oracle.com >> >> I still think that the patch can fix the problem, I hope Darren can re-test >> it >> or give me more info. >> >> >> http://lore.kernel.org/all/20240820071913.68004-1-xuanz...@linux.alibaba.com >> >> If that can not work or Darren can not reply in time, Michael you can try >> this >> patch set. > > Just making sure netdev maintainers see this, this patch is for net. > >> Thanks. >> >> Xuan Zhuo (3): >> Revert "virtio_net: rx remove premapped failover code" >> Revert "virtio_net: big mode skip the unmap check" >> virtio_net: disable premapped mode by default >> >> drivers/net/virtio_net.c | 95 +++- >> 1 file changed, 46 insertions(+), 49 deletions(-) >> >> -- >> 2.32.0.3.g01195cf9f
Re: [PATCH net] virtio-net: fix overflow inside virtnet_rq_alloc
Hi,
Apologies, I've been OOTO for the best part of 2 weeks, I'll try get to
this as soon as I can, but playing catch-up right now.
Thanks,
Darren.
On Saturday, 2024-09-07 at 12:16:24 +09, Takero Funaki wrote:
> 2024年9月6日(金) 18:55 Michael S. Tsirkin :
>>
>> On Fri, Sep 06, 2024 at 05:46:02PM +0800, Xuan Zhuo wrote:
>> > On Fri, 6 Sep 2024 05:44:27 -0400, "Michael S. Tsirkin"
>> > wrote:
>> > > On Fri, Sep 06, 2024 at 05:25:36PM +0800, Xuan Zhuo wrote:
>> > > > On Fri, 6 Sep 2024 05:08:56 -0400, "Michael S. Tsirkin"
>> > > > wrote:
>> > > > > On Fri, Sep 06, 2024 at 04:53:38PM +0800, Xuan Zhuo wrote:
>> > > > > > On Fri, 6 Sep 2024 04:43:29 -0400, "Michael S. Tsirkin"
>> > > > > > wrote:
>> > > > > > > On Tue, Aug 20, 2024 at 03:19:13PM +0800, Xuan Zhuo wrote:
>> > > > > > > > leads to regression on VM with the sysctl value of:
>> > > > > > > >
>> > > > > > > > - net.core.high_order_alloc_disable=1
>> > > > > > > >
>> > > > > > > > which could see reliable crashes or scp failure (scp a file
>> > > > > > > > 100M in size
>> > > > > > > > to VM):
>> > > > > > > >
>> > > > > > > > The issue is that the virtnet_rq_dma takes up 16 bytes at the
>> > > > > > > > beginning
>> > > > > > > > of a new frag. When the frag size is larger than PAGE_SIZE,
>> > > > > > > > everything is fine. However, if the frag is only one page and
>> > > > > > > > the
>> > > > > > > > total size of the buffer and virtnet_rq_dma is larger than one
>> > > > > > > > page, an
>> > > > > > > > overflow may occur. In this case, if an overflow is possible,
>> > > > > > > > I adjust
>> > > > > > > > the buffer size. If net.core.high_order_alloc_disable=1, the
>> > > > > > > > maximum
>> > > > > > > > buffer size is 4096 - 16. If
>> > > > > > > > net.core.high_order_alloc_disable=0, only
>> > > > > > > > the first buffer of the frag is affected.
>> > > > > > > >
>> > > > > > > > Fixes: f9dac92ba908 ("virtio_ring: enable premapped mode
>> > > > > > > > whatever use_dma_api")
>> > > > > > > > Reported-by: "Si-Wei Liu"
>> > > > > > > > Closes:
>> > > > > > > > http://lore.kernel.org/all/8b20cc28-45a9-4643-8e87-ba164a540...@oracle.com
>> > > > > > > > Signed-off-by: Xuan Zhuo
>> > > > > > >
>> > > > > > >
>> > > > > > > Guys where are we going with this? We have a crasher right now,
>> > > > > > > if this is not fixed ASAP I'd have to revert a ton of
>> > > > > > > work Xuan Zhuo just did.
>> > > > > >
>> > > > > > I think this patch can fix it and I tested it.
>> > > > > > But Darren said this patch did not work.
>> > > > > > I need more info about the crash that Darren encountered.
>> > > > > >
>> > > > > > Thanks.
>> > > > >
>> > > > > So what are we doing? Revert the whole pile for now?
>> > > > > Seems to be a bit of a pity, but maybe that's the best we can do
>> > > > > for this release.
>> > > >
>> > > > @Jason Could you review this?
>> > > >
>> > > > I think this problem is clear, though I do not know why it did not work
>> > > > for Darren.
>> > > >
>> > > > Thanks.
>> > > >
>> > >
>> > > No regressions is a hard rule. If we can't figure out the regression
>> > > now, we should revert and you can try again for the next release.
>> >
>> > I see. I think I fixed it.
>> >
>> > Hope Darren can reply before you post the revert patches.
>> >
>> > Thanks.
>> >
>>
>> It's very rushed anyway. I posted the reverts, but as RFC for now.
>> You should post a debugging patch for Darren to help you figure
>> out what is going on.
>>
>>
>
> Hello,
>
> My issue [1], which bisected to the commit f9dac92ba908, was resolved
> after applying the patch on v6.11-rc6.
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=219154
>
> In my case, random crashes occur when receiving large data under heavy
> memory/IO load. Although the crash details differ, the memory
> corruption during data transfers is consistent.
>
> If Darren is unable to confirm the fix, would it be possible to
> consider merging this patch to close [1] instead?
>
> Thanks.
Re: [PATCH net] virtio-net: fix overflow inside virtnet_rq_alloc
Hi Michael,
On Tuesday, 2024-08-20 at 12:50:39 -04, Michael S. Tsirkin wrote:
> On Tue, Aug 20, 2024 at 03:19:13PM +0800, Xuan Zhuo wrote:
>> leads to regression on VM with the sysctl value of:
>>
>> - net.core.high_order_alloc_disable=1
>
>
>
>
>> which could see reliable crashes or scp failure (scp a file 100M in size
>> to VM):
>>
>> The issue is that the virtnet_rq_dma takes up 16 bytes at the beginning
>> of a new frag. When the frag size is larger than PAGE_SIZE,
>> everything is fine. However, if the frag is only one page and the
>> total size of the buffer and virtnet_rq_dma is larger than one page, an
>> overflow may occur. In this case, if an overflow is possible, I adjust
>> the buffer size. If net.core.high_order_alloc_disable=1, the maximum
>> buffer size is 4096 - 16. If net.core.high_order_alloc_disable=0, only
>> the first buffer of the frag is affected.
>>
>> Fixes: f9dac92ba908 ("virtio_ring: enable premapped mode whatever
>> use_dma_api")
>> Reported-by: "Si-Wei Liu"
>> Closes:
>> http://lore.kernel.org/all/8b20cc28-45a9-4643-8e87-ba164a540...@oracle.com
>> Signed-off-by: Xuan Zhuo
>
>
> Darren, could you pls test and confirm?
Unfortunately with this change I seem to still get a panic as soon as I start a
download using wget:
[ 144.055630] Kernel panic - not syncing: corrupted stack end detected inside
scheduler
[ 144.056249] CPU: 8 PID: 37894 Comm: sleep Kdump: loaded Not tainted
6.10.0-1.el8uek.x86_64 #2
[ 144.056850] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
1.16.0-4.module+el8.9.0+90173+a3f3e83a 04/01/2014
[ 144.057585] Call Trace:
[ 144.057791]
[ 144.057973] panic+0x347/0x370
[ 144.058223] schedule_debug.isra.0+0xfb/0x100
[ 144.058565] __schedule+0x58/0x6a0
[ 144.058838] ? refill_stock+0x26/0x50
[ 144.059120] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.059473] do_task_dead+0x42/0x50
[ 144.059752] do_exit+0x31e/0x4b0
[ 144.060011] ? __audit_syscall_entry+0xee/0x150
[ 144.060352] do_group_exit+0x30/0x80
[ 144.060633] __x64_sys_exit_group+0x18/0x20
[ 144.060946] do_syscall_64+0x8c/0x1c0
[ 144.061228] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.061570] ? __audit_filter_op+0xbe/0x140
[ 144.061873] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.062204] ? audit_reset_context+0x232/0x310
[ 144.062514] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.062851] ? syscall_exit_work+0x103/0x130
[ 144.063148] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.063473] ? syscall_exit_to_user_mode+0x77/0x220
[ 144.063813] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.064142] ? do_syscall_64+0xb9/0x1c0
[ 144.064411] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.064747] ? do_syscall_64+0xb9/0x1c0
[ 144.065018] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.065345] ? do_read_fault+0x109/0x1b0
[ 144.065628] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.065961] ? do_fault+0x1aa/0x2f0
[ 144.066212] ? handle_pte_fault+0x102/0x1a0
[ 144.066503] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.066836] ? __handle_mm_fault+0x5ed/0x710
[ 144.067137] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.067464] ? __count_memcg_events+0x72/0x110
[ 144.067779] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.068106] ? count_memcg_events.constprop.0+0x26/0x50
[ 144.068457] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.068788] ? handle_mm_fault+0xae/0x320
[ 144.069068] ? srso_alias_return_thunk+0x5/0xfbef5
[ 144.069395] ? do_user_addr_fault+0x34a/0x6b0
[ 144.069708] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 144.070049] RIP: 0033:0x7fc5524f9c66
[ 144.070307] Code: Unable to access opcode bytes at 0x7fc5524f9c3c.
[ 144.070720] RSP: 002b:7ffee052beb8 EFLAGS: 0246 ORIG_RAX:
00e7
[ 144.071214] RAX: ffda RBX: 7fc5527bb860 RCX: 7fc5524f9c66
[ 144.071684] RDX: RSI: 003c RDI:
[ 144.072146] RBP: R08: 00e7 R09: ff78
[ 144.072608] R10: 7ffee052bdef R11: 0246 R12: 7fc5527bb860
[ 144.073076] R13: 0002 R14: 7fc5527c4528 R15:
[ 144.073543]
[ 144.074780] Kernel Offset: 0x37c0 from 0x8100 (relocation
range: 0x8000-0xbfff)
Thanks,
Darren.
>> ---
>> drivers/net/virtio_net.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index c6af18948092..e5286a6da863 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -918,9 +918,6 @@ static void *virtnet_rq_alloc(struct receive_queue *rq,
>> u32 size, gfp_t gfp)
>> void *buf, *head;
>> dma_addr_t addr;
>>
>> -if (unlikely(!skb_page_frag_refill(size, alloc_frag, gfp)))
>> -return NULL;
>> -
>> head = page_address(alloc_frag->page);
>>
>> dma = head;
>> @@ -2421,6 +2418,9 @@ static int add_recvbuf_small(struct virtnet_info *vi,
>> str
Re: [PATCH net V2] vhost: correctly remove wait queue during poll failure
On Tue, Mar 27, 2018 at 08:50:52PM +0800, Jason Wang wrote:
We tried to remove vq poll from wait queue, but do not check whether
or not it was in a list before. This will lead double free. Fixing
this by switching to use vhost_poll_stop() which zeros poll->wqh after
removing poll from waitqueue to make sure it won't be freed twice.
Cc: Darren Kenny
Reported-by: syzbot+c0272972b01b872e6...@syzkaller.appspotmail.com
Fixes: 2b8b328b61c79 ("vhost_net: handle polling errors when setting backend")
Signed-off-by: Jason Wang
Reviewed-by: Darren Kenny
---
Changes from V1:
- tweak the commit log for to match the code
---
drivers/vhost/vhost.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d..5d5a9d9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -212,8 +212,7 @@ int vhost_poll_start(struct vhost_poll *poll, struct file
*file)
if (mask)
vhost_poll_wakeup(&poll->wait, 0, 0, poll_to_key(mask));
if (mask & EPOLLERR) {
- if (poll->wqh)
- remove_wait_queue(poll->wqh, &poll->wait);
+ vhost_poll_stop(poll);
ret = -EINVAL;
}
--
2.7.4
Re: [PATCH net] vhost: correctly remove wait queue during poll failure
Hi Jason,
On Tue, Mar 27, 2018 at 11:47:22AM +0800, Jason Wang wrote:
We tried to remove vq poll from wait queue, but do not check whether
or not it was in a list before. This will lead double free. Fixing
this by checking poll->wqh to make sure it was in a list.
This text seems at odds with the code below, instead of checking
poll-whq, you are removing that check...
Maybe the text needs rewording?
Thanks,
Darren.
Reported-by: syzbot+c0272972b01b872e6...@syzkaller.appspotmail.com
Fixes: 2b8b328b61c79 ("vhost_net: handle polling errors when setting backend")
Signed-off-by: Jason Wang
---
drivers/vhost/vhost.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d..5d5a9d9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -212,8 +212,7 @@ int vhost_poll_start(struct vhost_poll *poll, struct file
*file)
if (mask)
vhost_poll_wakeup(&poll->wait, 0, 0, poll_to_key(mask));
if (mask & EPOLLERR) {
- if (poll->wqh)
- remove_wait_queue(poll->wqh, &poll->wait);
+ vhost_poll_stop(poll);
ret = -EINVAL;
}
--
2.7.4
Re: [PATCH] vhost: fix vhost ioctl signature to build with clang
On Wed, Mar 14, 2018 at 10:05:06AM -0700, Sonny Rao wrote:
Clang is particularly anal about signed vs unsigned comparisons and
doesn't like the fact that some ioctl numbers set the MSB, so we get
this error when trying to build vhost on aarch64:
drivers/vhost/vhost.c:1400:7: error: overflow converting case value to
switch condition type (3221794578 to 18446744072636378898)
[-Werror, -Wswitch]
case VHOST_GET_VRING_BASE:
3221794578 is 0xC008AF12 in hex
18446744072636378898 is 0xC008AF12 in hex
Fix this by using unsigned ints in the function signature for
vhost_vring_ioctl().
Signed-off-by: Sonny Rao
Reviewed-by: Darren Kenny
All the other callers of this function already appear to assume that
it is an unsigned int.
Thanks,
Darren.
---
drivers/vhost/vhost.c | 2 +-
drivers/vhost/vhost.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d5c8b4..5316319d84081 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1337,7 +1337,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct
vhost_memory __user *m)
return -EFAULT;
}
-long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
+long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user
*argp)
{
struct file *eventfp, *filep = NULL;
bool pollstart = false, pollstop = false;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index ac4b6056f19ae..d8ee85ae8fdcc 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -45,7 +45,7 @@ void vhost_poll_stop(struct vhost_poll *poll);
void vhost_poll_flush(struct vhost_poll *poll);
void vhost_poll_queue(struct vhost_poll *poll);
void vhost_work_flush(struct vhost_dev *dev, struct vhost_work *work);
-long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
+long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user
*argp);
struct vhost_log {
u64 addr;
@@ -177,7 +177,7 @@ void vhost_dev_reset_owner(struct vhost_dev *, struct
vhost_umem *);
void vhost_dev_cleanup(struct vhost_dev *);
void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, void __user *argp);
-long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp);
+long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user
*argp);
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
--
2.13.5
