netdev - Re: [PATCH net-next v12 03/12] virtio: Expose generic device capability operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <29fa4996-38e3-4146-81d3-f8b188e047e9@nvidia.com>
Date: Mon, 24 Nov 2025 16:24:37 -0600
From: Dan Jurgens <danielj@...dia.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, jasowang@...hat.com, pabeni@...hat.com,
 virtualization@...ts.linux.dev, parav@...dia.com, shshitrit@...dia.com,
 yohadt@...dia.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
 jgg@...pe.ca, kevin.tian@...el.com, kuba@...nel.org, andrew+netdev@...n.ch,
 edumazet@...gle.com
Subject: Re: [PATCH net-next v12 03/12] virtio: Expose generic device
 capability operations

On 11/24/25 2:30 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2025 at 01:15:14PM -0600, Daniel Jurgens wrote:
>> Currently querying and setting capabilities is restricted to a single
>> capability and contained within the virtio PCI driver. However, each
>> device type has generic and device specific capabilities, that may be
>> queried and set. In subsequent patches virtio_net will query and set
>> flow filter capabilities.
>>
>> This changes the size of virtio_admin_cmd_query_cap_id_result. It's safe
>> to do because this data is written by DMA, so a newer controller can't
>> overrun the size on an older kernel.
>>
>> Signed-off-by: Daniel Jurgens <danielj@...dia.com>
>> Reviewed-by: Parav Pandit <parav@...dia.com>
>> Reviewed-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>>
>> ---
>> v4: Moved this logic from virtio_pci_modern to new file
>>     virtio_admin_commands.
>>
>> v12:
>>   - Removed uapi virtio_pci include in virtio_admin.h. MST
>>   - Added virtio_pci uapi include to virtio_admin_commands.c
>>   - Put () around cap in macro. MST
>>   - Removed nonsense comment above VIRTIO_ADMIN_MAX_CAP. MST
>>   - +1 VIRTIO_ADMIN_MAX_CAP when calculating array size. MST
>>   - Updated commit message
>> ---
>>  drivers/virtio/Makefile                |  2 +-
>>  drivers/virtio/virtio_admin_commands.c | 91 ++++++++++++++++++++++++++
>>  include/linux/virtio_admin.h           | 80 ++++++++++++++++++++++
>>  include/uapi/linux/virtio_pci.h        |  6 +-
>>  4 files changed, 176 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/virtio/virtio_admin_commands.c
>>  create mode 100644 include/linux/virtio_admin.h
>>
>> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
>> index eefcfe90d6b8..2b4a204dde33 100644
>> --- a/drivers/virtio/Makefile
>> +++ b/drivers/virtio/Makefile
>> @@ -1,5 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
>> +obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o virtio_admin_commands.o
>>  obj-$(CONFIG_VIRTIO_ANCHOR) += virtio_anchor.o
>>  obj-$(CONFIG_VIRTIO_PCI_LIB) += virtio_pci_modern_dev.o
>>  obj-$(CONFIG_VIRTIO_PCI_LIB_LEGACY) += virtio_pci_legacy_dev.o
>> diff --git a/drivers/virtio/virtio_admin_commands.c b/drivers/virtio/virtio_admin_commands.c
>> new file mode 100644
>> index 000000000000..a2254e71e8dc
>> --- /dev/null
>> +++ b/drivers/virtio/virtio_admin_commands.c
>> @@ -0,0 +1,91 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/virtio.h>
>> +#include <linux/virtio_config.h>
>> +#include <linux/virtio_admin.h>
>> +#include <uapi/linux/virtio_pci.h>
>> +
>> +int virtio_admin_cap_id_list_query(struct virtio_device *vdev,
>> +				   struct virtio_admin_cmd_query_cap_id_result *data)
>> +{
>> +	struct virtio_admin_cmd cmd = {};
>> +	struct scatterlist result_sg;
>> +
>> +	if (!vdev->config->admin_cmd_exec)
>> +		return -EOPNOTSUPP;
>> +
>> +	sg_init_one(&result_sg, data, sizeof(*data));
>> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_CAP_ID_LIST_QUERY);
>> +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
>> +	cmd.result_sg = &result_sg;
>> +
>> +	return vdev->config->admin_cmd_exec(vdev, &cmd);
>> +}
>> +EXPORT_SYMBOL_GPL(virtio_admin_cap_id_list_query);
>> +
>> +int virtio_admin_cap_get(struct virtio_device *vdev,
>> +			 u16 id,
>> +			 void *caps,
>> +			 size_t cap_size)
> 
> 
> I still don't get why cap_size needs to be as large as size_t.
> 
> if you don't care what's it size is, just say "unsigned".
> or u8 as a hint to users it's a small value.

The size is small for net flow filters, but this is supposed to be a
generic interface for future uses as well. Why limit it?

> 
>> +{
>> +	struct virtio_admin_cmd_cap_get_data *data;
>> +	struct virtio_admin_cmd cmd = {};
>> +	struct scatterlist result_sg;
>> +	struct scatterlist data_sg;
>> +	int err;
>> +
>> +	if (!vdev->config->admin_cmd_exec)
>> +		return -EOPNOTSUPP;
>> +
>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> 
> uses kzalloc without including linux/slab.h
> 
> 
> 
>> +	if (!data)
>> +		return -ENOMEM;
>> +