* Re: [PATCH net] net: genl: fix error path memory leak in policy dumping
From: Jakub Kicinski @ 2022-08-16 1:23 UTC (permalink / raw)
To: davem; +Cc: netdev, edumazet, pabeni, johannes.berg,
syzbot+dc54d9ba8153b216cae0
In-Reply-To: <20220815182021.48925-1-kuba@kernel.org>
On Mon, 15 Aug 2022 11:20:21 -0700 Jakub Kicinski wrote:
> If construction of the array of policies fails when recording
> non-first policy we need to unwind.
>
> Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
> Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
syzbot says still pooped, indeed there's more leaks in
netlink_policy_dump_add_policy() itself. v2 coming...
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: Add support for writing to nf_conn:mark
From: kernel test robot @ 2022-08-16 1:15 UTC (permalink / raw)
To: Daniel Xu, bpf, ast, daniel, andrii, memxor
Cc: kbuild-all, Daniel Xu, pablo, fw, netfilter-devel, netdev,
linux-kernel
In-Reply-To: <f850bb7e20950736d9175c61d7e0691098e06182.1660592020.git.dxu@dxuuu.xyz>
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: um-x86_64_defconfig (https://download.01.org/0day-ci/archive/20220816/202208160931.5FG8tZ8G-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/c7b21d163eb9c61514dd86baf4281deb4d4387bb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Daniel-Xu/Support-direct-writes-to-nf_conn-mark/20220816-060429
git checkout c7b21d163eb9c61514dd86baf4281deb4d4387bb
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=um SUBARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
net/core/filter.c: In function 'tc_cls_act_btf_struct_access':
>> net/core/filter.c:8723:24: error: implicit declaration of function 'btf_struct_access' [-Werror=implicit-function-declaration]
8723 | return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/btf_struct_access +8723 net/core/filter.c
8714
8715 static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log,
8716 const struct btf *btf,
8717 const struct btf_type *t, int off,
8718 int size, enum bpf_access_type atype,
8719 u32 *next_btf_id,
8720 enum bpf_type_flag *flag)
8721 {
8722 if (atype == BPF_READ)
> 8723 return btf_struct_access(log, btf, t, off, size, atype, next_btf_id,
8724 flag);
8725
8726 return nf_conntrack_btf_struct_access(log, btf, t, off, size, atype,
8727 next_btf_id, flag);
8728 }
8729
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* Re: [RFC net-next 2/4] ynl: add the schema for the schemas
From: Jakub Kicinski @ 2022-08-16 0:47 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, davem, edumazet, pabeni, sdf, jacob.e.keller, vadfed,
jiri, dsahern, stephen, fw, linux-doc
In-Reply-To: <6b972ef603ff2bc3a3f3e489aa6638f6246c1e48.camel@sipsolutions.net>
On Mon, 15 Aug 2022 22:09:11 +0200 Johannes Berg wrote:
> On Wed, 2022-08-10 at 19:23 -0700, Jakub Kicinski wrote:
> >
> > + attributes:
> > + description: List of attributes in the space.
> > + type: array
> > + items:
> > + type: object
> > + required: [ name, type ]
> > + additionalProperties: False
> > + properties:
> > + name:
> > + type: string
> > + type: &attr-type
> > + enum: [ unused, flag, binary, u8, u16, u32, u64, s32, s64,
> > + nul-string, multi-attr, nest, array-nest, nest-type-value ]
>
> nest-type-value?
It's the incredibly inventive nesting format used in genetlink policy
dumps where the type of the sub-attr(s there are actually two levels)
carry a value (index of the policy and attribute) rather than denoting
a type :S :S :S
I really need to document the types, I know...
> > + description:
> > + description: Documentation of the attribute.
> > + type: string
> > + type-value:
> > + description: Name of the value extracted from the type of a nest-type-value attribute.
> > + type: array
> > + items:
> > + type: string
> > + len:
> > + oneOf: [ { type: string }, { type: integer }]
> > + sub-type: *attr-type
> > + nested-attributes:
> > + description: Name of the space (sub-space) used inside the attribute.
> > + type: string
>
> Maybe expand that description a bit, it's not really accurate for
> "array-nest"?
Slightly guessing but I think I know what you mean -> the value of the
array is a nest with index as the type and then inside that is the
entry of the array with its attributes <- and that's where the space is
applied, not at the first nest level?
Right, I should probably put that in the docs rather than the schema,
array-nests are expected to strip one layer of nesting and put the
value taken from the type (:D) into an @idx member of the struct
representing the values of the array. Or at least that's what I do in
the C codegen.
Not that any of these beautiful, precious formats should be encouraged
going forward. multi-attr all the way!
> > + enum:
> > + description: Name of the enum used for the atttribute.
>
> typo - attribute
Thanks!
> Do you mean the "name of the enumeration" or the "name of the
> enumeration constant"? (per C99 concepts) I'm a bit confused? I guess
> you mean the "name of the enumeration constant" though I agree most
> people probably don't know the names from C99 (I had to look them up too
> for the sake of being precise here ...)
I meant the type. I think. When u32 carries values of an enum.
Enumeration constant for the attribute type is constructed from
it's name and the prefix/suffix kludge.
^ permalink raw reply
* Re: [PATCH v1 0/3] Bring back driver_deferred_probe_check_state() for now
From: Saravana Kannan @ 2022-08-15 23:36 UTC (permalink / raw)
To: Luca Weiss
Cc: Tony Lindgren, Greg Kroah-Hartman, Rafael J. Wysocki,
Kevin Hilman, Ulf Hansson, Pavel Machek, Len Brown, Andrew Lunn,
Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, naresh.kamboju, kernel-team,
linux-kernel, linux-pm, netdev
In-Reply-To: <CM6REZS9Z8AC.2KCR9N3EFLNQR@otso>
On Mon, Aug 15, 2022 at 9:57 AM Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> On Mon Aug 15, 2022 at 1:01 PM CEST, Tony Lindgren wrote:
> > * Saravana Kannan <saravanak@google.com> [700101 02:00]:
> > > More fixes/changes are needed before driver_deferred_probe_check_state()
> > > can be deleted. So, bring it back for now.
> > >
> > > Greg,
> > >
> > > Can we get this into 5.19? If not, it might not be worth picking up this
> > > series. I could just do the other/more fixes in time for 5.20.
> >
> > Yes please pick this as fixes for v6.0-rc series, it fixes booting for
> > me. I've replied with fixes tags for the two patches that were causing
> > regressions for me.
> >
>
> Hi,
>
> for me Patch 1+3 fix display probe on Qualcomm SM6350 (although display
> for this SoC isn't upstream yet, there are lots of other SoCs with very
> similar setup).
>
> Probe for DPU silently fails, with CONFIG_DEBUG_DRIVER=y we get this:
>
> msm-mdss ae00000.mdss: __genpd_dev_pm_attach() failed to find PM domain: -2
>
> While I'm not familiar with the specifics of fw_devlink, the dtsi has
> power-domains = <&dispcc MDSS_GDSC> for this node but it doesn't pick
> that up for some reason.
>
> We can also see that a bit later dispcc finally probes.
Luca,
Can you test with this series instead and see if it fixes this issue?
https://lore.kernel.org/lkml/20220810060040.321697-1-saravanak@google.com/
You might also need to add this delta on top of the series if the
series itself isn't sufficient.
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2f012e826986..866755d8ad95 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2068,7 +2068,11 @@ static int fw_devlink_create_devlink(struct device *con,
device_links_write_unlock();
}
- sup_dev = get_dev_from_fwnode(sup_handle);
+ if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
+ sup_dev = fwnode_get_next_parent_dev(sup_handle);
+ else
+ sup_dev = get_dev_from_fwnode(sup_handle);
+
if (sup_dev) {
/*
* If it's one of those drivers that don't actually bind to
-Saravana
^ permalink raw reply related
* Re: [PATCH v3 1/5] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Guenter Roeck @ 2022-08-15 23:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Xuan Zhuo, Jason Wang, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Greg KH, Andres Freund
In-Reply-To: <20220815215938.154999-2-mst@redhat.com>
On Mon, Aug 15, 2022 at 06:00:25PM -0400, Michael S. Tsirkin wrote:
> This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
>
> This has been reported to trip up guests on GCP (Google Cloud).
> The reason is that virtio_find_vqs_ctx_size is broken on legacy
> devices. We can in theory fix virtio_find_vqs_ctx_size but
> in fact the patch itself has several other issues:
>
> - It treats unknown speed as < 10G
> - It leaves userspace no way to find out the ring size set by hypervisor
> - It tests speed when link is down
> - It ignores the virtio spec advice:
> Both \field{speed} and \field{duplex} can change, thus the driver
> is expected to re-read these values after receiving a
> configuration change notification.
> - It is not clear the performance impact has been tested properly
>
> Revert the patch for now.
>
> Reported-by: Andres Freund <andres@anarazel.de>
> Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
> Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
> Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
> Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
> Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
> Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Andres Freund <andres@anarazel.de>
> Tested-by: From: Guenter Roeck <linux@roeck-us.net>
s/From: //
> ---
> drivers/net/virtio_net.c | 42 ++++------------------------------------
> 1 file changed, 4 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d934774e9733..ece00b84e3a7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3432,29 +3432,6 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
> (unsigned int)GOOD_PACKET_LEN);
> }
>
> -static void virtnet_config_sizes(struct virtnet_info *vi, u32 *sizes)
> -{
> - u32 i, rx_size, tx_size;
> -
> - if (vi->speed == SPEED_UNKNOWN || vi->speed < SPEED_10000) {
> - rx_size = 1024;
> - tx_size = 1024;
> -
> - } else if (vi->speed < SPEED_40000) {
> - rx_size = 1024 * 4;
> - tx_size = 1024 * 4;
> -
> - } else {
> - rx_size = 1024 * 8;
> - tx_size = 1024 * 8;
> - }
> -
> - for (i = 0; i < vi->max_queue_pairs; i++) {
> - sizes[rxq2vq(i)] = rx_size;
> - sizes[txq2vq(i)] = tx_size;
> - }
> -}
> -
> static int virtnet_find_vqs(struct virtnet_info *vi)
> {
> vq_callback_t **callbacks;
> @@ -3462,7 +3439,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> int ret = -ENOMEM;
> int i, total_vqs;
> const char **names;
> - u32 *sizes;
> bool *ctx;
>
> /* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
> @@ -3490,15 +3466,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx = NULL;
> }
>
> - sizes = kmalloc_array(total_vqs, sizeof(*sizes), GFP_KERNEL);
> - if (!sizes)
> - goto err_sizes;
> -
> /* Parameters for control virtqueue, if any */
> if (vi->has_cvq) {
> callbacks[total_vqs - 1] = NULL;
> names[total_vqs - 1] = "control";
> - sizes[total_vqs - 1] = 64;
> }
>
> /* Allocate/initialize parameters for send/receive virtqueues */
> @@ -3513,10 +3484,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> ctx[rxq2vq(i)] = true;
> }
>
> - virtnet_config_sizes(vi, sizes);
> -
> - ret = virtio_find_vqs_ctx_size(vi->vdev, total_vqs, vqs, callbacks,
> - names, sizes, ctx, NULL);
> + ret = virtio_find_vqs_ctx(vi->vdev, total_vqs, vqs, callbacks,
> + names, ctx, NULL);
> if (ret)
> goto err_find;
>
> @@ -3536,8 +3505,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>
>
> err_find:
> - kfree(sizes);
> -err_sizes:
> kfree(ctx);
> err_ctx:
> kfree(names);
> @@ -3897,9 +3864,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> vi->curr_queue_pairs = num_online_cpus();
> vi->max_queue_pairs = max_queue_pairs;
>
> - virtnet_init_settings(dev);
> - virtnet_update_settings(vi);
> -
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> @@ -3912,6 +3876,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
> netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>
> + virtnet_init_settings(dev);
> +
> if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> vi->failover = net_failover_create(vi->dev);
> if (IS_ERR(vi->failover)) {
> --
> MST
>
^ permalink raw reply
* [syzbot] inconsistent lock state in p9_fd_request
From: syzbot @ 2022-08-15 22:54 UTC (permalink / raw)
To: asmadeus, davem, edumazet, ericvh, kuba, linux-kernel, linux_oss,
lucho, netdev, pabeni, syzkaller-bugs, v9fs-developer
Hello,
syzbot found the following issue on:
HEAD commit: 568035b01cfb Linux 6.0-rc1
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12a5b2a5080000
kernel config: https://syzkaller.appspot.com/x/.config?x=e706e91b2a433db
dashboard link: https://syzkaller.appspot.com/bug?extid=c4455787f92b4f78d5b1
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+c4455787f92b4f78d5b1@syzkaller.appspotmail.com
================================
WARNING: inconsistent lock state
6.0.0-rc1-syzkaller #0 Not tainted
--------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
syz-executor.3/5046 [HC0[0]:SC0[0]:HE1:SE1] takes:
ffff88801e763818 (&clnt->lock){?...}-{2:2}, at: spin_lock include/linux/spinlock.h:349 [inline]
ffff88801e763818 (&clnt->lock){?...}-{2:2}, at: p9_fd_request+0x85/0x330 net/9p/trans_fd.c:672
{IN-HARDIRQ-W} state was registered at:
lock_acquire kernel/locking/lockdep.c:5666 [inline]
lock_acquire+0x1ab/0x570 kernel/locking/lockdep.c:5631
__raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
_raw_spin_lock_irqsave+0x39/0x50 kernel/locking/spinlock.c:162
p9_tag_remove net/9p/client.c:367 [inline]
p9_req_put net/9p/client.c:375 [inline]
p9_req_put+0xc6/0x250 net/9p/client.c:372
req_done+0x1de/0x2e0 net/9p/trans_virtio.c:148
vring_interrupt drivers/virtio/virtio_ring.c:2454 [inline]
vring_interrupt+0x29d/0x3d0 drivers/virtio/virtio_ring.c:2429
__handle_irq_event_percpu+0x227/0x870 kernel/irq/handle.c:158
handle_irq_event_percpu kernel/irq/handle.c:193 [inline]
handle_irq_event+0xa7/0x1e0 kernel/irq/handle.c:210
handle_edge_irq+0x25f/0xd00 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+0x9d/0x210 arch/x86/kernel/irq.c:250
common_interrupt+0xa4/0xc0 arch/x86/kernel/irq.c:240
asm_common_interrupt+0x22/0x40 arch/x86/include/asm/idtentry.h:640
lock_is_held_type+0xe/0x140 kernel/locking/lockdep.c:5694
lock_is_held include/linux/lockdep.h:283 [inline]
rcu_read_lock_sched_held+0x3a/0x70 kernel/rcu/update.c:125
trace_lock_release include/trace/events/lock.h:69 [inline]
lock_release+0x560/0x780 kernel/locking/lockdep.c:5677
rcu_lock_release include/linux/rcupdate.h:285 [inline]
rcu_read_unlock include/linux/rcupdate.h:739 [inline]
batadv_nc_purge_orig_hash net/batman-adv/network-coding.c:412 [inline]
batadv_nc_worker+0x86b/0xfa0 net/batman-adv/network-coding.c:719
process_one_work+0x991/0x1610 kernel/workqueue.c:2289
worker_thread+0x665/0x1080 kernel/workqueue.c:2436
kthread+0x2e4/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
irq event stamp: 527
hardirqs last enabled at (527): [<ffffffff8982206f>] __raw_spin_unlock_irq include/linux/spinlock_api_smp.h:159 [inline]
hardirqs last enabled at (527): [<ffffffff8982206f>] _raw_spin_unlock_irq+0x1f/0x40 kernel/locking/spinlock.c:202
hardirqs last disabled at (526): [<ffffffff89821e41>] __raw_spin_lock_irq include/linux/spinlock_api_smp.h:117 [inline]
hardirqs last disabled at (526): [<ffffffff89821e41>] _raw_spin_lock_irq+0x41/0x50 kernel/locking/spinlock.c:170
softirqs last enabled at (0): [<ffffffff8146165e>] copy_process+0x213e/0x7090 kernel/fork.c:2201
softirqs last disabled at (0): [<0000000000000000>] 0x0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&clnt->lock);
<Interrupt>
lock(&clnt->lock);
*** DEADLOCK ***
no locks held by syz-executor.3/5046.
stack backtrace:
CPU: 0 PID: 5046 Comm: syz-executor.3 Not tainted 6.0.0-rc1-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_usage_bug kernel/locking/lockdep.c:3961 [inline]
valid_state kernel/locking/lockdep.c:3973 [inline]
mark_lock_irq kernel/locking/lockdep.c:4176 [inline]
mark_lock.part.0.cold+0x18/0xd8 kernel/locking/lockdep.c:4632
mark_lock kernel/locking/lockdep.c:4596 [inline]
mark_usage kernel/locking/lockdep.c:4541 [inline]
__lock_acquire+0x847/0x56d0 kernel/locking/lockdep.c:5007
lock_acquire kernel/locking/lockdep.c:5666 [inline]
lock_acquire+0x1ab/0x570 kernel/locking/lockdep.c:5631
__raw_spin_lock include/linux/spinlock_api_smp.h:133 [inline]
_raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:154
spin_lock include/linux/spinlock.h:349 [inline]
p9_fd_request+0x85/0x330 net/9p/trans_fd.c:672
p9_client_rpc+0x2f0/0xce0 net/9p/client.c:660
p9_client_version net/9p/client.c:880 [inline]
p9_client_create+0xaec/0x1070 net/9p/client.c:985
v9fs_session_init+0x1e2/0x1810 fs/9p/v9fs.c:408
v9fs_mount+0xba/0xc90 fs/9p/vfs_super.c:126
legacy_get_tree+0x105/0x220 fs/fs_context.c:610
vfs_get_tree+0x89/0x2f0 fs/super.c:1530
do_new_mount fs/namespace.c:3040 [inline]
path_mount+0x1326/0x1e20 fs/namespace.c:3370
do_mount fs/namespace.c:3383 [inline]
__do_sys_mount fs/namespace.c:3591 [inline]
__se_sys_mount fs/namespace.c:3568 [inline]
__x64_sys_mount+0x27f/0x300 fs/namespace.c:3568
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f072b089279
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f072c122168 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007f072b19bf80 RCX: 00007f072b089279
RDX: 0000000020000040 RSI: 0000000020000000 RDI: 0000000000000000
RBP: 00007f072b0e3189 R08: 0000000020000400 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffcabcfe84f R14: 00007f072c122300 R15: 0000000000022000
</TASK>
---
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 syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH v3 0/5] virtio: drop sizing vqs during init
From: Linus Torvalds @ 2022-08-15 22:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Xuan Zhuo, Jason Wang, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Jens Axboe, James Bottomley, Martin K. Petersen, Guenter Roeck,
Greg KH
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
On Mon, Aug 15, 2022 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> But the benefit is unclear in any case, so let's revert for now.
Should I take this patch series directly, or will you be sending a
pull request (preferred)?
Linus
^ permalink raw reply
* Re: igc: missing HW timestamps at TX
From: Vladimir Oltean @ 2022-08-15 22:26 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: Ferenc Fejes, netdev@vger.kernel.org, marton12050@gmail.com,
peti.antal99@gmail.com
In-Reply-To: <87v8qti3u2.fsf@intel.com>
Hi Vinicius,
On Mon, Aug 15, 2022 at 02:39:33PM -0700, Vinicius Costa Gomes wrote:
> Just some aditional information (note that I know very little about
> interrupt internal workings), igc_intr_msi() is called when MSI-X is not
> enabled (i.e. "MSI only" system), igc_msix_other() is called when MSI-X
> is available. When MSI-X is available, i225/i226 sets up a separate
> interrupt handler for "general" events, the TX timestamp being available
> to be read from the registers is one those events.
Thanks for the extra information.
Why is the i225/i226 emitting an interrupt about the availability of a
new TX timestamp, if the igc driver polls for its availability anyway?
In other words, when IGC_TSICR_TXTS is found set, is a TX timestamp
available or is it not? Why does the driver schedule a deferred work
item to retrieve it?
^ permalink raw reply
* [PATCH v3 5/5] virtio: Revert "virtio: find_vqs() add arg sizes"
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH, Richard Weinberger, Anton Ivanov,
Johannes Berg, Hans de Goede, Mark Gross, Vadim Pasternak,
Bjorn Andersson, Mathieu Poirier, Cornelia Huck, Halil Pasic,
Eric Farman, Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, linux-um,
platform-driver-x86, linux-remoteproc, linux-s390, kvm
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
This reverts commit a10fba0377145fccefea4dc4dd5915b7ed87e546: the
proposed API isn't supported on all transports but no
effort was made to address this.
It might not be hard to fix if we want to: maybe just
rename size to size_hint and make sure legacy
transports ignore the hint.
But it's not sure what the benefit is in any case, so
let's drop it.
Fixes: a10fba037714 ("virtio: find_vqs() add arg sizes")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/um/drivers/virtio_uml.c | 2 +-
drivers/platform/mellanox/mlxbf-tmfifo.c | 1 -
drivers/remoteproc/remoteproc_virtio.c | 1 -
drivers/s390/virtio/virtio_ccw.c | 1 -
drivers/virtio/virtio_mmio.c | 1 -
drivers/virtio/virtio_pci_common.c | 2 +-
drivers/virtio/virtio_pci_common.h | 2 +-
drivers/virtio/virtio_pci_modern.c | 7 ++-----
drivers/virtio/virtio_vdpa.c | 1 -
include/linux/virtio_config.h | 14 +++++---------
10 files changed, 10 insertions(+), 22 deletions(-)
diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
index 79e38afd4b91..e719af8bdf56 100644
--- a/arch/um/drivers/virtio_uml.c
+++ b/arch/um/drivers/virtio_uml.c
@@ -1011,7 +1011,7 @@ static struct virtqueue *vu_setup_vq(struct virtio_device *vdev,
static int vu_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[], u32 sizes[], const bool *ctx,
+ const char * const names[], const bool *ctx,
struct irq_affinity *desc)
{
struct virtio_uml_device *vu_dev = to_virtio_uml_device(vdev);
diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
index 8be13d416f48..1ae3c56b66b0 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -928,7 +928,6 @@ static int mlxbf_tmfifo_virtio_find_vqs(struct virtio_device *vdev,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
const char * const names[],
- u32 sizes[],
const bool *ctx,
struct irq_affinity *desc)
{
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 81c4f5776109..0f7706e23eb9 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -158,7 +158,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
const char * const names[],
- u32 sizes[],
const bool * ctx,
struct irq_affinity *desc)
{
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 896896e32664..a10dbe632ef9 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -637,7 +637,6 @@ static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
const char * const names[],
- u32 sizes[],
const bool *ctx,
struct irq_affinity *desc)
{
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index dfcecfd7aba1..3ff746e3f24a 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -474,7 +474,6 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
const char * const names[],
- u32 sizes[],
const bool *ctx,
struct irq_affinity *desc)
{
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 7ad734584823..ad258a9d3b9f 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -396,7 +396,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
/* the config->find_vqs() implementation */
int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[], u32 sizes[], const bool *ctx,
+ const char * const names[], const bool *ctx,
struct irq_affinity *desc)
{
int err;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index a5ff838b85a5..23112d84218f 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -110,7 +110,7 @@ void vp_del_vqs(struct virtio_device *vdev);
/* the config->find_vqs() implementation */
int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[], u32 sizes[], const bool *ctx,
+ const char * const names[], const bool *ctx,
struct irq_affinity *desc);
const char *vp_bus_name(struct virtio_device *vdev);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index be51ec849252..c3b9f2761849 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -347,15 +347,12 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
static int vp_modern_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char * const names[],
- u32 sizes[],
- const bool *ctx,
+ const char * const names[], const bool *ctx,
struct irq_affinity *desc)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
struct virtqueue *vq;
- int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names, sizes, ctx,
- desc);
+ int rc = vp_find_vqs(vdev, nvqs, vqs, callbacks, names, ctx, desc);
if (rc)
return rc;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 9bc4d110b800..c6b9b5062043 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -272,7 +272,6 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
const char * const names[],
- u32 sizes[],
const bool *ctx,
struct irq_affinity *desc)
{
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 888f7e96f0c7..36ec7be1f480 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -55,7 +55,6 @@ struct virtio_shm_region {
* include a NULL entry for vqs that do not need a callback
* names: array of virtqueue names (mainly for debugging)
* include a NULL entry for vqs unused by driver
- * sizes: array of virtqueue sizes
* Returns 0 on success or error status
* @del_vqs: free virtqueues found by find_vqs().
* @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
@@ -104,9 +103,7 @@ struct virtio_config_ops {
void (*reset)(struct virtio_device *vdev);
int (*find_vqs)(struct virtio_device *, unsigned nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[],
- u32 sizes[],
- const bool *ctx,
+ const char * const names[], const bool *ctx,
struct irq_affinity *desc);
void (*del_vqs)(struct virtio_device *);
void (*synchronize_cbs)(struct virtio_device *);
@@ -215,7 +212,7 @@ struct virtqueue *virtio_find_single_vq(struct virtio_device *vdev,
const char *names[] = { n };
struct virtqueue *vq;
int err = vdev->config->find_vqs(vdev, 1, &vq, callbacks, names, NULL,
- NULL, NULL);
+ NULL);
if (err < 0)
return ERR_PTR(err);
return vq;
@@ -227,8 +224,7 @@ int virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
const char * const names[],
struct irq_affinity *desc)
{
- return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, NULL,
- NULL, desc);
+ return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, NULL, desc);
}
static inline
@@ -237,8 +233,8 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
const char * const names[], const bool *ctx,
struct irq_affinity *desc)
{
- return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, NULL,
- ctx, desc);
+ return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, ctx,
+ desc);
}
/**
--
MST
^ permalink raw reply related
* [PATCH v3 4/5] virtio_pci: Revert "virtio_pci: support the arg sizes of find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH, Andres Freund
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
This reverts commit cdb44806fca2d0ad29ca644cbf1505433902ee0c: the legacy
path is wrong and in fact can not support the proposed API since for a
legacy device we never communicate the vq size to the hypervisor.
Reported-by: Andres Freund <andres@anarazel.de>
Fixes: cdb44806fca2 ("virtio_pci: support the arg sizes of find_vqs()")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_pci_common.c | 18 ++++++++----------
drivers/virtio/virtio_pci_common.h | 1 -
drivers/virtio/virtio_pci_legacy.c | 6 +-----
drivers/virtio/virtio_pci_modern.c | 10 +++-------
4 files changed, 12 insertions(+), 23 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 00ad476a815d..7ad734584823 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -174,7 +174,6 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
void (*callback)(struct virtqueue *vq),
const char *name,
- u32 size,
bool ctx,
u16 msix_vec)
{
@@ -187,7 +186,7 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int in
if (!info)
return ERR_PTR(-ENOMEM);
- vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, size, ctx,
+ vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
msix_vec);
if (IS_ERR(vq))
goto out_info;
@@ -284,7 +283,7 @@ void vp_del_vqs(struct virtio_device *vdev)
static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[], u32 sizes[], bool per_vq_vectors,
+ const char * const names[], bool per_vq_vectors,
const bool *ctx,
struct irq_affinity *desc)
{
@@ -327,8 +326,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
else
msix_vec = VP_MSIX_VQ_VECTOR;
vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
- sizes ? sizes[i] : 0,
- ctx ? ctx[i] : false, msix_vec);
+ ctx ? ctx[i] : false,
+ msix_vec);
if (IS_ERR(vqs[i])) {
err = PTR_ERR(vqs[i]);
goto error_find;
@@ -358,7 +357,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
struct virtqueue *vqs[], vq_callback_t *callbacks[],
- const char * const names[], u32 sizes[], const bool *ctx)
+ const char * const names[], const bool *ctx)
{
struct virtio_pci_device *vp_dev = to_vp_device(vdev);
int i, err, queue_idx = 0;
@@ -380,7 +379,6 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
continue;
}
vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
- sizes ? sizes[i] : 0,
ctx ? ctx[i] : false,
VIRTIO_MSI_NO_VECTOR);
if (IS_ERR(vqs[i])) {
@@ -404,15 +402,15 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
int err;
/* Try MSI-X with one vector per queue. */
- err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, sizes, true, ctx, desc);
+ err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, true, ctx, desc);
if (!err)
return 0;
/* Fallback: MSI-X with one vector for config, one shared for queues. */
- err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, sizes, false, ctx, desc);
+ err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, false, ctx, desc);
if (!err)
return 0;
/* Finally fall back to regular interrupts. */
- return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, sizes, ctx);
+ return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
}
const char *vp_bus_name(struct virtio_device *vdev)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index c0448378b698..a5ff838b85a5 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -80,7 +80,6 @@ struct virtio_pci_device {
unsigned int idx,
void (*callback)(struct virtqueue *vq),
const char *name,
- u32 size,
bool ctx,
u16 msix_vec);
void (*del_vq)(struct virtio_pci_vq_info *info);
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index d75e5c4e637f..2257f1b3d8ae 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -112,7 +112,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
unsigned int index,
void (*callback)(struct virtqueue *vq),
const char *name,
- u32 size,
bool ctx,
u16 msix_vec)
{
@@ -126,13 +125,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
if (!num || vp_legacy_get_queue_enable(&vp_dev->ldev, index))
return ERR_PTR(-ENOENT);
- if (!size || size > num)
- size = num;
-
info->msix_vector = msix_vec;
/* create the vring */
- vq = vring_create_virtqueue(index, size,
+ vq = vring_create_virtqueue(index, num,
VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,
true, false, ctx,
vp_notify, callback, name);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index f7965c5dd36b..be51ec849252 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -293,7 +293,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
unsigned int index,
void (*callback)(struct virtqueue *vq),
const char *name,
- u32 size,
bool ctx,
u16 msix_vec)
{
@@ -311,18 +310,15 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
if (!num || vp_modern_get_queue_enable(mdev, index))
return ERR_PTR(-ENOENT);
- if (!size || size > num)
- size = num;
-
- if (size & (size - 1)) {
- dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", size);
+ if (num & (num - 1)) {
+ dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
return ERR_PTR(-EINVAL);
}
info->msix_vector = msix_vec;
/* create the vring */
- vq = vring_create_virtqueue(index, size,
+ vq = vring_create_virtqueue(index, num,
SMP_CACHE_BYTES, &vp_dev->vdev,
true, true, ctx,
vp_notify, callback, name);
--
MST
^ permalink raw reply related
* [PATCH v3 3/5] virtio-mmio: Revert "virtio_mmio: support the arg sizes of find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
This reverts commit fbed86abba6e0472d98079790e58060e4332608a.
The API is now unused, let's not carry dead code around.
Fixes: fbed86abba6e ("virtio_mmio: support the arg sizes of find_vqs()")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_mmio.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index c492a57531c6..dfcecfd7aba1 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -360,7 +360,7 @@ static void vm_synchronize_cbs(struct virtio_device *vdev)
static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int index,
void (*callback)(struct virtqueue *vq),
- const char *name, u32 size, bool ctx)
+ const char *name, bool ctx)
{
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
struct virtio_mmio_vq_info *info;
@@ -395,11 +395,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
goto error_new_virtqueue;
}
- if (!size || size > num)
- size = num;
-
/* Create the vring */
- vq = vring_create_virtqueue(index, size, VIRTIO_MMIO_VRING_ALIGN, vdev,
+ vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev,
true, true, ctx, vm_notify, callback, name);
if (!vq) {
err = -ENOMEM;
@@ -503,7 +500,6 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
}
vqs[i] = vm_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
- sizes ? sizes[i] : 0,
ctx ? ctx[i] : false);
if (IS_ERR(vqs[i])) {
vm_del_vqs(vdev);
--
MST
^ permalink raw reply related
* [PATCH v3 0/5] virtio: drop sizing vqs during init
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH
Reporting after I botched up v2 posting. Sorry about the noise.
Supplying size during init does not work for all transports.
In fact for legacy pci doing that causes a memory
corruption which was reported on Google Cloud.
We might get away with changing size to size_hint so it's
safe to ignore and then fixing legacy to ignore the hint.
But the benefit is unclear in any case, so let's revert for now.
Any new version will have to come with
- documentation of performance gains
- performance testing showing existing workflows
are not harmed materially. especially ones with
bursty traffic
- report of testing on legacy devices
Huge shout out to Andres Freund for the effort spent reproducing and
debugging! Thanks to Guenter Roeck for help with testing!
changes from v2
drop unrelated patches
changes from v1
revert the ring size api, it's unused now
Michael S. Tsirkin (5):
virtio_net: Revert "virtio_net: set the default max ring size by
find_vqs()"
virtio: Revert "virtio: add helper virtio_find_vqs_ctx_size()"
virtio-mmio: Revert "virtio_mmio: support the arg sizes of find_vqs()"
virtio_pci: Revert "virtio_pci: support the arg sizes of find_vqs()"
virtio: Revert "virtio: find_vqs() add arg sizes"
arch/um/drivers/virtio_uml.c | 2 +-
drivers/net/virtio_net.c | 42 +++---------------------
drivers/platform/mellanox/mlxbf-tmfifo.c | 1 -
drivers/remoteproc/remoteproc_virtio.c | 1 -
drivers/s390/virtio/virtio_ccw.c | 1 -
drivers/virtio/virtio_mmio.c | 9 ++---
drivers/virtio/virtio_pci_common.c | 20 +++++------
drivers/virtio/virtio_pci_common.h | 3 +-
drivers/virtio/virtio_pci_legacy.c | 6 +---
drivers/virtio/virtio_pci_modern.c | 17 +++-------
drivers/virtio/virtio_vdpa.c | 1 -
include/linux/virtio_config.h | 26 +++------------
12 files changed, 28 insertions(+), 101 deletions(-)
--
MST
^ permalink raw reply
* [PATCH v3 2/5] virtio: Revert "virtio: add helper virtio_find_vqs_ctx_size()"
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
This reverts commit fe3dc04e31aa51f91dc7f741a5f76cc4817eb5b4: the
API is now unused and in fact can't be implemented on top of a legacy
device.
Fixes: fe3dc04e31aa ("virtio: add helper virtio_find_vqs_ctx_size()")
Cc: "Xuan Zhuo" <xuanzhuo@linux.alibaba.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_config.h | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 6adff09f7170..888f7e96f0c7 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -241,18 +241,6 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
ctx, desc);
}
-static inline
-int virtio_find_vqs_ctx_size(struct virtio_device *vdev, u32 nvqs,
- struct virtqueue *vqs[],
- vq_callback_t *callbacks[],
- const char * const names[],
- u32 sizes[],
- const bool *ctx, struct irq_affinity *desc)
-{
- return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, sizes,
- ctx, desc);
-}
-
/**
* virtio_synchronize_cbs - synchronize with virtqueue callbacks
* @vdev: the device
--
MST
^ permalink raw reply related
* [PATCH v3 1/5] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 22:00 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH, Andres Freund
In-Reply-To: <20220815215938.154999-1-mst@redhat.com>
This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
This has been reported to trip up guests on GCP (Google Cloud).
The reason is that virtio_find_vqs_ctx_size is broken on legacy
devices. We can in theory fix virtio_find_vqs_ctx_size but
in fact the patch itself has several other issues:
- It treats unknown speed as < 10G
- It leaves userspace no way to find out the ring size set by hypervisor
- It tests speed when link is down
- It ignores the virtio spec advice:
Both \field{speed} and \field{duplex} can change, thus the driver
is expected to re-read these values after receiving a
configuration change notification.
- It is not clear the performance impact has been tested properly
Revert the patch for now.
Reported-by: Andres Freund <andres@anarazel.de>
Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Andres Freund <andres@anarazel.de>
Tested-by: From: Guenter Roeck <linux@roeck-us.net>
---
drivers/net/virtio_net.c | 42 ++++------------------------------------
1 file changed, 4 insertions(+), 38 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d934774e9733..ece00b84e3a7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3432,29 +3432,6 @@ static unsigned int mergeable_min_buf_len(struct virtnet_info *vi, struct virtqu
(unsigned int)GOOD_PACKET_LEN);
}
-static void virtnet_config_sizes(struct virtnet_info *vi, u32 *sizes)
-{
- u32 i, rx_size, tx_size;
-
- if (vi->speed == SPEED_UNKNOWN || vi->speed < SPEED_10000) {
- rx_size = 1024;
- tx_size = 1024;
-
- } else if (vi->speed < SPEED_40000) {
- rx_size = 1024 * 4;
- tx_size = 1024 * 4;
-
- } else {
- rx_size = 1024 * 8;
- tx_size = 1024 * 8;
- }
-
- for (i = 0; i < vi->max_queue_pairs; i++) {
- sizes[rxq2vq(i)] = rx_size;
- sizes[txq2vq(i)] = tx_size;
- }
-}
-
static int virtnet_find_vqs(struct virtnet_info *vi)
{
vq_callback_t **callbacks;
@@ -3462,7 +3439,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
int ret = -ENOMEM;
int i, total_vqs;
const char **names;
- u32 *sizes;
bool *ctx;
/* We expect 1 RX virtqueue followed by 1 TX virtqueue, followed by
@@ -3490,15 +3466,10 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
ctx = NULL;
}
- sizes = kmalloc_array(total_vqs, sizeof(*sizes), GFP_KERNEL);
- if (!sizes)
- goto err_sizes;
-
/* Parameters for control virtqueue, if any */
if (vi->has_cvq) {
callbacks[total_vqs - 1] = NULL;
names[total_vqs - 1] = "control";
- sizes[total_vqs - 1] = 64;
}
/* Allocate/initialize parameters for send/receive virtqueues */
@@ -3513,10 +3484,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
ctx[rxq2vq(i)] = true;
}
- virtnet_config_sizes(vi, sizes);
-
- ret = virtio_find_vqs_ctx_size(vi->vdev, total_vqs, vqs, callbacks,
- names, sizes, ctx, NULL);
+ ret = virtio_find_vqs_ctx(vi->vdev, total_vqs, vqs, callbacks,
+ names, ctx, NULL);
if (ret)
goto err_find;
@@ -3536,8 +3505,6 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
err_find:
- kfree(sizes);
-err_sizes:
kfree(ctx);
err_ctx:
kfree(names);
@@ -3897,9 +3864,6 @@ static int virtnet_probe(struct virtio_device *vdev)
vi->curr_queue_pairs = num_online_cpus();
vi->max_queue_pairs = max_queue_pairs;
- virtnet_init_settings(dev);
- virtnet_update_settings(vi);
-
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
@@ -3912,6 +3876,8 @@ static int virtnet_probe(struct virtio_device *vdev)
netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs);
netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
+ virtnet_init_settings(dev);
+
if (virtio_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
vi->failover = net_failover_create(vi->dev);
if (IS_ERR(vi->failover)) {
--
MST
^ permalink raw reply related
* Re: [PATCH v2 1/1] virtio: kerneldocs fixes and enhancements
From: Michael S. Tsirkin @ 2022-08-15 21:56 UTC (permalink / raw)
To: linux-kernel
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, Greg KH, Ricardo Cañuelo, Cornelia Huck
In-Reply-To: <20220815215251.154451-2-mst@redhat.com>
On Mon, Aug 15, 2022 at 05:53:24PM -0400, Michael S. Tsirkin wrote:
> From: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
>
> Fix variable names in some kerneldocs, naming in others.
> Add kerneldocs for struct vring_desc and vring_interrupt.
>
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> Message-Id: <20220810094004.1250-2-ricardo.canuelo@collabora.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Ouch this got here by mistake. Just ignore this patch pls -
it's already in my tree but does not belong in the patchset.
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d66c8e6d0ef3..4620e9d79dde 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2426,6 +2426,14 @@ static inline bool more_used(const struct vring_virtqueue *vq)
> return vq->packed_ring ? more_used_packed(vq) : more_used_split(vq);
> }
>
> +/**
> + * vring_interrupt - notify a virtqueue on an interrupt
> + * @irq: the IRQ number (ignored)
> + * @_vq: the struct virtqueue to notify
> + *
> + * Calls the callback function of @_vq to process the virtqueue
> + * notification.
> + */
> irqreturn_t vring_interrupt(int irq, void *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index a3f73bb6733e..dcab9c7e8784 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -11,7 +11,7 @@
> #include <linux/gfp.h>
>
> /**
> - * virtqueue - a queue to register buffers for sending or receiving.
> + * struct virtqueue - a queue to register buffers for sending or receiving.
> * @list: the chain of virtqueues for this device
> * @callback: the function to call when buffers are consumed (can be NULL).
> * @name: the name of this virtqueue (mainly for debugging)
> @@ -97,7 +97,7 @@ int virtqueue_resize(struct virtqueue *vq, u32 num,
> void (*recycle)(struct virtqueue *vq, void *buf));
>
> /**
> - * virtio_device - representation of a device using virtio
> + * struct virtio_device - representation of a device using virtio
> * @index: unique position on the virtio bus
> * @failed: saved value for VIRTIO_CONFIG_S_FAILED bit (for restore)
> * @config_enabled: configuration change reporting enabled
> @@ -156,7 +156,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev);
> list_for_each_entry(vq, &vdev->vqs, list)
>
> /**
> - * virtio_driver - operations for a virtio I/O driver
> + * struct virtio_driver - operations for a virtio I/O driver
> * @driver: underlying device driver (populate name and owner).
> * @id_table: the ids serviced by this driver.
> * @feature_table: an array of feature numbers supported by this driver.
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 36ec7be1f480..4b517649cfe8 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -239,7 +239,7 @@ int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
>
> /**
> * virtio_synchronize_cbs - synchronize with virtqueue callbacks
> - * @vdev: the device
> + * @dev: the virtio device
> */
> static inline
> void virtio_synchronize_cbs(struct virtio_device *dev)
> @@ -258,7 +258,7 @@ void virtio_synchronize_cbs(struct virtio_device *dev)
>
> /**
> * virtio_device_ready - enable vq use in probe function
> - * @vdev: the device
> + * @dev: the virtio device
> *
> * Driver must call this to use vqs in the probe function.
> *
> @@ -306,7 +306,7 @@ const char *virtio_bus_name(struct virtio_device *vdev)
> /**
> * virtqueue_set_affinity - setting affinity for a virtqueue
> * @vq: the virtqueue
> - * @cpu: the cpu no.
> + * @cpu_mask: the cpu mask
> *
> * Pay attention the function are best-effort: the affinity hint may not be set
> * due to config support, irq type and sharing.
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index 476d3e5c0fe7..f8c20d3de8da 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -93,15 +93,21 @@
> #define VRING_USED_ALIGN_SIZE 4
> #define VRING_DESC_ALIGN_SIZE 16
>
> -/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
> +/**
> + * struct vring_desc - Virtio ring descriptors,
> + * 16 bytes long. These can chain together via @next.
> + *
> + * @addr: buffer address (guest-physical)
> + * @len: buffer length
> + * @flags: descriptor flags
> + * @next: index of the next descriptor in the chain,
> + * if the VRING_DESC_F_NEXT flag is set. We chain unused
> + * descriptors via this, too.
> + */
> struct vring_desc {
> - /* Address (guest-physical). */
> __virtio64 addr;
> - /* Length. */
> __virtio32 len;
> - /* The flags as indicated above. */
> __virtio16 flags;
> - /* We chain unused descriptors via this, too */
> __virtio16 next;
> };
>
> --
> MST
>
^ permalink raw reply
* Re: [PATCH] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 21:47 UTC (permalink / raw)
To: Andres Freund
Cc: Guenter Roeck, linux-kernel, Xuan Zhuo, Jason Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
virtualization, netdev, Linus Torvalds, Jens Axboe,
James Bottomley, Martin K. Petersen, Greg KH, c
In-Reply-To: <20220815214604.x7g342h3oadruxx2@awork3.anarazel.de>
On Mon, Aug 15, 2022 at 02:46:04PM -0700, Andres Freund wrote:
> Hi,
>
> On 2022-08-15 17:39:08 -0400, Michael S. Tsirkin wrote:
> > On Mon, Aug 15, 2022 at 02:28:39PM -0700, Andres Freund wrote:
> > > On 2022-08-15 17:04:10 -0400, Michael S. Tsirkin wrote:
> > > > So virtio has a queue_size register. When read, it will give you
> > > > originally the maximum queue size. Normally we just read it and
> > > > use it as queue size.
> > > >
> > > > However, when queue memory allocation fails, and unconditionally with a
> > > > network device with the problematic patch, driver is asking the
> > > > hypervisor to make the ring smaller by writing a smaller value into this
> > > > register.
> > > >
> > > > I suspect that what happens is hypervisor still uses the original value
> > > > somewhere.
> > >
> > > It looks more like the host is never told about the changed size for legacy
> > > devices...
> > >
> > > Indeed, adding a vp_legacy_set_queue_size() & call to it to setup_vq(), makes
> > > 5.19 + restricting queue sizes to 1024 boot again.
> >
> > Interesting, the register is RO in the legacy interface.
> > And to be frank I can't find where is vp_legacy_set_queue_size
> > even implemented. It's midnight here too ...
>
> Yea, I meant that added both vp_legacy_set_queue_size() and a call to it. I
> was just quickly experimenting around.
interesting that it's writeable on GCP. It's RO on QEMU.
>
> > Yes I figured this out too. And I was able to reproduce on qemu now.
>
> Cool.
>
>
> > I'm posting a new patchset reverting all the handing of resize
> > restrictions, I think we should rethink it for the next release.
>
> Makes sense.
>
> Greetings,
>
> Andres Freund
^ permalink raw reply
* Re: upstream kernel crashes
From: Michael S. Tsirkin @ 2022-08-15 21:32 UTC (permalink / raw)
To: Andres Freund
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, linux-kernel, Greg KH, c
In-Reply-To: <20220815205330.m54g7vcs77r6owd6@awork3.anarazel.de>
On Mon, Aug 15, 2022 at 01:53:30PM -0700, Andres Freund wrote:
> Hi,
>
> On 2022-08-15 16:21:51 -0400, Michael S. Tsirkin wrote:
> > On Mon, Aug 15, 2022 at 10:46:17AM -0700, Andres Freund wrote:
> > > Hi,
> > >
> > > On 2022-08-15 12:50:52 -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Aug 15, 2022 at 09:45:03AM -0700, Andres Freund wrote:
> > > > > Hi,
> > > > >
> > > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote:
> > > > > > OK so this gives us a quick revert as a solution for now.
> > > > > > Next, I would appreciate it if you just try this simple hack.
> > > > > > If it crashes we either have a long standing problem in virtio
> > > > > > code or more likely a gcp bug where it can't handle smaller
> > > > > > rings than what device requestes.
> > > > > > Thanks!
> > > > >
> > > > > I applied the below and the problem persists.
> > > > >
> > > > > [...]
> > > >
> > > > Okay!
> > >
> > > Just checking - I applied and tested this atop 6.0-rc1, correct? Or did you
> > > want me to test it with the 762faee5a267 reverted? I guess what you're trying
> > > to test if a smaller queue than what's requested you'd want to do so without
> > > the problematic patch applied...
> > >
> > >
> > > Either way, I did this, and there are no issues that I could observe. No
> > > oopses, no broken networking. But:
> > >
> > > To make sure it does something I added a debugging printk - which doesn't show
> > > up. I assume this is at a point at least earlyprintk should work (which I see
> > > getting enabled via serial)?
> > >
>
> > Sorry if I was unclear. I wanted to know whether the change somehow
> > exposes a driver bug or a GCP bug. So what I wanted to do is to test
> > this patch on top of *5.19*, not on top of the revert.
>
> Right, the 5.19 part was clear, just the earlier test:
>
> > > > > On 2022-08-15 11:40:59 -0400, Michael S. Tsirkin wrote:
> > > > > > OK so this gives us a quick revert as a solution for now.
> > > > > > Next, I would appreciate it if you just try this simple hack.
> > > > > > If it crashes we either have a long standing problem in virtio
> > > > > > code or more likely a gcp bug where it can't handle smaller
> > > > > > Thanks!
>
> I wasn't sure about.
>
> After I didn't see any effect on 5.19 + your patch, I grew a bit suspicious
> and added the printks.
>
>
> > Yes I think printk should work here.
>
> The reason the debug patch didn't change anything, and that my debug printk
> didn't show, is that gcp uses the legacy paths...
Wait a second. Eureka I think!
So I think GCP is not broken.
I think what's broken is this patch:
commit cdb44806fca2d0ad29ca644cbf1505433902ee0c
Author: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Date: Mon Aug 1 14:38:54 2022 +0800
virtio_pci: support the arg sizes of find_vqs()
Specifically:
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 2257f1b3d8ae..d75e5c4e637f 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -112,6 +112,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
unsigned int index,
void (*callback)(struct virtqueue *vq),
const char *name,
+ u32 size,
bool ctx,
u16 msix_vec)
{
@@ -125,10 +126,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
if (!num || vp_legacy_get_queue_enable(&vp_dev->ldev, index))
return ERR_PTR(-ENOENT);
+ if (!size || size > num)
+ size = num;
+
info->msix_vector = msix_vec;
/* create the vring */
- vq = vring_create_virtqueue(index, num,
+ vq = vring_create_virtqueue(index, size,
VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,
true, false, ctx,
vp_notify, callback, name);
So if you pass the size parameter for a legacy device it will
try to make the ring smaller and that is not legal with
legacy at all. But the driver treats legacy and modern
the same, it allocates a smaller queue anyway.
Lo and behold, I pass disable-modern=on to qemu and it happily
corrupts memory exactly the same as GCP does.
So the new find_vqs API is actually completely broken, it can not work for
legacy at all and for added fun there's no way to find out
that it's legacy. Maybe we should interpret the patch
So I think I will also revert
04ca0b0b16f11faf74fa92468dab51b8372586cd..fe3dc04e31aa51f91dc7f741a5f76cc4817eb5b4
> If there were a bug in the legacy path, it'd explain why the problem only
> shows on gcp, and not in other situations.
>
> I'll queue testing the legacy path with the equivalent change.
>
> - Andres
>
>
> Greetings,
>
> Andres Freund
^ permalink raw reply related
* Re: [PATCH] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Andres Freund @ 2022-08-15 21:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Guenter Roeck, linux-kernel, Xuan Zhuo, Jason Wang,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
virtualization, netdev, Linus Torvalds, Jens Axboe,
James Bottomley, Martin K. Petersen, Greg KH, c
In-Reply-To: <20220815165608-mutt-send-email-mst@kernel.org>
Hi,
On 2022-08-15 17:04:10 -0400, Michael S. Tsirkin wrote:
> So virtio has a queue_size register. When read, it will give you
> originally the maximum queue size. Normally we just read it and
> use it as queue size.
>
> However, when queue memory allocation fails, and unconditionally with a
> network device with the problematic patch, driver is asking the
> hypervisor to make the ring smaller by writing a smaller value into this
> register.
>
> I suspect that what happens is hypervisor still uses the original value
> somewhere.
It looks more like the host is never told about the changed size for legacy
devices...
Indeed, adding a vp_legacy_set_queue_size() & call to it to setup_vq(), makes
5.19 + restricting queue sizes to 1024 boot again. I'd bet that it also would
fix 6.0rc1, but I'm running out of time to test that.
Greetings,
Andres Freund
^ permalink raw reply
* Re: [PATCH net] net: genl: fix error path memory leak in policy dumping
From: Rafael Soares @ 2022-08-15 21:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, johannes.berg,
syzbot+dc54d9ba8153b216cae0
In-Reply-To: <20220815182021.48925-1-kuba@kernel.org>
On Mon, Aug 15, 2022 at 11:20:21AM -0700, Jakub Kicinski wrote:
> If construction of the array of policies fails when recording
> non-first policy we need to unwind.
>
> Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
> Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> net/netlink/genetlink.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 1afca2a6c2ac..57010927e20a 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1174,13 +1174,17 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
> op.policy,
> op.maxattr);
> if (err)
> - return err;
> + goto err_free_state;
There's another call to netlink_policy_dump_add_policy() right above
this one. The patch I posted to syzkaller frees the memory inside
netlink_policy_dump_add_policy() and the result was OK.
> }
> }
>
> if (!ctx->state)
> return -ENODATA;
> return 0;
> +
> +err_free_state:
> + netlink_policy_dump_free(ctx->state);
> + return err;
> }
>
> static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
> --
> 2.37.2
>
^ permalink raw reply
* Re: [PATCH 5/6] virtio/vsock: add support for dgram
From: kernel test robot @ 2022-08-15 21:02 UTC (permalink / raw)
To: Bobby Eshleman
Cc: kbuild-all, Bobby Eshleman, Cong Wang, Jiang Wang,
Stefan Hajnoczi, Stefano Garzarella, Michael S. Tsirkin,
Jason Wang, Eric Dumazet, Jakub Kicinski, Paolo Abeni, kvm,
virtualization, netdev, linux-kernel
In-Reply-To: <3cb082f1c88f3f2ef1fc250dbc0745fb79c745c7.1660362668.git.bobby.eshleman@bytedance.com>
Hi Bobby,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.0-rc1 next-20220815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220816/202208160405.cG02E3MZ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/cbb332da78c86ac574688831ed6f404d04d506db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bobby-Eshleman/virtio-vsock-introduce-dgrams-sk_buff-and-qdisc/20220816-015812
git checkout cbb332da78c86ac574688831ed6f404d04d506db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/vmw_vsock/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
net/vmw_vsock/virtio_transport_common.c: In function 'virtio_transport_dgram_do_dequeue':
>> net/vmw_vsock/virtio_transport_common.c:605:13: warning: variable 'free_space' set but not used [-Wunused-but-set-variable]
605 | u32 free_space;
| ^~~~~~~~~~
vim +/free_space +605 net/vmw_vsock/virtio_transport_common.c
597
598 static ssize_t
599 virtio_transport_dgram_do_dequeue(struct vsock_sock *vsk,
600 struct msghdr *msg, size_t len)
601 {
602 struct virtio_vsock_sock *vvs = vsk->trans;
603 struct sk_buff *skb;
604 size_t total = 0;
> 605 u32 free_space;
606 int err = -EFAULT;
607
608 spin_lock_bh(&vvs->rx_lock);
609 if (total < len && !skb_queue_empty_lockless(&vvs->rx_queue)) {
610 skb = __skb_dequeue(&vvs->rx_queue);
611
612 total = len;
613 if (total > skb->len - vsock_metadata(skb)->off)
614 total = skb->len - vsock_metadata(skb)->off;
615 else if (total < skb->len - vsock_metadata(skb)->off)
616 msg->msg_flags |= MSG_TRUNC;
617
618 /* sk_lock is held by caller so no one else can dequeue.
619 * Unlock rx_lock since memcpy_to_msg() may sleep.
620 */
621 spin_unlock_bh(&vvs->rx_lock);
622
623 err = memcpy_to_msg(msg, skb->data + vsock_metadata(skb)->off, total);
624 if (err)
625 return err;
626
627 spin_lock_bh(&vvs->rx_lock);
628
629 virtio_transport_dec_rx_pkt(vvs, skb);
630 consume_skb(skb);
631 }
632
633 free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt);
634
635 spin_unlock_bh(&vvs->rx_lock);
636
637 if (total > 0 && msg->msg_name) {
638 /* Provide the address of the sender. */
639 DECLARE_SOCKADDR(struct sockaddr_vm *, vm_addr, msg->msg_name);
640
641 vsock_addr_init(vm_addr, le64_to_cpu(vsock_hdr(skb)->src_cid),
642 le32_to_cpu(vsock_hdr(skb)->src_port));
643 msg->msg_namelen = sizeof(*vm_addr);
644 }
645 return total;
646 }
647
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* Re: [PATCH] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 21:04 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, Xuan Zhuo, Jason Wang, Andres Freund,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
virtualization, netdev, Linus Torvalds, Jens Axboe,
James Bottomley, Martin K. Petersen, Greg KH, c
In-Reply-To: <20220815205053.GD509309@roeck-us.net>
On Mon, Aug 15, 2022 at 01:50:53PM -0700, Guenter Roeck wrote:
> On Mon, Aug 15, 2022 at 04:42:51PM -0400, Michael S. Tsirkin wrote:
> > On Mon, Aug 15, 2022 at 01:34:26PM -0700, Guenter Roeck wrote:
> > > On Mon, Aug 15, 2022 at 05:16:50AM -0400, Michael S. Tsirkin wrote:
> > > > This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
> > > >
> > > > This has been reported to trip up guests on GCP (Google Cloud). Why is
> > > > not yet clear - to be debugged, but the patch itself has several other
> > > > issues:
> > > >
> > > > - It treats unknown speed as < 10G
> > > > - It leaves userspace no way to find out the ring size set by hypervisor
> > > > - It tests speed when link is down
> > > > - It ignores the virtio spec advice:
> > > > Both \field{speed} and \field{duplex} can change, thus the driver
> > > > is expected to re-read these values after receiving a
> > > > configuration change notification.
> > > > - It is not clear the performance impact has been tested properly
> > > >
> > > > Revert the patch for now.
> > > >
> > > > Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
> > > > Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
> > > > Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
> > > > Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
> > > > Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
> > > > Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Tested-by: Andres Freund <andres@anarazel.de>
> > >
> > > I ran this patch through a total of 14 syskaller tests, 2 test runs each on
> > > 7 different crashes reported by syzkaller (as reported to the linux-kernel
> > > mailing list). No problems were reported. I also ran a single cross-check
> > > with one of the syzkaller runs on top of v6.0-rc1, without this patch.
> > > That test run failed.
> > >
> > > Overall, I think we can call this fixed.
> > >
> > > Guenter
> >
> > It's more of a work around though since we don't yet have the root
> > cause for this. I suspect a GCP hypervisor bug at the moment.
> > This is excercising a path we previously only took on GFP_KERNEL
> > allocation failures during probe, I don't think that happens a lot.
> >
>
> Even a hypervisor bug should not trigger crashes like this one,
> though, or at least I think so. Any idea what to look for on the
> hypervisor side, and/or what it might be doing wrong ?
>
> Thanks,
> Guenter
Sure!
So virtio has a queue_size register. When read, it will give you
originally the maximum queue size. Normally we just read it and
use it as queue size.
However, when queue memory allocation fails, and unconditionally with a
network device with the problematic patch, driver is asking the
hypervisor to make the ring smaller by writing a smaller value into this
register.
I suspect that what happens is hypervisor still uses the original value
somewhere. Then it things the ring is bigger than what driver allocated.
If we get lucky and nothing important landed in the several pages
covered by the larger ting, then the only effect is that driver does not
see the data hypervisor writes in the ring, and this is the network
failures observed - most likely DHCP responses get lost and
guest never gets an IP. OTOH if something important lands there then when
hypervisor overwrites that memory it gets corrupted and we get crashes.
--
MST
^ permalink raw reply
* Re: upstream kernel crashes
From: Andres Freund @ 2022-08-15 21:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Xuan Zhuo, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, virtualization, netdev,
Linus Torvalds, Jens Axboe, James Bottomley, Martin K. Petersen,
Guenter Roeck, linux-kernel, Greg KH, c
In-Reply-To: <20220815205330.m54g7vcs77r6owd6@awork3.anarazel.de>
On 2022-08-15 13:53:31 -0700, Andres Freund wrote:
> The reason the debug patch didn't change anything, and that my debug printk
> didn't show, is that gcp uses the legacy paths...
>
> If there were a bug in the legacy path, it'd explain why the problem only
> shows on gcp, and not in other situations.
>
> I'll queue testing the legacy path with the equivalent change.
Booting with the equivalent change, atop 5.19, in the legacy setup_vq()
reliably causes boot to hang:
[ 0.718768] ACPI: button: Sleep Button [SLPF]
[ 0.721989] ACPI: \_SB_.LNKC: Enabled at IRQ 11
[ 0.722688] adebug: use legacy: 0
[ 0.722724] virtio-pci 0000:00:03.0: virtio_pci: leaving for legacy driver
[ 0.724286] adebug: probe modern: -19
[ 0.727353] ACPI: \_SB_.LNKD: Enabled at IRQ 10
[ 0.728719] adebug: use legacy: 0
[ 0.728766] virtio-pci 0000:00:04.0: virtio_pci: leaving for legacy driver
[ 0.730422] adebug: probe modern: -19
[ 0.733552] ACPI: \_SB_.LNKA: Enabled at IRQ 10
[ 0.734923] adebug: use legacy: 0
[ 0.734957] virtio-pci 0000:00:05.0: virtio_pci: leaving for legacy driver
[ 0.736426] adebug: probe modern: -19
[ 0.739039] ACPI: \_SB_.LNKB: Enabled at IRQ 11
[ 0.740350] adebug: use legacy: 0
[ 0.740390] virtio-pci 0000:00:06.0: virtio_pci: leaving for legacy driver
[ 0.742142] adebug: probe modern: -19
[ 0.747627] adebug: legacy setup_vq
[ 0.748243] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 256
[ 0.751081] adebug: legacy setup_vq
[ 0.751110] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 256
[ 0.754028] adebug: legacy setup_vq
[ 0.754059] virtio-pci 0000:00:05.0: adebug: legacy: not limiting queue size, only 1
[ 0.757760] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[ 0.759135] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
[ 0.760399] 00:04: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
[ 0.761610] 00:05: ttyS2 at I/O 0x3e8 (irq = 6, base_baud = 115200) is a 16550A
[ 0.762923] 00:06: ttyS3 at I/O 0x2e8 (irq = 7, base_baud = 115200) is a 16550A
[ 0.764222] Non-volatile memory driver v1.3
[ 0.768857] adebug: legacy setup_vq
[ 0.768882] virtio-pci 0000:00:06.0: adebug: legacy: not limiting queue size, only 256
[ 0.773002] Linux agpgart interface v0.103
[ 0.775424] loop: module loaded
[ 0.780513] adebug: legacy setup_vq
[ 0.780538] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024
[ 0.784075] adebug: legacy setup_vq
[ 0.784104] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024
[ 0.787073] adebug: legacy setup_vq
[ 0.787101] virtio-pci 0000:00:03.0: adebug: legacy: limiting queue size from 8192 to 1024
[ 0.790379] scsi host0: Virtio SCSI HBA
[ 0.795968] Freeing initrd memory: 7236K
Greetings,
Andres Freund
^ permalink raw reply
* [PATCH 5.19 1114/1157] batman-adv: tracing: Use the new __vstring() helper
From: Greg Kroah-Hartman @ 2022-08-15 18:07 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Marek Lindner, Ingo Molnar,
Andrew Morton, Simon Wunderlich, Antonio Quartulli,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
b.a.t.m.a.n, netdev, Sven Eckelmann, Steven Rostedt (Google),
Sasha Levin
In-Reply-To: <20220815180439.416659447@linuxfoundation.org>
From: Steven Rostedt (Google) <rostedt@goodmis.org>
[ Upstream commit 9abc291812d784bd4a26c01af4ebdbf9f2dbf0bb ]
Instead of open coding a __dynamic_array() with a fixed length (which
defeats the purpose of the dynamic array in the first place). Use the new
__vstring() helper that will use a va_list and only write enough of the
string into the ring buffer that is needed.
Link: https://lkml.kernel.org/r/20220724191650.236b1355@rorschach.local.home
Cc: Marek Lindner <mareklindner@neomailbox.ch>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Simon Wunderlich <sw@simonwunderlich.de>
Cc: Antonio Quartulli <a@unstable.cc>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: b.a.t.m.a.n@lists.open-mesh.org
Cc: netdev@vger.kernel.org
Acked-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
net/batman-adv/trace.h | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/batman-adv/trace.h b/net/batman-adv/trace.h
index d673ebdd0426..31c8f922651d 100644
--- a/net/batman-adv/trace.h
+++ b/net/batman-adv/trace.h
@@ -28,8 +28,6 @@
#endif /* CONFIG_BATMAN_ADV_TRACING */
-#define BATADV_MAX_MSG_LEN 256
-
TRACE_EVENT(batadv_dbg,
TP_PROTO(struct batadv_priv *bat_priv,
@@ -40,16 +38,13 @@ TRACE_EVENT(batadv_dbg,
TP_STRUCT__entry(
__string(device, bat_priv->soft_iface->name)
__string(driver, KBUILD_MODNAME)
- __dynamic_array(char, msg, BATADV_MAX_MSG_LEN)
+ __vstring(msg, vaf->fmt, vaf->va)
),
TP_fast_assign(
__assign_str(device, bat_priv->soft_iface->name);
__assign_str(driver, KBUILD_MODNAME);
- WARN_ON_ONCE(vsnprintf(__get_dynamic_array(msg),
- BATADV_MAX_MSG_LEN,
- vaf->fmt,
- *vaf->va) >= BATADV_MAX_MSG_LEN);
+ __assign_vstr(msg, vaf->fmt, vaf->va);
),
TP_printk(
--
2.35.1
^ permalink raw reply related
* Re: [PATCH] virtio_net: Revert "virtio_net: set the default max ring size by find_vqs()"
From: Michael S. Tsirkin @ 2022-08-15 20:42 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kernel, Xuan Zhuo, Jason Wang, Andres Freund,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
virtualization, netdev, Linus Torvalds, Jens Axboe,
James Bottomley, Martin K. Petersen, Greg KH, c
In-Reply-To: <20220815203426.GA509309@roeck-us.net>
On Mon, Aug 15, 2022 at 01:34:26PM -0700, Guenter Roeck wrote:
> On Mon, Aug 15, 2022 at 05:16:50AM -0400, Michael S. Tsirkin wrote:
> > This reverts commit 762faee5a2678559d3dc09d95f8f2c54cd0466a7.
> >
> > This has been reported to trip up guests on GCP (Google Cloud). Why is
> > not yet clear - to be debugged, but the patch itself has several other
> > issues:
> >
> > - It treats unknown speed as < 10G
> > - It leaves userspace no way to find out the ring size set by hypervisor
> > - It tests speed when link is down
> > - It ignores the virtio spec advice:
> > Both \field{speed} and \field{duplex} can change, thus the driver
> > is expected to re-read these values after receiving a
> > configuration change notification.
> > - It is not clear the performance impact has been tested properly
> >
> > Revert the patch for now.
> >
> > Link: https://lore.kernel.org/r/20220814212610.GA3690074%40roeck-us.net
> > Link: https://lore.kernel.org/r/20220815070203.plwjx7b3cyugpdt7%40awork3.anarazel.de
> > Link: https://lore.kernel.org/r/3df6bb82-1951-455d-a768-e9e1513eb667%40www.fastmail.com
> > Link: https://lore.kernel.org/r/FCDC5DDE-3CDD-4B8A-916F-CA7D87B547CE%40anarazel.de
> > Fixes: 762faee5a267 ("virtio_net: set the default max ring size by find_vqs()")
> > Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Andres Freund <andres@anarazel.de>
>
> I ran this patch through a total of 14 syskaller tests, 2 test runs each on
> 7 different crashes reported by syzkaller (as reported to the linux-kernel
> mailing list). No problems were reported. I also ran a single cross-check
> with one of the syzkaller runs on top of v6.0-rc1, without this patch.
> That test run failed.
>
> Overall, I think we can call this fixed.
>
> Guenter
It's more of a work around though since we don't yet have the root
cause for this. I suspect a GCP hypervisor bug at the moment.
This is excercising a path we previously only took on GFP_KERNEL
allocation failures during probe, I don't think that happens a lot.
--
MST
^ permalink raw reply
* Re: [RFC net-next 1/4] ynl: add intro docs for the concept
From: Jakub Kicinski @ 2022-08-16 0:32 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, davem, edumazet, pabeni, sdf, jacob.e.keller, vadfed,
jiri, dsahern, stephen, fw, linux-doc
In-Reply-To: <273db0bc09c0e074a8875679e5e07ea047b61c27.camel@sipsolutions.net>
On Mon, 15 Aug 2022 22:09:29 +0200 Johannes Berg wrote:
> On Wed, 2022-08-10 at 19:23 -0700, Jakub Kicinski wrote:
> >
> > +Note that attribute spaces do not themselves nest, nested attributes refer to their internal
> > +space via a ``nested-attributes`` property, so the YAML spec does not resemble the format
> > +of the netlink messages directly.
>
> I find this a bit ... confusing.
>
> I think reading the other patch I know what you mean, but if I think of
> this I think more of the policy declarations than the message itself,
> and there we do refer to another policy?
>
> Maybe reword a bit and say
>
> Note that attribute spaces do not themselves nest, nested attributes
> refer to their internal space via a ``nested-attributes`` property
> (the name of another or the same attribute space).
>
> or something?
I think I put the cart before the horse in this looong sentence. How
about:
Note that the YAML spec is "flattened" and is not meant to visually
resemble the format of the netlink messages (unlike certain ad-hoc documentation
formats seen in kernel comments). In the YAML spec subordinate attribute sets
are not defined inline as a nest, but defined in a separate attribute set
referred to with a ``nested-attributes`` property of the container.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox