netdev - Re: [PATCH net-next v11 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <b4692b58-3bb3-42fb-8436-84b38c6b04a8@nvidia.com>
Date: Tue, 18 Nov 2025 21:27:22 -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 v11 03/12] virtio: Expose generic device
 capability operations

On 11/18/25 3:42 PM, Michael S. Tsirkin wrote:
> On Tue, Nov 18, 2025 at 08:38:53AM -0600, Daniel Jurgens wrote:

>> +#ifndef _LINUX_VIRTIO_ADMIN_H
>> +#define _LINUX_VIRTIO_ADMIN_H
> 
> 
> Guards normally come before #include - there is no
> point in pulling in uapi/linux/virtio_pci.h - just
> extra work for the compiler.
> 
> 

Removed the include.

> 
>> +
>> +struct virtio_device;

>> + */
>> +#define VIRTIO_CAP_IN_LIST(cap_list, cap) \
>> +	(!!(1 & (le64_to_cpu(cap_list->supported_caps[cap / 64]) >> cap % 64)))
> 
> while this works if cap is a variable, it will behave
> unexpectedly if cap or even cap_list is an expression.
> 
> A standard practice is to put all macro arguments in brackets:
> !!(1 & (le64_to_cpu((cap_list)->supported_caps[(cap) / 64]) >> (cap) % 64)))
> 
> 

done

> 
> 
> 
>> +

>>  #define VIRTIO_DEV_PARTS_CAP 0x0000
>>  
>> +/* Update this value to largest implemented cap number. */
> 
> implemented by what?

Removed the comment.

> 
>> +#define VIRTIO_ADMIN_MAX_CAP 0x0fff
>> +

>> -#define MAX_CAP_ID __KERNEL_DIV_ROUND_UP(VIRTIO_DEV_PARTS_CAP + 1, 64)
>> +#define VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE __KERNEL_DIV_ROUND_UP(VIRTIO_ADMIN_MAX_CAP, 64)
> 
> Don't you mean VIRTIO_ADMIN_MAX_CAP + 1 here?
> E.g. if VIRTIO_ADMIN_MAX_CAP was 0 we would need space for 1 capability,
> right?
> 

Added the +1, it's the same result either way here.

>>  
>>  struct virtio_admin_cmd_query_cap_id_result {
>> -	__le64 supported_caps[MAX_CAP_ID];
>> +	__le64 supported_caps[VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE];
>>  };
>>  
> 
> I feel it's worth explaining in commit log you are changing a
> uapi structure, and explaining that it is safe.
> 

Done

> 
>>  struct virtio_admin_cmd_cap_get_data {
>> -- 
>> 2.50.1
>