[PATCH net-next v23 00/14] virtio_net: Add ethtool flow rules support

[PATCH net-next v23 00/14] virtio_net: Add ethtool flow rules support

[Shahar Shitrit]

This series implements ethtool flow rules support for virtio_net using the
virtio flow filter (FF) specification. The implementation allows users to
configure packet filtering rules through ethtool commands, directing
packets to specific receive queues, or dropping them based on various
header fields.

The series starts with infrastructure changes to expose virtio PCI admin
capabilities and object management APIs. It then creates the virtio_net
directory structure and implements the flow filter functionality with
support for:

- Layer 2 (Ethernet) flow rules
- IPv4 and IPv6 flow rules
- TCP and UDP flow rules (both IPv4 and IPv6)
- Rule querying and management operations

Setting, deleting and viewing flow filters, -1 action is drop, positive
integers steer to that RQ:

$ ethtool -u ens9
4 RX rings available
Total 0 rules

$ ethtool -U ens9 flow-type ether src 1c:34:da:4a:33:dd action 0
Added rule with ID 0
$ ethtool -U ens9 flow-type udp4 dst-port 5001 action 3
Added rule with ID 1
$ ethtool -U ens9 flow-type tcp6 src-ip fc00::2 dst-port 5001 action 2
Added rule with ID 2
$ ethtool -U ens9 flow-type ip4 src-ip 192.168.51.101 action 1
Added rule with ID 3
$ ethtool -U ens9 flow-type ip6 dst-ip fc00::1 action -1
Added rule with ID 4
$ ethtool -U ens9 flow-type ip6 src-ip fc00::2 action -1
Added rule with ID 5
$ ethtool -U ens9 delete 4
$ ethtool -u ens9
4 RX rings available
Total 5 rules

Filter: 0
        Flow Type: Raw Ethernet
        Src MAC addr: 1C:34:DA:4A:33:DD mask: 00:00:00:00:00:00
        Dest MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF
        Ethertype: 0x0 mask: 0xFFFF
        Action: Direct to queue 0

Filter: 1
        Rule Type: UDP over IPv4
        Src IP addr: 0.0.0.0 mask: 255.255.255.255
        Dest IP addr: 0.0.0.0 mask: 255.255.255.255
        TOS: 0x0 mask: 0xff
        Src port: 0 mask: 0xffff
        Dest port: 5001 mask: 0x0
        Action: Direct to queue 3

Filter: 2
        Rule Type: TCP over IPv6
        Src IP addr: fc00::2 mask: ::
        Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
        Traffic Class: 0x0 mask: 0xff
        Src port: 0 mask: 0xffff
        Dest port: 5001 mask: 0x0
        Action: Direct to queue 2

Filter: 3
        Rule Type: Raw IPv4
        Src IP addr: 192.168.51.101 mask: 0.0.0.0
        Dest IP addr: 0.0.0.0 mask: 255.255.255.255
        TOS: 0x0 mask: 0xff
        Protocol: 0 mask: 0xff
        L4 bytes: 0x0 mask: 0xffffffff
        Action: Direct to queue 1

Filter: 5
        Rule Type: Raw IPv6
        Src IP addr: fc00::2 mask: ::
        Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
        Traffic Class: 0x0 mask: 0xff
        Protocol: 0 mask: 0xff
        L4 bytes: 0x0 mask: 0xffffffff
        Action: Drop

---
v2: https://lore.kernel.org/netdev/20250908164046.25051-1-danielj@nvidia.com/
  - Fix sparse warnings
  - Fix memory leak on subsequent failure to allocate
  - Fix some Typos

v3: https://lore.kernel.org/netdev/20250923141920.283862-1-danielj@nvidia.com/
  - Added admin_ops to virtio_device kdoc.

v4:
  - Fixed double free bug inserting flows
  - Fixed incorrect protocol field check parsing ip4 headers.
  - (u8 *) changed to (void *)
  - Added kdoc comments to UAPI changes.
  - No longer split up virtio_net.c
  - Added config op to execute admin commands.
      - virtio_pci assigns vp_modern_admin_cmd_exec to this callback.
  - Moved admin command API to new core file virtio_admin_commands.c

v5:
  - Fixed compile error
  - Fixed static analysis warning on () after macro
  - Added missing fields to kdoc comments
  - Aligned parameter name between prototype and kdoc

v6:
  - Fix sparse warning "array of flexible structures" Jakub K/Simon H
  - Use new variable and validate ff_mask_size before set_cap. MST

v7:
  - Change virtnet_ff_init to return a value. Allow -EOPNOTSUPP. Xuan
  - Set ff->ff_{caps, mask, actions} NULL in error path. Paolo Abini
  - Move for (int i removal hung back a patch. Paolo Abini

v8
  - Removed unused num_classifiers. Jason Wang
  - Use real_ff_mask_size when setting the selector caps. Jason Wang

v9:
  - Set err to -ENOMEM after alloc failures in virtnet_ff_init. Simon H

v10:
  - Return -EOPNOTSUPP in virnet_ff_init before allocing any memory.
    Jason Wang/Paolo Abeni

v11:
  - Return -EINVAL if any resource limit is 0. Simon Horman
  - Ensure we don't overrun alloced space of ff->ff_mask by moving the
    real_ff_mask_size > ff_mask_size check into the loop. Simon Horman

v12: Many comments by MST, thanks Michael. Only the most significant
     listed here:
  - Fixed leak of key in build_and_insert.
  - Fixed setting ethhdr proto for IPv6.
  - Added 2 byte pad to struct virtio_net_ff_cap_data.
  - Use and set rule_cnt when querying all flows.
  - Cleanup and reinit in freeze/restore path.

v13:
  - Add private comment for reserved field in kdoc. Jakub
  - Serveral comments from MST details in patches. Most significant:
	- Fixed bug in ip4, check l3_mask vs mask when setting addrs.
	- Changed ff_mask cap checking to not break on expanded
	  selector types
	- Changed virtio_admin_obj_destroy to return void.
	- Check tos field for ip4.
	- Don't accept tclass field for ip6.
	- If ip6 only flow check that l4_proto isn't set.

v14:
  - Handle virtio_ff_init errors in freeze/restore. MST
  - Don't set proto in parse_ip4/6. The casted struct may not have that
    field, and the proto field was set explicitly anyway. Simon H/AI.

v15:
  - In virtnet_restore_up only call virtnet_close in err path if
    netif_running. AI

v16:
  - Return 0 from virtnet_restore_up if virtnet_init_ff return not
    supported. AI
  - Rebased over removing series to remove delayed refill.

v17:
  - Properly handle unaligned reads/writes. MST
  - Fix use after free if init fails during virtnet_restor. AI
  - Fix memory leak when validating the classifer vs caps fails. AI
  - Added missing includes. MSTA

v18:
  - Validate selector cap lengths, instead of just checking they don't
    exceed a max. AI
  - Add __count_by attribute to flexible arrays in UAPI definitions.
    Paolo A.

v19:
  - Style fixes. AI

v20:
  - Added missing include

v21:
  - Use le64_to_cpu() and BIT_ULL() instead of cpu_to_le64() for cap checking.
  - Don't use __counted_by on flexible array of flexible structs.
  - Replace UAPI header includes with kernel header includes.
  - Add missing includes for linux/types.h and linux/byteorder/generic.h.
  - Clamp the reported action count to the driver-supported maximum.
  - Clamp the reported selector count to the driver-supported maximum.
  - Validate sel->type is not 0.
  - Reduce selectors' count in case selector's type is invalid.
  - Move virtio_device_ready() before virtnet_ff_init() as the flow filter
    initialization requires the device to be in ready state to issue admin
    commands.
  - Remove forward declarations.
  - Validate action is supported before inserting rule.
  - Convert ring_cookie to vq before assigning ff_rule->vq_index.
  - reword a comment.
  - Introduce a new patch that moves flow_type_mask() to include/linux/ethtool.h.
  - Wrap __le32 limit fields in le32_to_cpu() to avoid sparse warnings.
  - Use put_unaligned() in parse_ip4() to avoid misaligned 32-bit stores on
    strict-alignment architectures.

v22:
  - Reword/fix typo in commit messages.
  - Remove include <uapi/linux/virtio_pci.h> from virtio_net.c.
  - Verify also selectors_per_classifier_limit in virtnet_ff_init().
  - Validate ff->ff_actions->count != 0.
  - Remove WARN_ON_ONCE() and replace -EINVAL with -EPROTO for errors on
    device side.
  - Use ff->ff_mask->count after it was initialized.
  - Add a patch to fix sleeping under spinlock in the admin command path.
  - Document that callers must zero-initialize the capability structure.
  - Convert macro VIRTIO_CAP_IN_LIST to be inline function.
  - Add WARN_ON_ONCE if allocation fails in virtio_admin_obj_destroy().
  - Change VIRTNET_FF_ETHTOOL_GROUP_PRIORITY to be 0.
  - Reject flow rules that require more selectors than the device supports
    (selectors_per_classifier_limit).
  - Report min(rules_limit, rules_per_group_limit) as the effective rule limit,
    since all rules reside in a single group.

v23:
  - Make virtio_admin_obj_destroy() return int instead of void + internal
    WARN_ON_ONCE, so callers can propagate the failure themselves.

Comments from internal Sashiko review:
> +int virtio_admin_obj_create(struct virtio_device *vdev,
> +			    u16 obj_type,
> +			    u32 obj_id,
> +			    u16 group_type,
> +			    u64 group_member_id,
> +			    const void *obj_specific_data,
> +			    size_t obj_specific_data_size)
> +{
[ ... ]
> +	obj_create_data->hdr.type = cpu_to_le16(obj_type);
> +	obj_create_data->hdr.id = cpu_to_le32(obj_id);
> +	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
> +	       obj_specific_data_size);
Can this memcpy trigger undefined behavior if callers pass NULL for
obj_specific_data and 0 for obj_specific_data_size? In C, passing a NULL
pointer to memcpy is undefined behavior even if the size is 0, which could
cause UBSAN splats.

[SS] It's the caller responsibly not to pass NULL. Current callers don't pass NULL.

> +	sg_init_one(&data_sg, data, data_size);
> +
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
> +	cmd.group_type = cpu_to_le16(group_type);
> +	cmd.group_member_id = cpu_to_le64(group_member_id);
> +	cmd.data_sg = &data_sg;
> +
> +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
This isn't a bug introduced by this patch, but does this execution path sleep
while holding a spinlock with interrupts disabled?
If this calls into virtqueue_exec_admin_cmd() (for example via
vp_modern_admin_cmd_exec()), the call chain looks like:
virtqueue_exec_admin_cmd()
    spin_lock_irqsave(&admin_vq->lock, flags);
    virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
Since GFP_KERNEL allows direct reclaim and sleeping, can this cause a deadlock
or trigger a BUG if the virtqueue falls back to allocating indirect descriptors?

[SS] Right, but not related to this series. Can introduce a fix in a different patch.

> +void virtio_admin_obj_destroy(struct virtio_device *vdev,
> +			      u16 obj_type,
> +			      u32 obj_id,
> +			      u16 group_type,
> +			      u64 group_member_id)
> +{
> +	struct virtio_admin_cmd_resource_obj_cmd_hdr *data;
> +	struct virtio_admin_cmd cmd = {};
> +	struct scatterlist data_sg;
> +	int err;
> +
> +	if (!vdev->config->admin_cmd_exec)
> +		return;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return;
If this kzalloc() fails, does this function return without sending the
destroy command to the hardware?
Since the return type is void, the caller has no way to detect this failure
and will likely free its local software tracking objects. Will this leave
the physical device and software tracking structures permanently
desynchronized, leaking the object on the virtio device?

[SS] Not much to do here. There could be a firmware error also,
     and then what we do? Destroy flow is best effort.

> +			break;
> +
> +		if (sel->length != get_mask_size(sel->type) ||
> +		    test_and_set_bit(sel->type, &sel_types)) {
> +			WARN_ON_ONCE(true);
Is it safe to use WARN_ON_ONCE() for validating device input? A malicious
or buggy virtio device could intentionally trigger this by providing an
invalid length or duplicate selector types, potentially leading to a kernel
panic if panic_on_warn is enabled. Would it be better to just return the
error without the warning?
> +			err = -EINVAL;
> +			goto err_ff_action;
> +		}
> +		real_ff_mask_size += sizeof(struct virtio_net_ff_selector) + sel->length;
> +		if (real_ff_mask_size > ff_mask_size) {
> +			WARN_ON_ONCE(true);
Similarly, could this WARN_ON_ONCE() be triggered by a malicious device
providing a larger size than expected, leading to a panic if panic_on_warn
is enabled?

[SS] These WARN_ON_ONCE() were requested by MST during review.

[ ... ]
> +static const struct ethtool_ops virtnet_ethtool_ops = {
[ ... ]
> +	.get_rxfh_fields = virtnet_get_hashflow,
> +	.set_rxfh_fields = virtnet_set_hashflow,
> +	.get_rx_ring_count = virtnet_get_rx_ring_count,
> +	.set_rxnfc = virtnet_set_rxnfc,
> +};
Does this also need an implementation of .get_rxnfc to prevent invalid
channel configurations?
When .get_rxnfc is missing, the ethtool core function
ethtool_get_max_rxnfc_channel() returns -EOPNOTSUPP. This causes
ethtool_check_max_channel() to silently skip validating the new channel
count against active flow rules.
If a user adds a flow rule targeting a specific RX queue, and then reduces
the active RX queues via ethtool -L, virtnet_set_channels() will update
the queue count without checking the active rules stored in
vi->ff.ethtool.rules.
Could this leave hardware rules pointing to disabled queues and cause
silent packet drops?

[SS] .get_rxnfc exists (introduced in a later patch).

> @@ -1258,6 +1259,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  	.get_shm_region  = vp_get_shm_region,
>  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
>  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> +	.admin_cmd_exec = vp_modern_admin_cmd_exec,
>  };
This isn't a bug introduced by this patch, but by exposing this function to
general drivers, can a driver trigger a NULL pointer dereference or
use-after-free?
If a driver calls the newly exposed config->admin_cmd_exec() early in probe
before find_vqs(), or late in remove after del_vqs(),
virtqueue_exec_admin_cmd() unconditionally dereferences admin_vq->info:
    vq = admin_vq->info->vq;
In vp_del_vqs(), the info structure is freed via kfree(info), but
vp_dev->admin_vq.info is never set to NULL, leaving a dangling pointer.

[SS] Current code is written in correct order. Future callers are responsible
     to call this when VQs are initialized.

This isn't a bug introduced by this patch, but does this error path lead to
stack corruption?
In virtqueue_exec_admin_cmd(), if virtqueue_kick() fails, it returns -EIO
without waiting for completion or detaching the exposed buffer:
    if (!virtqueue_kick(vq))
            goto unlock_err;
If the hardware later processes the descriptor and triggers
vp_modern_avq_done(), it will call complete(&cmd->completion). Since the cmd
structure was likely allocated on the caller's stack, this can overwrite
out-of-scope stack memory.

[SS] The vq->broken flag set by virtqueue_notify() causes virtqueue_get_buf()
     to return NULL in vp_modern_avq_done(), so complete() is never called on
     the freed cmd. No stack corruption.

This isn't a bug introduced by this patch, but can this function return an
uninitialized variable?
virtqueue_exec_admin_cmd() returns cmd->ret at the end of the function:
    return cmd->ret;
However, on a successful command execution, vp_modern_avq_done() never
explicitly sets cmd->ret to 0. If a driver allocates struct virtio_admin_cmd
on the stack without zero-initializing it, could this return stack garbage
and erroneously indicate failure?

[SS] cmd is initialized in all callers struct with virtio_admin_cmd cmd = {};

If the device responds with fewer bytes than the driver's provided data
buffer, does this leave the rest of the output buffer uninitialized?
The wrapper discards cmd.result_sg_size without zero-initializing data and
returns 0, which could expose uninitialized memory to callers.

[SS] Caller's responsibility to initialize the buffer.

Second, if virtqueue_add_sgs() returns -ENOSPC, the code loops infinitely
with cpu_relax() without a timeout or sleep:
drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
...
		if (ret == -ENOSPC) {
			spin_unlock_irqrestore(&admin_vq->lock, flags);
			cpu_relax();
			goto again;
		}
...
}
Can this soft-lockup the CPU if the virtqueue remains full and the device is
unresponsive?

[SS] Pre-existing, not introduced by this series, and bounded in practice by
     the virtqueue_is_broken() check. Not something to fix here.

Third, if virtqueue_kick() fails, the code jumps to unlock_err without
detaching the buffer from the virtqueue:
drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
...
	if (!virtqueue_kick(vq))
		goto unlock_err;
	spin_unlock_irqrestore(&admin_vq->lock, flags);
	wait_for_completion(&cmd->completion);
	return cmd->ret;
unlock_err:
	spin_unlock_irqrestore(&admin_vq->lock, flags);
	return -EIO;
...
}
Does this leave a dangling pointer on the virtqueue?
If the device later processes the command or the virtqueue is torn down,
could it write to the freed heap memory and invoke complete() on a dangling
stack pointer?

[SS] Not a bug. vq->broken prevents complete() from firing.

> +	sg_init_one(&data_sg, data, sizeof(*data));
> +	sg_init_one(&result_sg, caps, cap_size);
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DEVICE_CAP_GET);
> +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
> +	cmd.data_sg = &data_sg;
> +	cmd.result_sg = &result_sg;
> +
> +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
Similar to virtio_admin_cap_id_list_query above, does this execution discard
cmd.result_sg_size without zeroing the caps buffer, potentially returning
uninitialized memory to the caller on a short response?

[SS] Standard kernel API contract, caller owns the buffer, current callers
     handle it correctly.

> +	data->id = cpu_to_le16(id);
> +	memcpy(data->cap_specific_data, caps, cap_size);
If a caller invokes virtio_admin_cap_set() to enable a capability with no
extra data (cap_size == 0 and caps == NULL), will this unconditionally pass a
NULL pointer to memcpy(), invoking undefined behavior?

[SS] Caller's responsibility.

> +/**
> + * VIRTIO_CAP_IN_LIST - Check if a capability is supported in the capability list
> + * @cap_list: Pointer to capability list structure containing supported_caps array
> + * @cap: Capability ID to check
> + *
> + * The cap_list contains a supported_caps array of little-endian 64-bit integers
> + * where each bit represents a capability. Bit 0 of the first element represents
> + * capability ID 0, bit 1 represents capability ID 1, and so on.
> + *
> + * Return: 1 if capability is supported, 0 otherwise
> + */
> +#define VIRTIO_CAP_IN_LIST(cap_list, cap) \
> +	(!!(1 & (le64_to_cpu(cap_list->supported_caps[(cap) / 64]) >> (cap) % 64)))
Could this macro trigger an out-of-bounds read on the supported_caps array if
a caller passes a cap value greater than VIRTIO_ADMIN_MAX_CAP?
Additionally, could evaluating cap multiple times cause unexpected side effects
if passed an expression like cap++?

[SS] All callers use named cap constants defined in the uapi headers, which are all
     within bounds by construction. Passing side-effecting expressions to macros is
     standard caller responsibility. No fix needed.

>  static void virtnet_ff_cleanup(struct virtnet_ff *ff)
>  {
> +	struct virtnet_ethtool_rule *eth_rule;
> +	unsigned long i;
> +
>  	if (!ff->ff_supported)
>  		return;
>
> +	xa_for_each(&ff->ethtool.rules, i, eth_rule)
> +		destroy_ethtool_rule(ff, eth_rule);
> +
Will this cause user-configured flow filter rules to be permanently lost
across system suspend/resume or device migration?
During suspend, device reset, or device freeze, virtnet_freeze_down() calls
virtnet_ff_cleanup(). This loop iterates over all configured flow rules,
sends the destroy commands to the hardware, and then calls kfree() via
destroy_ethtool_rule(), permanently destroying the software representation of
the rules.
Upon resume, virtnet_restore_up() invokes virtnet_ff_init(), which
initializes the flow filters as completely empty. The driver appears to make
no attempt to retain the software state of the rules during suspend or
replay them to the device during restore, meaning users must manually
recreate all flow filter rules every time the system resumes or the device
is migrated.

[SS] This is intentional for now. Could be a follow up feature.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
-- 
2.49.0

[PATCH net-next v23 01/14] virtio_pci: Remove supported_caps cache and build assert

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Following patches introduce capability IDs above 63 (e.g. 0x800),
making the capability ID list larger than a single u64. Remove the
cached supported_caps field so the code uses data->supported_caps
directly, transparently handling a bitmap of any size. Remove the
build assert that enforced the list fit in 64 bits.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/virtio/virtio_pci_common.h |  1 -
 drivers/virtio/virtio_pci_modern.c | 10 +++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 8cd01de27baf..fc26e035e7a6 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -48,7 +48,6 @@ struct virtio_pci_admin_vq {
 	/* Protects virtqueue access. */
 	spinlock_t lock;
 	u64 supported_cmds;
-	u64 supported_caps;
 	u8 max_dev_parts_objects;
 	struct ida dev_parts_ida;
 	/* Name of the admin queue: avq.$vq_index. */
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 6d8ae2a6a8ca..ef4a64eb9982 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -304,10 +304,10 @@ virtio_pci_admin_cmd_dev_parts_objects_enable(struct virtio_device *virtio_dev)
 
 static void virtio_pci_admin_cmd_cap_init(struct virtio_device *virtio_dev)
 {
-	struct virtio_pci_device *vp_dev = to_vp_device(virtio_dev);
 	struct virtio_admin_cmd_query_cap_id_result *data;
 	struct virtio_admin_cmd cmd = {};
 	struct scatterlist result_sg;
+	u64 caps;
 	int ret;
 
 	data = kzalloc_obj(*data);
@@ -323,12 +323,8 @@ static void virtio_pci_admin_cmd_cap_init(struct virtio_device *virtio_dev)
 	if (ret)
 		goto end;
 
-	/* Max number of caps fits into a single u64 */
-	BUILD_BUG_ON(sizeof(data->supported_caps) > sizeof(u64));
-
-	vp_dev->admin_vq.supported_caps = le64_to_cpu(data->supported_caps[0]);
-
-	if (!(vp_dev->admin_vq.supported_caps & (1 << VIRTIO_DEV_PARTS_CAP)))
+	caps = le64_to_cpu(data->supported_caps[0]);
+	if (!(caps & BIT_ULL(VIRTIO_DEV_PARTS_CAP)))
 		goto end;
 
 	virtio_pci_admin_cmd_dev_parts_objects_enable(virtio_dev);
-- 
2.49.0

[PATCH net-next v23 02/14] virtio_pci: Fix sleeping under spinlock in admin command path

[Shahar Shitrit]

virtqueue_exec_admin_cmd() calls virtqueue_add_sgs() with GFP_KERNEL
while holding a spinlock. GFP_KERNEL allocations can sleep, which is
illegal in atomic context.

The issue becomes more significant with the next commit, which exposes
vp_modern_admin_cmd_exec() through virtio_config_ops. This makes the
admin command path available to device drivers and potentially reachable
from userspace, increasing the likelihood of hitting the invalid sleep
under memory pressure.

Fix this by replacing the spinlock with a mutex, which allows GFP_KERNEL
to sleep as needed. Since the virtqueue done callback runs in interrupt
context and cannot take a mutex, defer its work to a work item that runs
in process context.

Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/virtio/virtio_pci_common.c |  5 ++++
 drivers/virtio/virtio_pci_common.h |  5 +++-
 drivers/virtio/virtio_pci_modern.c | 37 +++++++++++++++++++-----------
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 164f480b18a6..ec0c92c782d6 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -256,6 +256,11 @@ static void vp_del_vq(struct virtqueue *vq, struct virtio_pci_vq_info *info)
 		spin_unlock_irqrestore(&vp_dev->lock, flags);
 	}
 
+	if (vp_is_avq(vq->vdev, vq->index)) {
+		cancel_work_sync(&vp_dev->admin_vq.work);
+		vp_dev->admin_vq.info = NULL;
+	}
+
 	vp_dev->del_vq(info);
 	kfree(info);
 }
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index fc26e035e7a6..8b0178295342 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -30,6 +30,7 @@
 #include <linux/highmem.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 struct virtio_pci_vq_info {
 	/* the actual virtqueue */
@@ -46,7 +47,9 @@ struct virtio_pci_admin_vq {
 	/* Virtqueue info associated with this admin queue. */
 	struct virtio_pci_vq_info *info;
 	/* Protects virtqueue access. */
-	spinlock_t lock;
+	struct mutex lock;
+	/* Admin command completion work. */
+	struct work_struct work;
 	u64 supported_cmds;
 	u8 max_dev_parts_objects;
 	struct ida dev_parts_ida;
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index ef4a64eb9982..dab63b044a31 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -47,13 +47,20 @@ static int vp_avq_index(struct virtio_device *vdev, u16 *index, u16 *num)
 void vp_modern_avq_done(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
+
+	schedule_work(&vp_dev->admin_vq.work);
+}
+
+static void vp_modern_avq_work(struct work_struct *work)
+{
 	unsigned int status_size = sizeof(struct virtio_admin_cmd_status);
+	struct virtio_pci_admin_vq *admin_vq =
+		container_of(work, struct virtio_pci_admin_vq, work);
+	struct virtqueue *vq = admin_vq->info->vq;
 	struct virtio_admin_cmd *cmd;
-	unsigned long flags;
 	unsigned int len;
 
-	spin_lock_irqsave(&admin_vq->lock, flags);
+	mutex_lock(&admin_vq->lock);
 	do {
 		virtqueue_disable_cb(vq);
 		while ((cmd = virtqueue_get_buf(vq, &len))) {
@@ -71,7 +78,7 @@ void vp_modern_avq_done(struct virtqueue *vq)
 			complete(&cmd->completion);
 		}
 	} while (!virtqueue_enable_cb(vq));
-	spin_unlock_irqrestore(&admin_vq->lock, flags);
+	mutex_unlock(&admin_vq->lock);
 }
 
 static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
@@ -82,7 +89,6 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
 				    struct virtio_admin_cmd *cmd)
 {
 	struct virtqueue *vq;
-	unsigned long flags;
 	int ret;
 
 	vq = admin_vq->info->vq;
@@ -100,11 +106,11 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
 	if (virtqueue_is_broken(vq))
 		return -EIO;
 
-	spin_lock_irqsave(&admin_vq->lock, flags);
+	mutex_lock(&admin_vq->lock);
 	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
 	if (ret < 0) {
 		if (ret == -ENOSPC) {
-			spin_unlock_irqrestore(&admin_vq->lock, flags);
+			mutex_unlock(&admin_vq->lock);
 			cpu_relax();
 			goto again;
 		}
@@ -112,14 +118,14 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
 	}
 	if (!virtqueue_kick(vq))
 		goto unlock_err;
-	spin_unlock_irqrestore(&admin_vq->lock, flags);
+	mutex_unlock(&admin_vq->lock);
 
 	wait_for_completion(&cmd->completion);
 
 	return cmd->ret;
 
 unlock_err:
-	spin_unlock_irqrestore(&admin_vq->lock, flags);
+	mutex_unlock(&admin_vq->lock);
 	return -EIO;
 }
 
@@ -350,6 +356,8 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
 	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
 		return;
 
+	cancel_work_sync(&vp_dev->admin_vq.work);
+
 	vq = vp_dev->admin_vq.info->vq;
 	if (!vq)
 		return;
@@ -554,10 +562,12 @@ static void vp_reset(struct virtio_device *vdev)
 	while (vp_modern_get_status(mdev))
 		msleep(1);
 
-	vp_modern_avq_cleanup(vdev);
-
-	/* Flush pending VQ/configuration callbacks. */
+	/* Flush pending VQ/configuration callbacks before cleanup, so that
+	 * vp_modern_avq_done() can no longer schedule admin_vq.work.
+	 */
 	vp_synchronize_vectors(vdev);
+
+	vp_modern_avq_cleanup(vdev);
 }
 
 static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)
@@ -1284,8 +1294,9 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->avq_index = vp_avq_index;
 	vp_dev->isr = mdev->isr;
 	vp_dev->vdev.id = mdev->id;
+	mutex_init(&vp_dev->admin_vq.lock);
+	INIT_WORK(&vp_dev->admin_vq.work, vp_modern_avq_work);
 
-	spin_lock_init(&vp_dev->admin_vq.lock);
 	return 0;
 }
 
-- 
2.49.0

[PATCH net-next v23 03/14] virtio: Add config_op for admin commands

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

This will allow device drivers to issue administration commands.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/virtio/virtio_pci_modern.c | 2 ++
 include/linux/virtio_config.h      | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index dab63b044a31..3b2515e29728 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -1248,6 +1248,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.get_shm_region  = vp_get_shm_region,
 	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
 	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
+	.admin_cmd_exec = vp_modern_admin_cmd_exec,
 };
 
 static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -1268,6 +1269,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.get_shm_region  = vp_get_shm_region,
 	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
 	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
+	.admin_cmd_exec = vp_modern_admin_cmd_exec,
 };
 
 /* the PCI probing function */
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 69f84ea85d71..e36a32e0a20c 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -108,6 +108,10 @@ struct virtqueue_info {
  *	Returns 0 on success or error status
  *	If disable_vq_and_reset is set, then enable_vq_after_reset must also be
  *	set.
+ * @admin_cmd_exec: Execute an admin VQ command (optional).
+ *	vdev: the virtio_device
+ *	cmd: the command to execute
+ *	Returns 0 on success or error status
  */
 struct virtio_config_ops {
 	void (*get)(struct virtio_device *vdev, unsigned offset,
@@ -137,6 +141,8 @@ struct virtio_config_ops {
 			       struct virtio_shm_region *region, u8 id);
 	int (*disable_vq_and_reset)(struct virtqueue *vq);
 	int (*enable_vq_after_reset)(struct virtqueue *vq);
+	int (*admin_cmd_exec)(struct virtio_device *vdev,
+			      struct virtio_admin_cmd *cmd);
 };
 
 /**
-- 
2.49.0

[PATCH net-next v23 04/14] virtio: Expose generic device capability operations

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

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. Since the API does not report how
much of the result was written, callers must zero-initialize the
structure before use. Any portion not written by the device then remains
zero and indicates that those capabilities are not supported.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/virtio/Makefile                |  2 +-
 drivers/virtio/virtio_admin_commands.c | 96 ++++++++++++++++++++++++++
 include/linux/virtio_admin.h           | 89 ++++++++++++++++++++++++
 include/uapi/linux/virtio_pci.h        |  6 +-
 4 files changed, 190 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..60d92875da23
--- /dev/null
+++ b/drivers/virtio/virtio_admin_commands.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_admin.h>
+#include <linux/overflow.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)
+{
+	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_obj(*data);
+	if (!data)
+		return -ENOMEM;
+
+	data->id = cpu_to_le16(id);
+	sg_init_one(&data_sg, data, sizeof(*data));
+	sg_init_one(&result_sg, caps, cap_size);
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DEVICE_CAP_GET);
+	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
+	cmd.data_sg = &data_sg;
+	cmd.result_sg = &result_sg;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtio_admin_cap_get);
+
+int virtio_admin_cap_set(struct virtio_device *vdev,
+			 u16 id,
+			 const void *caps,
+			 size_t cap_size)
+{
+	struct virtio_admin_cmd_cap_set_data *data;
+	struct virtio_admin_cmd cmd = {};
+	struct scatterlist data_sg;
+	size_t data_size;
+	int err;
+
+	if (!vdev->config->admin_cmd_exec)
+		return -EOPNOTSUPP;
+
+	if (check_add_overflow(sizeof(*data), cap_size, &data_size))
+		return -EOVERFLOW;
+
+	data = kzalloc(data_size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->id = cpu_to_le16(id);
+	memcpy(data->cap_specific_data, caps, cap_size);
+	sg_init_one(&data_sg, data, data_size);
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DRIVER_CAP_SET);
+	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
+	cmd.data_sg = &data_sg;
+	cmd.result_sg = NULL;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtio_admin_cap_set);
diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
new file mode 100644
index 000000000000..1177c4a20e53
--- /dev/null
+++ b/include/linux/virtio_admin.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Header file for virtio admin operations
+ */
+
+#ifndef _LINUX_VIRTIO_ADMIN_H
+#define _LINUX_VIRTIO_ADMIN_H
+
+#include <linux/bug.h>
+#include <linux/types.h>
+#include <linux/byteorder/generic.h>
+#include <uapi/linux/virtio_pci.h>
+
+struct virtio_device;
+
+/**
+ * virtio_cap_in_list - Check if a capability is supported in the capability list
+ * @cap_list: Pointer to capability list structure containing supported_caps array
+ * @cap: Capability ID to check
+ *
+ * The cap_list contains a supported_caps array of little-endian 64-bit integers
+ * where each bit represents a capability. Bit 0 of the first element represents
+ * capability ID 0, bit 1 represents capability ID 1, and so on.
+ *
+ * Return: true if capability is supported, false otherwise
+ */
+static inline bool virtio_cap_in_list(
+	const struct virtio_admin_cmd_query_cap_id_result *cap_list, u16 cap)
+{
+	BUILD_BUG_ON(cap > VIRTIO_ADMIN_MAX_CAP);
+	return !!(1 & (le64_to_cpu(cap_list->supported_caps[cap / 64]) >>
+		       (cap % 64)));
+}
+
+/**
+ * virtio_admin_cap_id_list_query - Query the list of available capability IDs
+ * @vdev: The virtio device to query
+ * @data: Pointer to result structure (must be zero-initialized and heap allocated)
+ *
+ * This function queries the virtio device for the list of available capability
+ * IDs that can be used with virtio_admin_cap_get() and virtio_admin_cap_set().
+ * The result is stored in the provided data structure.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
+ * operations or capability queries, or a negative error code on other failures.
+ */
+int virtio_admin_cap_id_list_query(struct virtio_device *vdev,
+				   struct virtio_admin_cmd_query_cap_id_result *data);
+
+/**
+ * virtio_admin_cap_get - Get capability data for a specific capability ID
+ * @vdev: The virtio device
+ * @id: Capability ID to retrieve
+ * @caps: Pointer to capability data structure (must be heap allocated)
+ * @cap_size: Size of the capability data structure
+ *
+ * This function retrieves a specific capability from the virtio device.
+ * The capability data is stored in the provided buffer. The caller must
+ * ensure the buffer is large enough to hold the capability data.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
+ * operations or capability retrieval, or a negative error code on other failures.
+ */
+int virtio_admin_cap_get(struct virtio_device *vdev,
+			 u16 id,
+			 void *caps,
+			 size_t cap_size);
+
+/**
+ * virtio_admin_cap_set - Set capability data for a specific capability ID
+ * @vdev: The virtio device
+ * @id: Capability ID to set
+ * @caps: Pointer to capability data structure (must be heap allocated)
+ * @cap_size: Size of the capability data structure
+ *
+ * This function sets a specific capability on the virtio device.
+ * The capability data is read from the provided buffer and applied
+ * to the device. The device may validate the capability data before
+ * applying it.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
+ * operations or capability setting, or a negative error code on other failures.
+ */
+int virtio_admin_cap_set(struct virtio_device *vdev,
+			 u16 id,
+			 const void *caps,
+			 size_t cap_size);
+
+#endif /* _LINUX_VIRTIO_ADMIN_H */
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index e732e3456e27..96d097d3757e 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -315,15 +315,17 @@ struct virtio_admin_cmd_notify_info_result {
 
 #define VIRTIO_DEV_PARTS_CAP 0x0000
 
+#define VIRTIO_ADMIN_MAX_CAP 0x0fff
+
 struct virtio_dev_parts_cap {
 	__u8 get_parts_resource_objects_limit;
 	__u8 set_parts_resource_objects_limit;
 };
 
-#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 + 1, 64)
 
 struct virtio_admin_cmd_query_cap_id_result {
-	__le64 supported_caps[MAX_CAP_ID];
+	__le64 supported_caps[VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE];
 };
 
 struct virtio_admin_cmd_cap_get_data {
-- 
2.49.0

[PATCH net-next v23 05/14] virtio: Expose object create and destroy API

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Object create and destroy were implemented specifically for dev parts
device objects. Create general purpose APIs for use by upper layer
drivers.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/virtio/virtio_admin_commands.c | 77 ++++++++++++++++++++++++++
 include/linux/virtio_admin.h           | 44 +++++++++++++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/virtio/virtio_admin_commands.c b/drivers/virtio/virtio_admin_commands.c
index 60d92875da23..24b06075d944 100644
--- a/drivers/virtio/virtio_admin_commands.c
+++ b/drivers/virtio/virtio_admin_commands.c
@@ -94,3 +94,80 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
 	return err;
 }
 EXPORT_SYMBOL_GPL(virtio_admin_cap_set);
+
+int virtio_admin_obj_create(struct virtio_device *vdev,
+			    u16 obj_type,
+			    u32 obj_id,
+			    u16 group_type,
+			    u64 group_member_id,
+			    const void *obj_specific_data,
+			    size_t obj_specific_data_size)
+{
+	size_t data_size = sizeof(struct virtio_admin_cmd_resource_obj_create_data);
+	struct virtio_admin_cmd_resource_obj_create_data *obj_create_data;
+	struct virtio_admin_cmd cmd = {};
+	struct scatterlist data_sg;
+	void *data;
+	int err;
+
+	if (!vdev->config->admin_cmd_exec)
+		return -EOPNOTSUPP;
+
+	if (check_add_overflow(data_size, obj_specific_data_size, &data_size))
+		return -EOVERFLOW;
+
+	data = kzalloc(data_size, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	obj_create_data = data;
+	obj_create_data->hdr.type = cpu_to_le16(obj_type);
+	obj_create_data->hdr.id = cpu_to_le32(obj_id);
+	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
+	       obj_specific_data_size);
+	sg_init_one(&data_sg, data, data_size);
+
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
+	cmd.group_type = cpu_to_le16(group_type);
+	cmd.group_member_id = cpu_to_le64(group_member_id);
+	cmd.data_sg = &data_sg;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtio_admin_obj_create);
+
+int virtio_admin_obj_destroy(struct virtio_device *vdev,
+			     u16 obj_type,
+			     u32 obj_id,
+			     u16 group_type,
+			     u64 group_member_id)
+{
+	struct virtio_admin_cmd_resource_obj_cmd_hdr *data;
+	struct virtio_admin_cmd cmd = {};
+	struct scatterlist data_sg;
+	int err;
+
+	if (!vdev->config->admin_cmd_exec)
+		return -EOPNOTSUPP;
+
+	data = kzalloc_obj(*data);
+	if (!data)
+		return -ENOMEM;
+
+	data->type = cpu_to_le16(obj_type);
+	data->id = cpu_to_le32(obj_id);
+	sg_init_one(&data_sg, data, sizeof(*data));
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_DESTROY);
+	cmd.group_type = cpu_to_le16(group_type);
+	cmd.group_member_id = cpu_to_le64(group_member_id);
+	cmd.data_sg = &data_sg;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtio_admin_obj_destroy);
diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
index 1177c4a20e53..ebd4106835d6 100644
--- a/include/linux/virtio_admin.h
+++ b/include/linux/virtio_admin.h
@@ -86,4 +86,48 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
 			 const void *caps,
 			 size_t cap_size);
 
+/**
+ * virtio_admin_obj_create - Create an object on a virtio device
+ * @vdev: the virtio device
+ * @obj_type: type of object to create
+ * @obj_id: ID for the new object
+ * @group_type: administrative group type for the operation
+ * @group_member_id: member identifier within the administrative group
+ * @obj_specific_data: object-specific data for creation
+ * @obj_specific_data_size: size of the object-specific data in bytes
+ *
+ * Creates a new object on the virtio device with the specified type and ID.
+ * The object may require object-specific data for proper initialization.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
+ * operations or object creation, or a negative error code on other failures.
+ */
+int virtio_admin_obj_create(struct virtio_device *vdev,
+			    u16 obj_type,
+			    u32 obj_id,
+			    u16 group_type,
+			    u64 group_member_id,
+			    const void *obj_specific_data,
+			    size_t obj_specific_data_size);
+
+/**
+ * virtio_admin_obj_destroy - Destroy an object on a virtio device
+ * @vdev: the virtio device
+ * @obj_type: type of object to destroy
+ * @obj_id: ID of the object to destroy
+ * @group_type: administrative group type for the operation
+ * @group_member_id: member identifier within the administrative group
+ *
+ * Destroys an existing object on the virtio device with the specified type
+ * and ID.
+ *
+ * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
+ * operations, or a negative error code on other failures.
+ */
+int virtio_admin_obj_destroy(struct virtio_device *vdev,
+			     u16 obj_type,
+			     u32 obj_id,
+			     u16 group_type,
+			     u64 group_member_id);
+
 #endif /* _LINUX_VIRTIO_ADMIN_H */
-- 
2.49.0

[PATCH net-next v23 06/14] virtio_net: Query and set flow filter caps

[Shahar Shitrit]

When probing a virtnet device, attempt to read the flow filter
capabilities. In order to use the feature the caps must also
be set. For now setting what was read is sufficient.

This patch adds uapi definitions virtio_net flow filters define in
version 1.4 of the VirtIO spec.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c           | 236 +++++++++++++++++++++++++++++
 include/uapi/linux/virtio_net_ff.h |  91 +++++++++++
 2 files changed, 327 insertions(+)
 create mode 100644 include/uapi/linux/virtio_net_ff.h

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e34c52d059d3..4f78b881e493 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -27,6 +27,10 @@
 #include <net/netdev_queues.h>
 #include <net/xdp_sock_drv.h>
 #include <net/page_pool/helpers.h>
+#include <linux/virtio_admin.h>
+#include <net/ipv6.h>
+#include <net/ip.h>
+#include <uapi/linux/virtio_net_ff.h>
 
 static int napi_weight = NAPI_POLL_WEIGHT;
 module_param(napi_weight, int, 0444);
@@ -282,6 +286,14 @@ static const struct virtnet_stat_desc virtnet_stats_tx_speed_desc_qstat[] = {
 	VIRTNET_STATS_DESC_TX_QSTAT(speed, ratelimit_packets, hw_drop_ratelimits),
 };
 
+struct virtnet_ff {
+	struct virtio_device *vdev;
+	bool ff_supported;
+	struct virtio_net_ff_cap_data *ff_caps;
+	struct virtio_net_ff_cap_mask_data *ff_mask;
+	struct virtio_net_ff_actions *ff_actions;
+};
+
 #define VIRTNET_Q_TYPE_RX 0
 #define VIRTNET_Q_TYPE_TX 1
 #define VIRTNET_Q_TYPE_CQ 2
@@ -474,6 +486,8 @@ struct virtnet_info {
 
 	struct virtio_net_rss_config_hdr *rss_hdr;
 
+	struct virtnet_ff ff;
+
 	/* Must be last as it ends in a flexible-array member. */
 	TRAILING_OVERLAP(struct virtio_net_rss_config_trailer, rss_trailer, hash_key_data,
 		u8 rss_hash_key_data[NETDEV_RSS_KEY_LEN];
@@ -519,6 +533,7 @@ static struct sk_buff *virtnet_skb_append_frag(struct receive_queue *rq,
 static void virtnet_xsk_completed(struct send_queue *sq, int num);
 static void free_unused_bufs(struct virtnet_info *vi);
 static void virtnet_del_vqs(struct virtnet_info *vi);
+static void remove_vq_common(struct virtnet_info *vi);
 
 enum virtnet_xmit_type {
 	VIRTNET_XMIT_TYPE_SKB,
@@ -5724,6 +5739,197 @@ static const struct netdev_stat_ops virtnet_stat_ops = {
 	.get_base_stats		= virtnet_get_base_stats,
 };
 
+static size_t get_mask_size(u16 type)
+{
+	switch (type) {
+	case VIRTIO_NET_FF_MASK_TYPE_ETH:
+		return sizeof(struct ethhdr);
+	case VIRTIO_NET_FF_MASK_TYPE_IPV4:
+		return sizeof(struct iphdr);
+	case VIRTIO_NET_FF_MASK_TYPE_IPV6:
+		return sizeof(struct ipv6hdr);
+	case VIRTIO_NET_FF_MASK_TYPE_TCP:
+		return sizeof(struct tcphdr);
+	case VIRTIO_NET_FF_MASK_TYPE_UDP:
+		return sizeof(struct udphdr);
+	}
+
+	return 0;
+}
+
+static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
+{
+	size_t ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data) +
+			      sizeof(struct virtio_net_ff_selector) *
+			      VIRTIO_NET_FF_MASK_TYPE_MAX;
+	struct virtio_admin_cmd_query_cap_id_result *cap_id_list;
+	struct virtio_net_ff_selector *sel;
+	unsigned long sel_types = 0;
+	size_t real_ff_mask_size;
+	int err;
+	int i;
+
+	if (!vdev->config->admin_cmd_exec)
+		return -EOPNOTSUPP;
+
+	cap_id_list = kzalloc(sizeof(*cap_id_list), GFP_KERNEL);
+	if (!cap_id_list)
+		return -ENOMEM;
+
+	err = virtio_admin_cap_id_list_query(vdev, cap_id_list);
+	if (err)
+		goto err_cap_list;
+
+	if (!(virtio_cap_in_list(cap_id_list, VIRTIO_NET_FF_RESOURCE_CAP) &&
+	      virtio_cap_in_list(cap_id_list, VIRTIO_NET_FF_SELECTOR_CAP) &&
+	      virtio_cap_in_list(cap_id_list, VIRTIO_NET_FF_ACTION_CAP))) {
+		err = -EOPNOTSUPP;
+		goto err_cap_list;
+	}
+
+	ff->ff_caps = kzalloc(sizeof(*ff->ff_caps), GFP_KERNEL);
+	if (!ff->ff_caps) {
+		err = -ENOMEM;
+		goto err_cap_list;
+	}
+
+	err = virtio_admin_cap_get(vdev,
+				   VIRTIO_NET_FF_RESOURCE_CAP,
+				   ff->ff_caps,
+				   sizeof(*ff->ff_caps));
+
+	if (err)
+		goto err_ff;
+
+	if (!le32_to_cpu(ff->ff_caps->groups_limit) ||
+	    !le32_to_cpu(ff->ff_caps->classifiers_limit) ||
+	    !le32_to_cpu(ff->ff_caps->rules_limit) ||
+	    !le32_to_cpu(ff->ff_caps->rules_per_group_limit) ||
+	    !ff->ff_caps->selectors_per_classifier_limit) {
+		err = -EINVAL;
+		goto err_ff;
+	}
+
+	/* VIRTIO_NET_FF_MASK_TYPE start at 1 */
+	for (i = 1; i <= VIRTIO_NET_FF_MASK_TYPE_MAX; i++)
+		ff_mask_size += get_mask_size(i);
+
+	ff->ff_mask = kzalloc(ff_mask_size, GFP_KERNEL);
+	if (!ff->ff_mask) {
+		err = -ENOMEM;
+		goto err_ff;
+	}
+
+	err = virtio_admin_cap_get(vdev,
+				   VIRTIO_NET_FF_SELECTOR_CAP,
+				   ff->ff_mask,
+				   ff_mask_size);
+
+	if (err)
+		goto err_ff_mask;
+
+	ff->ff_mask->count = min_t(u8, ff->ff_mask->count,
+				   VIRTIO_NET_FF_MASK_TYPE_MAX);
+
+	ff->ff_actions = kzalloc(sizeof(*ff->ff_actions) +
+					VIRTIO_NET_FF_ACTION_MAX,
+					GFP_KERNEL);
+	if (!ff->ff_actions) {
+		err = -ENOMEM;
+		goto err_ff_mask;
+	}
+
+	err = virtio_admin_cap_get(vdev,
+				   VIRTIO_NET_FF_ACTION_CAP,
+				   ff->ff_actions,
+				   sizeof(*ff->ff_actions) + VIRTIO_NET_FF_ACTION_MAX);
+
+	if (err)
+		goto err_ff_action;
+
+	ff->ff_actions->count = min_t(u8, ff->ff_actions->count,
+				      VIRTIO_NET_FF_ACTION_MAX);
+	if (!ff->ff_actions->count)
+		goto err_ff_action;
+
+	err = virtio_admin_cap_set(vdev,
+				   VIRTIO_NET_FF_RESOURCE_CAP,
+				   ff->ff_caps,
+				   sizeof(*ff->ff_caps));
+	if (err)
+		goto err_ff_action;
+
+	real_ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data);
+	sel = (void *)&ff->ff_mask->selectors;
+
+	for (i = 0; i < ff->ff_mask->count; i++) {
+		/* If the selector type is unknown it may indicate the spec
+		 * has been revised to include new types of selectors
+		 */
+		if (!sel->type || sel->type > VIRTIO_NET_FF_MASK_TYPE_MAX)
+			break;
+
+		if (sel->length != get_mask_size(sel->type) ||
+		    test_and_set_bit(sel->type, &sel_types)) {
+			err = -EPROTO;
+			goto err_ff_action;
+		}
+		real_ff_mask_size += sizeof(*sel) + sel->length;
+		if (real_ff_mask_size > ff_mask_size) {
+			err = -EPROTO;
+			goto err_ff_action;
+		}
+		sel = (void *)sel + sizeof(*sel) + sel->length;
+	}
+	ff->ff_mask->count = i;
+
+	err = virtio_admin_cap_set(vdev,
+				   VIRTIO_NET_FF_SELECTOR_CAP,
+				   ff->ff_mask,
+				   real_ff_mask_size);
+	if (err)
+		goto err_ff_action;
+
+	err = virtio_admin_cap_set(vdev,
+				   VIRTIO_NET_FF_ACTION_CAP,
+				   ff->ff_actions,
+				   sizeof(*ff->ff_actions) + ff->ff_actions->count);
+	if (err)
+		goto err_ff_action;
+
+	ff->vdev = vdev;
+	ff->ff_supported = true;
+
+	kfree(cap_id_list);
+
+	return 0;
+
+err_ff_action:
+	kfree(ff->ff_actions);
+	ff->ff_actions = NULL;
+err_ff_mask:
+	kfree(ff->ff_mask);
+	ff->ff_mask = NULL;
+err_ff:
+	kfree(ff->ff_caps);
+	ff->ff_caps = NULL;
+err_cap_list:
+	kfree(cap_id_list);
+
+	return err;
+}
+
+static void virtnet_ff_cleanup(struct virtnet_ff *ff)
+{
+	if (!ff->ff_supported)
+		return;
+
+	kfree(ff->ff_actions);
+	kfree(ff->ff_mask);
+	kfree(ff->ff_caps);
+	ff->ff_supported = false;
+}
+
 static void virtnet_freeze_down(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
@@ -5742,6 +5948,10 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
 	netif_tx_lock_bh(vi->dev);
 	netif_device_detach(vi->dev);
 	netif_tx_unlock_bh(vi->dev);
+
+	rtnl_lock();
+	virtnet_ff_cleanup(&vi->ff);
+	rtnl_unlock();
 }
 
 static int init_vqs(struct virtnet_info *vi);
@@ -5771,9 +5981,24 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 			goto err_destroy_pools;
 	}
 
+	/*
+	 * Initialize flow filters. Not supported is an acceptable and common
+	 * return code
+	 */
+	rtnl_lock();
+	err = virtnet_ff_init(&vi->ff, vi->vdev);
+	if (err && err != -EOPNOTSUPP) {
+		rtnl_unlock();
+		virtnet_freeze_down(vi->vdev);
+		remove_vq_common(vi);
+		return err;
+	}
+	rtnl_unlock();
+
 	netif_tx_lock_bh(vi->dev);
 	netif_device_attach(vi->dev);
 	netif_tx_unlock_bh(vi->dev);
+
 	return 0;
 
 err_destroy_pools:
@@ -7057,6 +7282,15 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 	virtio_device_ready(vdev);
 
+	/* Initialize flow filters. Not supported is an acceptable and common
+	 * return code
+	 */
+	err = virtnet_ff_init(&vi->ff, vi->vdev);
+	if (err && err != -EOPNOTSUPP) {
+		rtnl_unlock();
+		goto free_unregister_netdev;
+	}
+
 	if (vi->has_rss || vi->has_rss_hash_report) {
 		if (!virtnet_commit_rss_command(vi)) {
 			dev_warn(&vdev->dev, "RSS disabled because committing failed.\n");
@@ -7139,6 +7373,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 free_unregister_netdev:
 	unregister_netdev(dev);
+	virtnet_ff_cleanup(&vi->ff);
 free_failover:
 	net_failover_destroy(vi->failover);
 free_page_pools:
@@ -7189,6 +7424,7 @@ static void virtnet_remove(struct virtio_device *vdev)
 	virtnet_free_irq_moder(vi);
 
 	unregister_netdev(vi->dev);
+	virtnet_ff_cleanup(&vi->ff);
 
 	net_failover_destroy(vi->failover);
 
diff --git a/include/uapi/linux/virtio_net_ff.h b/include/uapi/linux/virtio_net_ff.h
new file mode 100644
index 000000000000..f9e771a0baff
--- /dev/null
+++ b/include/uapi/linux/virtio_net_ff.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
+ *
+ * Header file for virtio_net flow filters
+ */
+#ifndef _LINUX_VIRTIO_NET_FF_H
+#define _LINUX_VIRTIO_NET_FF_H
+
+#include <linux/types.h>
+#include <linux/stddef.h>
+
+#define VIRTIO_NET_FF_RESOURCE_CAP 0x800
+#define VIRTIO_NET_FF_SELECTOR_CAP 0x801
+#define VIRTIO_NET_FF_ACTION_CAP 0x802
+
+/**
+ * struct virtio_net_ff_cap_data - Flow filter resource capability limits
+ * @groups_limit: maximum number of flow filter groups supported by the device
+ * @classifiers_limit: maximum number of classifiers supported by the device
+ * @rules_limit: maximum number of rules supported device-wide across all groups
+ * @rules_per_group_limit: maximum number of rules allowed in a single group
+ * @last_rule_priority: priority value associated with the lowest-priority rule
+ * @selectors_per_classifier_limit: maximum selectors allowed in one classifier
+ */
+struct virtio_net_ff_cap_data {
+	__le32 groups_limit;
+	__le32 classifiers_limit;
+	__le32 rules_limit;
+	__le32 rules_per_group_limit;
+	__u8 last_rule_priority;
+	__u8 selectors_per_classifier_limit;
+	/* private: */
+	__u8 reserved[2];
+};
+
+/**
+ * struct virtio_net_ff_selector - Selector mask descriptor
+ * @type: selector type, one of VIRTIO_NET_FF_MASK_TYPE_* constants
+ * @flags: selector flags, see VIRTIO_NET_FF_MASK_F_* constants
+ * @reserved: must be set to 0 by the driver and ignored by the device
+ * @length: size in bytes of @mask
+ * @reserved1: must be set to 0 by the driver and ignored by the device
+ * @mask: variable-length mask payload for @type, length given by @length
+ *
+ * A selector describes a header mask that a classifier can apply. The format
+ * of @mask depends on @type.
+ */
+struct virtio_net_ff_selector {
+	__u8 type;
+	__u8 flags;
+	__u8 reserved[2];
+	__u8 length;
+	__u8 reserved1[3];
+	__u8 mask[] __counted_by(length);
+};
+
+#define VIRTIO_NET_FF_MASK_TYPE_ETH  1
+#define VIRTIO_NET_FF_MASK_TYPE_IPV4 2
+#define VIRTIO_NET_FF_MASK_TYPE_IPV6 3
+#define VIRTIO_NET_FF_MASK_TYPE_TCP  4
+#define VIRTIO_NET_FF_MASK_TYPE_UDP  5
+#define VIRTIO_NET_FF_MASK_TYPE_MAX  VIRTIO_NET_FF_MASK_TYPE_UDP
+
+/**
+ * struct virtio_net_ff_cap_mask_data - Supported selector mask formats
+ * @count: number of entries in @selectors
+ * @reserved: must be set to 0 by the driver and ignored by the device
+ * @selectors: packed array of struct virtio_net_ff_selector.
+ */
+struct virtio_net_ff_cap_mask_data {
+	__u8 count;
+	__u8 reserved[7];
+	__u8 selectors[];
+};
+
+#define VIRTIO_NET_FF_MASK_F_PARTIAL_MASK (1 << 0)
+
+#define VIRTIO_NET_FF_ACTION_DROP 1
+#define VIRTIO_NET_FF_ACTION_RX_VQ 2
+#define VIRTIO_NET_FF_ACTION_MAX  VIRTIO_NET_FF_ACTION_RX_VQ
+/**
+ * struct virtio_net_ff_actions - Supported flow actions
+ * @count: number of supported actions in @actions
+ * @reserved: must be set to 0 by the driver and ignored by the device
+ * @actions: array of action identifiers (VIRTIO_NET_FF_ACTION_*)
+ */
+struct virtio_net_ff_actions {
+	__u8 count;
+	__u8 reserved[7];
+	__u8 actions[] __counted_by(count);
+};
+#endif
-- 
2.49.0

[PATCH net-next v23 07/14] virtio_net: Create a FF group for ethtool steering

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

All ethtool steering rules will go in one group, create it during
initialization.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c           | 32 ++++++++++++++++++++++++++++++
 include/uapi/linux/virtio_net_ff.h | 15 ++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f78b881e493..294cfd70662d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -286,6 +286,9 @@ static const struct virtnet_stat_desc virtnet_stats_tx_speed_desc_qstat[] = {
 	VIRTNET_STATS_DESC_TX_QSTAT(speed, ratelimit_packets, hw_drop_ratelimits),
 };
 
+#define VIRTNET_FF_ETHTOOL_GROUP_PRIORITY 0
+#define VIRTNET_FF_MAX_GROUPS 1
+
 struct virtnet_ff {
 	struct virtio_device *vdev;
 	bool ff_supported;
@@ -5762,6 +5765,7 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 	size_t ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data) +
 			      sizeof(struct virtio_net_ff_selector) *
 			      VIRTIO_NET_FF_MASK_TYPE_MAX;
+	struct virtio_net_resource_obj_ff_group ethtool_group = {};
 	struct virtio_admin_cmd_query_cap_id_result *cap_id_list;
 	struct virtio_net_ff_selector *sel;
 	unsigned long sel_types = 0;
@@ -5852,6 +5856,12 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 	if (!ff->ff_actions->count)
 		goto err_ff_action;
 
+	if (le32_to_cpu(ff->ff_caps->groups_limit) < VIRTNET_FF_MAX_GROUPS) {
+		err = -ENOSPC;
+		goto err_ff_action;
+	}
+	ff->ff_caps->groups_limit = cpu_to_le32(VIRTNET_FF_MAX_GROUPS);
+
 	err = virtio_admin_cap_set(vdev,
 				   VIRTIO_NET_FF_RESOURCE_CAP,
 				   ff->ff_caps,
@@ -5897,6 +5907,19 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 	if (err)
 		goto err_ff_action;
 
+	ethtool_group.group_priority = cpu_to_le16(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);
+
+	/* Use priority for the object ID. */
+	err = virtio_admin_obj_create(vdev,
+				      VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
+				      VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
+				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
+				      0,
+				      &ethtool_group,
+				      sizeof(ethtool_group));
+	if (err)
+		goto err_ff_action;
+
 	ff->vdev = vdev;
 	ff->ff_supported = true;
 
@@ -5921,9 +5944,18 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 
 static void virtnet_ff_cleanup(struct virtnet_ff *ff)
 {
+	int err;
+
 	if (!ff->ff_supported)
 		return;
 
+	err = virtio_admin_obj_destroy(ff->vdev,
+				       VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
+				       VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
+				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
+				       0);
+	WARN_ON_ONCE(err);
+
 	kfree(ff->ff_actions);
 	kfree(ff->ff_mask);
 	kfree(ff->ff_caps);
diff --git a/include/uapi/linux/virtio_net_ff.h b/include/uapi/linux/virtio_net_ff.h
index f9e771a0baff..b0780e49df9f 100644
--- a/include/uapi/linux/virtio_net_ff.h
+++ b/include/uapi/linux/virtio_net_ff.h
@@ -12,6 +12,8 @@
 #define VIRTIO_NET_FF_SELECTOR_CAP 0x801
 #define VIRTIO_NET_FF_ACTION_CAP 0x802
 
+#define VIRTIO_NET_RESOURCE_OBJ_FF_GROUP 0x0200
+
 /**
  * struct virtio_net_ff_cap_data - Flow filter resource capability limits
  * @groups_limit: maximum number of flow filter groups supported by the device
@@ -88,4 +90,17 @@ struct virtio_net_ff_actions {
 	__u8 reserved[7];
 	__u8 actions[] __counted_by(count);
 };
+
+/**
+ * struct virtio_net_resource_obj_ff_group - Flow filter group object
+ * @group_priority: priority of the group used to order evaluation
+ *
+ * This structure is the payload for the VIRTIO_NET_RESOURCE_OBJ_FF_GROUP
+ * administrative object. Devices use @group_priority to order flow filter
+ * groups. Multi-byte fields are little-endian.
+ */
+struct virtio_net_resource_obj_ff_group {
+	__le16 group_priority;
+};
+
 #endif
-- 
2.49.0

[PATCH net-next v23 08/14] ethtool: Introduce ethtool_flow_type_mask()

[Shahar Shitrit]

Add a common ethtool_flow_type_mask() helper to mask extension bits
from an ethtool flow type.

Move the helper to include/linux/ethtool.h and convert mlx5e and
mlx5i to use it, removing duplicate implementations.

Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/en_fs_ethtool.c | 17 ++++++-----------
 .../ethernet/mellanox/mlx5/core/ipoib/ethtool.c |  7 +------
 include/linux/ethtool.h                         |  6 ++++++
 3 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
index aecfba7deeb0..7247f6cbdb32 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs_ethtool.c
@@ -53,11 +53,6 @@ struct mlx5e_ethtool_steering {
 
 static int flow_type_to_traffic_type(u32 flow_type);
 
-static u32 flow_type_mask(u32 flow_type)
-{
-	return flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS);
-}
-
 struct mlx5e_ethtool_rule {
 	struct list_head             list;
 	struct ethtool_rx_flow_spec  flow_spec;
@@ -91,7 +86,7 @@ static struct mlx5e_ethtool_table *get_flow_table(struct mlx5e_priv *priv,
 	int table_size;
 	int prio;
 
-	switch (flow_type_mask(fs->flow_type)) {
+	switch (ethtool_flow_type_mask(fs->flow_type)) {
 	case TCP_V4_FLOW:
 	case UDP_V4_FLOW:
 	case TCP_V6_FLOW:
@@ -350,7 +345,7 @@ static int set_flow_attrs(u32 *match_c, u32 *match_v,
 					     outer_headers);
 	void *outer_headers_v = MLX5_ADDR_OF(fte_match_param, match_v,
 					     outer_headers);
-	u32 flow_type = flow_type_mask(fs->flow_type);
+	u32 flow_type = ethtool_flow_type_mask(fs->flow_type);
 
 	switch (flow_type) {
 	case TCP_V4_FLOW:
@@ -435,7 +430,7 @@ static int flow_get_tirn(struct mlx5e_priv *priv,
 		if (!rss)
 			return -ENOENT;
 
-		flow_type = flow_type_mask(fs->flow_type);
+		flow_type = ethtool_flow_type_mask(fs->flow_type);
 		tt = flow_type_to_traffic_type(flow_type);
 		if (tt < 0)
 			return -EINVAL;
@@ -673,7 +668,7 @@ static int validate_flow(struct mlx5e_priv *priv,
 		if (fs->ring_cookie >= priv->channels.params.num_channels)
 			return -EINVAL;
 
-	switch (flow_type_mask(fs->flow_type)) {
+	switch (ethtool_flow_type_mask(fs->flow_type)) {
 	case ETHER_FLOW:
 		num_tuples += validate_ethter(fs);
 		break;
@@ -906,7 +901,7 @@ int mlx5e_ethtool_set_rxfh_fields(struct mlx5e_priv *priv,
 
 	rss_idx = nfc->rss_context;
 
-	flow_type = flow_type_mask(nfc->flow_type);
+	flow_type = ethtool_flow_type_mask(nfc->flow_type);
 	tt = flow_type_to_traffic_type(flow_type);
 	if (tt < 0)
 		return tt;
@@ -951,7 +946,7 @@ int mlx5e_ethtool_get_rxfh_fields(struct mlx5e_priv *priv,
 
 	rss_idx = nfc->rss_context;
 
-	flow_type = flow_type_mask(nfc->flow_type);
+	flow_type = ethtool_flow_type_mask(nfc->flow_type);
 	tt = flow_type_to_traffic_type(flow_type);
 	if (tt < 0)
 		return tt;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
index 01ddc3def9ac..83eaceddc437 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
@@ -233,11 +233,6 @@ static int mlx5i_get_link_ksettings(struct net_device *netdev,
 	return 0;
 }
 
-static u32 mlx5i_flow_type_mask(u32 flow_type)
-{
-	return flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS);
-}
-
 static int mlx5i_set_rxfh_fields(struct net_device *dev,
 				 const struct ethtool_rxfh_fields *cmd,
 				 struct netlink_ext_ack *extack)
@@ -260,7 +255,7 @@ static int mlx5i_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	struct mlx5e_priv *priv = mlx5i_epriv(dev);
 	struct ethtool_rx_flow_spec *fs = &cmd->fs;
 
-	if (mlx5i_flow_type_mask(fs->flow_type) == ETHER_FLOW)
+	if (ethtool_flow_type_mask(fs->flow_type) == ETHER_FLOW)
 		return -EINVAL;
 
 	return mlx5e_ethtool_set_rxnfc(priv, cmd);
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12683b5d125e..adecb9225ee3 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1567,4 +1567,10 @@ struct ethtool_forced_speed_map {
 
 void
 ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size);
+
+static inline u32 ethtool_flow_type_mask(u32 flow_type)
+{
+	return flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS);
+}
+
 #endif /* _LINUX_ETHTOOL_H */
-- 
2.49.0

[PATCH net-next v23 09/14] virtio_net: Implement layer 2 ethtool flow rules

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Filtering a flow requires a classifier to match the packets, and a rule
to filter on the matches.

A classifier consists of one or more selectors. There is one selector
per header type. A selector must only use fields set in the selector
capability. If partial matching is supported, the classifier mask for a
particular field can be a subset of the mask for that field in the
capability.

The rule consists of a priority, an action and a key. The key is a byte
array containing headers corresponding to the selectors in the
classifier.

This patch implements ethtool rules for ethernet headers.

Example:
$ ethtool -U ens9 flow-type ether dst 08:11:22:33:44:54 action 30
Added rule with ID 1

The rule in the example directs received packets with the specified
destination MAC address to rq 30.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c           | 529 +++++++++++++++++++++++++++--
 include/uapi/linux/virtio_net_ff.h |  50 +++
 2 files changed, 551 insertions(+), 28 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 294cfd70662d..6217c5bcfee9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -31,6 +31,7 @@
 #include <net/ipv6.h>
 #include <net/ip.h>
 #include <uapi/linux/virtio_net_ff.h>
+#include <linux/xarray.h>
 
 static int napi_weight = NAPI_POLL_WEIGHT;
 module_param(napi_weight, int, 0444);
@@ -286,6 +287,11 @@ static const struct virtnet_stat_desc virtnet_stats_tx_speed_desc_qstat[] = {
 	VIRTNET_STATS_DESC_TX_QSTAT(speed, ratelimit_packets, hw_drop_ratelimits),
 };
 
+struct virtnet_ethtool_ff {
+	struct xarray rules;
+	int    num_rules;
+};
+
 #define VIRTNET_FF_ETHTOOL_GROUP_PRIORITY 0
 #define VIRTNET_FF_MAX_GROUPS 1
 
@@ -295,6 +301,8 @@ struct virtnet_ff {
 	struct virtio_net_ff_cap_data *ff_caps;
 	struct virtio_net_ff_cap_mask_data *ff_mask;
 	struct virtio_net_ff_actions *ff_actions;
+	struct xarray classifiers;
+	struct virtnet_ethtool_ff ethtool;
 };
 
 #define VIRTNET_Q_TYPE_RX 0
@@ -5619,34 +5627,6 @@ static u32 virtnet_get_rx_ring_count(struct net_device *dev)
 	return vi->curr_queue_pairs;
 }
 
-static const struct ethtool_ops virtnet_ethtool_ops = {
-	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES |
-		ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
-	.get_drvinfo = virtnet_get_drvinfo,
-	.get_link = ethtool_op_get_link,
-	.get_ringparam = virtnet_get_ringparam,
-	.set_ringparam = virtnet_set_ringparam,
-	.get_strings = virtnet_get_strings,
-	.get_sset_count = virtnet_get_sset_count,
-	.get_ethtool_stats = virtnet_get_ethtool_stats,
-	.set_channels = virtnet_set_channels,
-	.get_channels = virtnet_get_channels,
-	.get_ts_info = ethtool_op_get_ts_info,
-	.get_link_ksettings = virtnet_get_link_ksettings,
-	.set_link_ksettings = virtnet_set_link_ksettings,
-	.set_coalesce = virtnet_set_coalesce,
-	.get_coalesce = virtnet_get_coalesce,
-	.set_per_queue_coalesce = virtnet_set_per_queue_coalesce,
-	.get_per_queue_coalesce = virtnet_get_per_queue_coalesce,
-	.get_rxfh_key_size = virtnet_get_rxfh_key_size,
-	.get_rxfh_indir_size = virtnet_get_rxfh_indir_size,
-	.get_rxfh = virtnet_get_rxfh,
-	.set_rxfh = virtnet_set_rxfh,
-	.get_rxfh_fields = virtnet_get_hashflow,
-	.set_rxfh_fields = virtnet_set_hashflow,
-	.get_rx_ring_count = virtnet_get_rx_ring_count,
-};
-
 static void virtnet_get_queue_stats_rx(struct net_device *dev, int i,
 				       struct netdev_queue_stats_rx *stats)
 {
@@ -5742,6 +5722,445 @@ static const struct netdev_stat_ops virtnet_stat_ops = {
 	.get_base_stats		= virtnet_get_base_stats,
 };
 
+struct virtnet_ethtool_rule {
+	struct ethtool_rx_flow_spec flow_spec;
+	u32 classifier_id;
+};
+
+/* The classifier struct must be the last field in this struct */
+struct virtnet_classifier {
+	size_t size;
+	u32 id;
+	struct virtio_net_resource_obj_ff_classifier classifier;
+};
+
+static_assert(sizeof(struct virtnet_classifier) ==
+	      ALIGN(offsetofend(struct virtnet_classifier, classifier),
+		    __alignof__(struct virtnet_classifier)),
+	      "virtnet_classifier: classifier must be the last member");
+
+static bool check_mask_vs_cap(const void *m, const void *c,
+			      u16 len, bool partial)
+{
+	const u8 *mask = m;
+	const u8 *cap = c;
+	int i;
+
+	for (i = 0; i < len; i++) {
+		if (partial && ((mask[i] & cap[i]) != mask[i]))
+			return false;
+		if (!partial && mask[i] != cap[i])
+			return false;
+	}
+
+	return true;
+}
+
+static
+struct virtio_net_ff_selector *get_selector_cap(const struct virtnet_ff *ff,
+						u8 selector_type)
+{
+	struct virtio_net_ff_selector *sel;
+	void *buf;
+	int i;
+
+	buf = &ff->ff_mask->selectors;
+	sel = buf;
+
+	for (i = 0; i < ff->ff_mask->count; i++) {
+		if (sel->type == selector_type)
+			return sel;
+
+		buf += sizeof(struct virtio_net_ff_selector) + sel->length;
+		sel = buf;
+	}
+
+	return NULL;
+}
+
+static bool validate_eth_mask(const struct virtnet_ff *ff,
+			      const struct virtio_net_ff_selector *sel,
+			      const struct virtio_net_ff_selector *sel_cap)
+{
+	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
+	struct ethhdr *cap, *mask;
+	struct ethhdr zeros = {};
+
+	cap = (struct ethhdr *)&sel_cap->mask;
+	mask = (struct ethhdr *)&sel->mask;
+
+	if (memcmp(&zeros.h_dest, mask->h_dest, sizeof(zeros.h_dest)) &&
+	    !check_mask_vs_cap(mask->h_dest, cap->h_dest,
+			       sizeof(mask->h_dest), partial_mask))
+		return false;
+
+	if (memcmp(&zeros.h_source, mask->h_source, sizeof(zeros.h_source)) &&
+	    !check_mask_vs_cap(mask->h_source, cap->h_source,
+			       sizeof(mask->h_source), partial_mask))
+		return false;
+
+	if (mask->h_proto &&
+	    !check_mask_vs_cap(&mask->h_proto, &cap->h_proto,
+			       sizeof(__be16), partial_mask))
+		return false;
+
+	return true;
+}
+
+static bool validate_mask(const struct virtnet_ff *ff,
+			  const struct virtio_net_ff_selector *sel)
+{
+	struct virtio_net_ff_selector *sel_cap = get_selector_cap(ff, sel->type);
+
+	if (!sel_cap)
+		return false;
+
+	switch (sel->type) {
+	case VIRTIO_NET_FF_MASK_TYPE_ETH:
+		return validate_eth_mask(ff, sel, sel_cap);
+	}
+
+	return false;
+}
+
+static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
+{
+	int err;
+
+	err = xa_alloc(&ff->classifiers, &c->id, c,
+		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->classifiers_limit) - 1),
+		       GFP_KERNEL);
+	if (err)
+		return err;
+
+	err = virtio_admin_obj_create(ff->vdev,
+				      VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
+				      c->id,
+				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
+				      0,
+				      &c->classifier,
+				      c->size);
+	if (err)
+		goto err_xarray;
+
+	return 0;
+
+err_xarray:
+	xa_erase(&ff->classifiers, c->id);
+
+	return err;
+}
+
+static void destroy_classifier(struct virtnet_ff *ff,
+			       u32 classifier_id)
+{
+	struct virtnet_classifier *c;
+	int err;
+
+	c = xa_load(&ff->classifiers, classifier_id);
+	if (c) {
+		err = virtio_admin_obj_destroy(ff->vdev,
+					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
+					       c->id,
+					       VIRTIO_ADMIN_GROUP_TYPE_SELF,
+					       0);
+		WARN_ON_ONCE(err);
+
+		xa_erase(&ff->classifiers, c->id);
+		kfree(c);
+	}
+}
+
+static int destroy_ethtool_rule(struct virtnet_ff *ff,
+				struct virtnet_ethtool_rule *eth_rule)
+{
+	int err;
+
+	ff->ethtool.num_rules--;
+
+	err = virtio_admin_obj_destroy(ff->vdev,
+				       VIRTIO_NET_RESOURCE_OBJ_FF_RULE,
+				       eth_rule->flow_spec.location,
+				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
+				       0);
+
+	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
+	destroy_classifier(ff, eth_rule->classifier_id);
+	kfree(eth_rule);
+
+	return err;
+}
+
+static int insert_rule(struct virtnet_ff *ff,
+		       struct virtnet_ethtool_rule *eth_rule,
+		       u32 classifier_id,
+		       const u8 *key,
+		       u8 key_size)
+{
+	struct ethtool_rx_flow_spec *fs = &eth_rule->flow_spec;
+	struct virtio_net_resource_obj_ff_rule *ff_rule;
+	int err;
+
+	ff_rule = kzalloc(sizeof(*ff_rule) + key_size, GFP_KERNEL);
+	if (!ff_rule)
+		return -ENOMEM;
+
+	/* Intentionally leave the priority as 0. All rules have the same
+	 * priority.
+	 */
+	ff_rule->group_id = cpu_to_le32(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);
+	ff_rule->classifier_id = cpu_to_le32(classifier_id);
+	ff_rule->key_length = key_size;
+	ff_rule->action = fs->ring_cookie == RX_CLS_FLOW_DISC ?
+					     VIRTIO_NET_FF_ACTION_DROP :
+					     VIRTIO_NET_FF_ACTION_RX_VQ;
+	ff_rule->vq_index = fs->ring_cookie != RX_CLS_FLOW_DISC ?
+					       cpu_to_le16(rxq2vq(fs->ring_cookie)) : 0;
+	memcpy(&ff_rule->keys, key, key_size);
+
+	err = virtio_admin_obj_create(ff->vdev,
+				      VIRTIO_NET_RESOURCE_OBJ_FF_RULE,
+				      fs->location,
+				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
+				      0,
+				      ff_rule,
+				      sizeof(*ff_rule) + key_size);
+	if (err)
+		goto err_ff_rule;
+
+	eth_rule->classifier_id = classifier_id;
+	ff->ethtool.num_rules++;
+	kfree(ff_rule);
+	kfree(key);
+
+	return 0;
+
+err_ff_rule:
+	kfree(ff_rule);
+
+	return err;
+}
+
+static bool supported_flow_type(const struct ethtool_rx_flow_spec *fs)
+{
+	switch (fs->flow_type) {
+	case ETHER_FLOW:
+		return true;
+	}
+
+	return false;
+}
+
+static int validate_flow_input(struct virtnet_ff *ff,
+			       const struct ethtool_rx_flow_spec *fs,
+			       u16 curr_queue_pairs)
+{
+	u8 required_action = fs->ring_cookie == RX_CLS_FLOW_DISC ?
+			     VIRTIO_NET_FF_ACTION_DROP :
+			     VIRTIO_NET_FF_ACTION_RX_VQ;
+	int i;
+
+	/* Force users to use RX_CLS_LOC_ANY - don't allow specific locations */
+	if (fs->location != RX_CLS_LOC_ANY)
+		return -EOPNOTSUPP;
+
+	if (fs->ring_cookie != RX_CLS_FLOW_DISC &&
+	    fs->ring_cookie >= curr_queue_pairs)
+		return -EINVAL;
+
+	for (i = 0; i < ff->ff_actions->count; i++)
+		if (ff->ff_actions->actions[i] == required_action)
+			goto action_ok;
+	return -EOPNOTSUPP;
+
+action_ok:
+	if (fs->flow_type != ethtool_flow_type_mask(fs->flow_type))
+		return -EOPNOTSUPP;
+
+	if (!supported_flow_type(fs))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
+				 u8 *key_size, size_t *classifier_size,
+				 int *num_hdrs)
+{
+	*num_hdrs = 1;
+	*key_size = sizeof(struct ethhdr);
+	/*
+	 * The classifier size is the size of the classifier header, a selector
+	 * header for each type of header in the match criteria, and each header
+	 * providing the mask for matching against.
+	 */
+	*classifier_size = *key_size +
+			   sizeof(struct virtio_net_resource_obj_ff_classifier) +
+			   sizeof(struct virtio_net_ff_selector) * (*num_hdrs);
+}
+
+static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
+				   u8 *key,
+				   const struct ethtool_rx_flow_spec *fs)
+{
+	struct ethhdr *eth_m = (struct ethhdr *)&selector->mask;
+	struct ethhdr *eth_k = (struct ethhdr *)key;
+
+	selector->type = VIRTIO_NET_FF_MASK_TYPE_ETH;
+	selector->length = sizeof(struct ethhdr);
+
+	memcpy(eth_m, &fs->m_u.ether_spec, sizeof(*eth_m));
+	memcpy(eth_k, &fs->h_u.ether_spec, sizeof(*eth_k));
+}
+
+static int
+validate_classifier_selectors(struct virtnet_ff *ff,
+			      struct virtio_net_resource_obj_ff_classifier *classifier,
+			      int num_hdrs)
+{
+	struct virtio_net_ff_selector *selector = (void *)classifier->selectors;
+	int i;
+
+	if (num_hdrs > ff->ff_caps->selectors_per_classifier_limit)
+		return -EINVAL;
+
+	for (i = 0; i < num_hdrs; i++) {
+		if (!validate_mask(ff, selector))
+			return -EINVAL;
+
+		selector = (((void *)selector) + sizeof(*selector) +
+					selector->length);
+	}
+
+	return 0;
+}
+
+static int build_and_insert(struct virtnet_ff *ff,
+			    struct virtnet_ethtool_rule *eth_rule)
+{
+	struct virtio_net_resource_obj_ff_classifier *classifier;
+	struct ethtool_rx_flow_spec *fs = &eth_rule->flow_spec;
+	struct virtio_net_ff_selector *selector;
+	struct virtnet_classifier *c;
+	size_t classifier_size;
+	int num_hdrs;
+	u8 key_size;
+	u8 *key;
+	int err;
+
+	calculate_flow_sizes(fs, &key_size, &classifier_size, &num_hdrs);
+
+	key = kzalloc(key_size, GFP_KERNEL);
+	if (!key)
+		return -ENOMEM;
+
+	/*
+	 * virtio_net_ff_obj_ff_classifier is already included in the
+	 * classifier_size.
+	 */
+	c = kzalloc(classifier_size +
+		    sizeof(struct virtnet_classifier) -
+		    sizeof(struct virtio_net_resource_obj_ff_classifier),
+		    GFP_KERNEL);
+	if (!c) {
+		kfree(key);
+		return -ENOMEM;
+	}
+
+	c->size = classifier_size;
+	classifier = &c->classifier;
+	classifier->count = num_hdrs;
+	selector = (void *)&classifier->selectors[0];
+
+	setup_eth_hdr_key_mask(selector, key, fs);
+
+	err = validate_classifier_selectors(ff, classifier, num_hdrs);
+	if (err)
+		goto err_classifier;
+
+	err = setup_classifier(ff, c);
+	if (err)
+		goto err_classifier;
+
+	err = insert_rule(ff, eth_rule, c->id, key, key_size);
+	if (err) {
+		/* destroy_classifier will free the classifier */
+		destroy_classifier(ff, c->id);
+		goto err_key;
+	}
+
+	return 0;
+
+err_classifier:
+	kfree(c);
+err_key:
+	kfree(key);
+
+	return err;
+}
+
+static int virtnet_ethtool_flow_insert(struct virtnet_ff *ff,
+				       struct ethtool_rx_flow_spec *fs,
+				       u16 curr_queue_pairs)
+{
+	struct virtnet_ethtool_rule *eth_rule;
+	int err;
+
+	if (!ff->ff_supported)
+		return -EOPNOTSUPP;
+
+	err = validate_flow_input(ff, fs, curr_queue_pairs);
+	if (err)
+		return err;
+
+	eth_rule = kzalloc(sizeof(*eth_rule), GFP_KERNEL);
+	if (!eth_rule)
+		return -ENOMEM;
+
+	err = xa_alloc(&ff->ethtool.rules, &fs->location, eth_rule,
+		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->rules_limit) - 1),
+		       GFP_KERNEL);
+	if (err)
+		goto err_rule;
+
+	eth_rule->flow_spec = *fs;
+
+	err = build_and_insert(ff, eth_rule);
+	if (err)
+		goto err_xa;
+
+	return err;
+
+err_xa:
+	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
+
+err_rule:
+	fs->location = RX_CLS_LOC_ANY;
+	kfree(eth_rule);
+
+	return err;
+}
+
+static int virtnet_ethtool_flow_remove(struct virtnet_ff *ff, int location)
+{
+	struct virtnet_ethtool_rule *eth_rule;
+	int err = 0;
+
+	if (!ff->ff_supported)
+		return -EOPNOTSUPP;
+
+	eth_rule = xa_load(&ff->ethtool.rules, location);
+	if (!eth_rule) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	err = destroy_ethtool_rule(ff, eth_rule);
+
+out:
+	return err;
+}
+
 static size_t get_mask_size(u16 type)
 {
 	switch (type) {
@@ -5760,6 +6179,50 @@ static size_t get_mask_size(u16 type)
 	return 0;
 }
 
+static int virtnet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	switch (info->cmd) {
+	case ETHTOOL_SRXCLSRLINS:
+		return virtnet_ethtool_flow_insert(&vi->ff, &info->fs,
+						   vi->curr_queue_pairs);
+	case ETHTOOL_SRXCLSRLDEL:
+		return virtnet_ethtool_flow_remove(&vi->ff, info->fs.location);
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct ethtool_ops virtnet_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES |
+		ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
+	.get_drvinfo = virtnet_get_drvinfo,
+	.get_link = ethtool_op_get_link,
+	.get_ringparam = virtnet_get_ringparam,
+	.set_ringparam = virtnet_set_ringparam,
+	.get_strings = virtnet_get_strings,
+	.get_sset_count = virtnet_get_sset_count,
+	.get_ethtool_stats = virtnet_get_ethtool_stats,
+	.set_channels = virtnet_set_channels,
+	.get_channels = virtnet_get_channels,
+	.get_ts_info = ethtool_op_get_ts_info,
+	.get_link_ksettings = virtnet_get_link_ksettings,
+	.set_link_ksettings = virtnet_set_link_ksettings,
+	.set_coalesce = virtnet_set_coalesce,
+	.get_coalesce = virtnet_get_coalesce,
+	.set_per_queue_coalesce = virtnet_set_per_queue_coalesce,
+	.get_per_queue_coalesce = virtnet_get_per_queue_coalesce,
+	.get_rxfh_key_size = virtnet_get_rxfh_key_size,
+	.get_rxfh_indir_size = virtnet_get_rxfh_indir_size,
+	.get_rxfh = virtnet_get_rxfh,
+	.set_rxfh = virtnet_set_rxfh,
+	.get_rxfh_fields = virtnet_get_hashflow,
+	.set_rxfh_fields = virtnet_set_hashflow,
+	.get_rx_ring_count = virtnet_get_rx_ring_count,
+	.set_rxnfc = virtnet_set_rxnfc,
+};
+
 static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 {
 	size_t ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data) +
@@ -5920,6 +6383,8 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 	if (err)
 		goto err_ff_action;
 
+	xa_init_flags(&ff->classifiers, XA_FLAGS_ALLOC);
+	xa_init_flags(&ff->ethtool.rules, XA_FLAGS_ALLOC);
 	ff->vdev = vdev;
 	ff->ff_supported = true;
 
@@ -5944,11 +6409,19 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
 
 static void virtnet_ff_cleanup(struct virtnet_ff *ff)
 {
+	struct virtnet_ethtool_rule *eth_rule;
+	unsigned long i;
 	int err;
 
 	if (!ff->ff_supported)
 		return;
 
+	xa_for_each(&ff->ethtool.rules, i, eth_rule)
+		WARN_ON_ONCE(destroy_ethtool_rule(ff, eth_rule));
+
+	xa_destroy(&ff->ethtool.rules);
+	xa_destroy(&ff->classifiers);
+
 	err = virtio_admin_obj_destroy(ff->vdev,
 				       VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
 				       VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
diff --git a/include/uapi/linux/virtio_net_ff.h b/include/uapi/linux/virtio_net_ff.h
index b0780e49df9f..9152021c99ba 100644
--- a/include/uapi/linux/virtio_net_ff.h
+++ b/include/uapi/linux/virtio_net_ff.h
@@ -13,6 +13,8 @@
 #define VIRTIO_NET_FF_ACTION_CAP 0x802
 
 #define VIRTIO_NET_RESOURCE_OBJ_FF_GROUP 0x0200
+#define VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER 0x0201
+#define VIRTIO_NET_RESOURCE_OBJ_FF_RULE 0x0202
 
 /**
  * struct virtio_net_ff_cap_data - Flow filter resource capability limits
@@ -103,4 +105,52 @@ struct virtio_net_resource_obj_ff_group {
 	__le16 group_priority;
 };
 
+/**
+ * struct virtio_net_resource_obj_ff_classifier - Flow filter classifier object
+ * @count: number of selector entries in @selectors
+ * @reserved: must be set to 0 by the driver and ignored by the device
+ * @selectors: array of selector descriptors that define match masks
+ *
+ * Payload for the VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER administrative object.
+ * Each selector describes a header mask used to match packets
+ * (see struct virtio_net_ff_selector). Selectors appear in the order they are
+ * to be applied.
+ */
+struct virtio_net_resource_obj_ff_classifier {
+	__u8 count;
+	__u8 reserved[7];
+	__u8 selectors[];
+};
+
+/**
+ * struct virtio_net_resource_obj_ff_rule - Flow filter rule object
+ * @group_id: identifier of the target flow filter group
+ * @classifier_id: identifier of the classifier referenced by this rule
+ * @rule_priority: relative priority of this rule within the group
+ * @key_length: number of bytes in @keys
+ * @action: action to perform, one of VIRTIO_NET_FF_ACTION_*
+ * @reserved: must be set to 0 by the driver and ignored by the device
+ * @vq_index: RX virtqueue index for VIRTIO_NET_FF_ACTION_RX_VQ, 0 otherwise
+ * @reserved1: must be set to 0 by the driver and ignored by the device
+ * @keys: concatenated key bytes matching the classifier's selectors order
+ *
+ * Payload for the VIRTIO_NET_RESOURCE_OBJ_FF_RULE administrative object.
+ * @group_id and @classifier_id refer to previously created objects of types
+ * VIRTIO_NET_RESOURCE_OBJ_FF_GROUP and VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER
+ * respectively. The key bytes are compared against packet headers using the
+ * masks provided by the classifier's selectors. Multi-byte fields are
+ * little-endian.
+ */
+struct virtio_net_resource_obj_ff_rule {
+	__le32 group_id;
+	__le32 classifier_id;
+	__u8 rule_priority;
+	__u8 key_length; /* length of key in bytes */
+	__u8 action;
+	__u8 reserved;
+	__le16 vq_index;
+	__u8 reserved1[2];
+	__u8 keys[];
+};
+
 #endif
-- 
2.49.0

[PATCH net-next v23 10/14] virtio_net: Use existing classifier if possible

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Classifiers can be used by more than one rule. If there is an existing
classifier, use it instead of creating a new one. If duplicate
classifiers are created it would artificially limit the number of rules
to the classifier limit, which is likely less than the rules limit.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c | 50 +++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6217c5bcfee9..6c07eab047af 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -32,6 +32,7 @@
 #include <net/ip.h>
 #include <uapi/linux/virtio_net_ff.h>
 #include <linux/xarray.h>
+#include <linux/refcount.h>
 
 static int napi_weight = NAPI_POLL_WEIGHT;
 module_param(napi_weight, int, 0444);
@@ -5730,12 +5731,13 @@ struct virtnet_ethtool_rule {
 /* The classifier struct must be the last field in this struct */
 struct virtnet_classifier {
 	size_t size;
+	refcount_t refcount;
 	u32 id;
-	struct virtio_net_resource_obj_ff_classifier classifier;
+	struct virtio_net_resource_obj_ff_classifier obj;
 };
 
 static_assert(sizeof(struct virtnet_classifier) ==
-	      ALIGN(offsetofend(struct virtnet_classifier, classifier),
+	      ALIGN(offsetofend(struct virtnet_classifier, obj),
 		    __alignof__(struct virtnet_classifier)),
 	      "virtnet_classifier: classifier must be the last member");
 
@@ -5823,11 +5825,24 @@ static bool validate_mask(const struct virtnet_ff *ff,
 	return false;
 }
 
-static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
+static int setup_classifier(struct virtnet_ff *ff,
+			    struct virtnet_classifier **c)
 {
+	struct virtnet_classifier *tmp;
+	unsigned long i;
 	int err;
 
-	err = xa_alloc(&ff->classifiers, &c->id, c,
+	xa_for_each(&ff->classifiers, i, tmp) {
+		if ((*c)->size == tmp->size &&
+		    !memcmp(&tmp->obj, &(*c)->obj, tmp->size)) {
+			refcount_inc(&tmp->refcount);
+			kfree(*c);
+			*c = tmp;
+			goto out;
+		}
+	}
+
+	err = xa_alloc(&ff->classifiers, &(*c)->id, *c,
 		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->classifiers_limit) - 1),
 		       GFP_KERNEL);
 	if (err)
@@ -5835,30 +5850,31 @@ static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
 
 	err = virtio_admin_obj_create(ff->vdev,
 				      VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
-				      c->id,
+				      (*c)->id,
 				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
 				      0,
-				      &c->classifier,
-				      c->size);
+				      &(*c)->obj,
+				      (*c)->size);
 	if (err)
 		goto err_xarray;
 
+	refcount_set(&(*c)->refcount, 1);
+out:
 	return 0;
 
 err_xarray:
-	xa_erase(&ff->classifiers, c->id);
+	xa_erase(&ff->classifiers, (*c)->id);
 
 	return err;
 }
 
-static void destroy_classifier(struct virtnet_ff *ff,
-			       u32 classifier_id)
+static void try_destroy_classifier(struct virtnet_ff *ff, u32 classifier_id)
 {
 	struct virtnet_classifier *c;
 	int err;
 
 	c = xa_load(&ff->classifiers, classifier_id);
-	if (c) {
+	if (c && refcount_dec_and_test(&c->refcount)) {
 		err = virtio_admin_obj_destroy(ff->vdev,
 					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
 					       c->id,
@@ -5885,7 +5901,7 @@ static int destroy_ethtool_rule(struct virtnet_ff *ff,
 				       0);
 
 	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
-	destroy_classifier(ff, eth_rule->classifier_id);
+	try_destroy_classifier(ff, eth_rule->classifier_id);
 	kfree(eth_rule);
 
 	return err;
@@ -6068,7 +6084,7 @@ static int build_and_insert(struct virtnet_ff *ff,
 	}
 
 	c->size = classifier_size;
-	classifier = &c->classifier;
+	classifier = &c->obj;
 	classifier->count = num_hdrs;
 	selector = (void *)&classifier->selectors[0];
 
@@ -6078,14 +6094,16 @@ static int build_and_insert(struct virtnet_ff *ff,
 	if (err)
 		goto err_classifier;
 
-	err = setup_classifier(ff, c);
+	err = setup_classifier(ff, &c);
 	if (err)
 		goto err_classifier;
 
 	err = insert_rule(ff, eth_rule, c->id, key, key_size);
 	if (err) {
-		/* destroy_classifier will free the classifier */
-		destroy_classifier(ff, c->id);
+		/* try_destroy_classifier will decrement the refcount on the
+		 * classifier and free it if needed.
+		 */
+		try_destroy_classifier(ff, c->id);
 		goto err_key;
 	}
 
-- 
2.49.0

[PATCH net-next v23 11/14] virtio_net: Implement IPv4 ethtool flow rules

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Add support for IP_USER type rules from ethtool.

Example:
$ ethtool -U ens9 flow-type ip4 src-ip 192.168.51.101 action -1
Added rule with ID 1

The example rule will drop packets with the source IP specified.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c | 130 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 124 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6c07eab047af..c33bbdda7559 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -33,6 +33,7 @@
 #include <uapi/linux/virtio_net_ff.h>
 #include <linux/xarray.h>
 #include <linux/refcount.h>
+#include <linux/unaligned.h>
 
 static int napi_weight = NAPI_POLL_WEIGHT;
 module_param(napi_weight, int, 0444);
@@ -5809,6 +5810,39 @@ static bool validate_eth_mask(const struct virtnet_ff *ff,
 	return true;
 }
 
+static bool validate_ip4_mask(const struct virtnet_ff *ff,
+			      const struct virtio_net_ff_selector *sel,
+			      const struct virtio_net_ff_selector *sel_cap)
+{
+	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
+	struct iphdr *cap, *mask;
+
+	cap = (struct iphdr *)&sel_cap->mask;
+	mask = (struct iphdr *)&sel->mask;
+
+	if (get_unaligned(&mask->saddr) &&
+	    !check_mask_vs_cap(&mask->saddr, &cap->saddr,
+			       sizeof(__be32), partial_mask))
+		return false;
+
+	if (get_unaligned(&mask->daddr) &&
+	    !check_mask_vs_cap(&mask->daddr, &cap->daddr,
+			       sizeof(__be32), partial_mask))
+		return false;
+
+	if (mask->protocol &&
+	    !check_mask_vs_cap(&mask->protocol, &cap->protocol,
+			       sizeof(u8), partial_mask))
+		return false;
+
+	if (mask->tos &&
+	    !check_mask_vs_cap(&mask->tos, &cap->tos,
+			       sizeof(u8), partial_mask))
+		return false;
+
+	return true;
+}
+
 static bool validate_mask(const struct virtnet_ff *ff,
 			  const struct virtio_net_ff_selector *sel)
 {
@@ -5820,11 +5854,41 @@ static bool validate_mask(const struct virtnet_ff *ff,
 	switch (sel->type) {
 	case VIRTIO_NET_FF_MASK_TYPE_ETH:
 		return validate_eth_mask(ff, sel, sel_cap);
+
+	case VIRTIO_NET_FF_MASK_TYPE_IPV4:
+		return validate_ip4_mask(ff, sel, sel_cap);
 	}
 
 	return false;
 }
 
+static void parse_ip4(struct iphdr *mask, struct iphdr *key,
+		      const struct ethtool_rx_flow_spec *fs)
+{
+	const struct ethtool_usrip4_spec *l3_mask = &fs->m_u.usr_ip4_spec;
+	const struct ethtool_usrip4_spec *l3_val  = &fs->h_u.usr_ip4_spec;
+
+	if (l3_mask->ip4src) {
+		put_unaligned(l3_mask->ip4src, &mask->saddr);
+		put_unaligned(l3_val->ip4src, &key->saddr);
+	}
+
+	if (l3_mask->ip4dst) {
+		put_unaligned(l3_mask->ip4dst, &mask->daddr);
+		put_unaligned(l3_val->ip4dst, &key->daddr);
+	}
+
+	if (l3_mask->tos) {
+		mask->tos = l3_mask->tos;
+		key->tos = l3_val->tos;
+	}
+}
+
+static bool has_ipv4(u32 flow_type)
+{
+	return flow_type == IP_USER_FLOW;
+}
+
 static int setup_classifier(struct virtnet_ff *ff,
 			    struct virtnet_classifier **c)
 {
@@ -5961,6 +6025,7 @@ static bool supported_flow_type(const struct ethtool_rx_flow_spec *fs)
 {
 	switch (fs->flow_type) {
 	case ETHER_FLOW:
+	case IP_USER_FLOW:
 		return true;
 	}
 
@@ -6003,8 +6068,18 @@ static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
 				 u8 *key_size, size_t *classifier_size,
 				 int *num_hdrs)
 {
+	size_t size = sizeof(struct ethhdr);
+
 	*num_hdrs = 1;
-	*key_size = sizeof(struct ethhdr);
+
+	if (fs->flow_type != ETHER_FLOW) {
+		++(*num_hdrs);
+		if (has_ipv4(fs->flow_type))
+			size += sizeof(struct iphdr);
+	}
+
+	BUG_ON(size > 0xff);
+	*key_size = size;
 	/*
 	 * The classifier size is the size of the classifier header, a selector
 	 * header for each type of header in the match criteria, and each header
@@ -6016,8 +6091,9 @@ static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
 }
 
 static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
-				   u8 *key,
-				   const struct ethtool_rx_flow_spec *fs)
+				  u8 *key,
+				  const struct ethtool_rx_flow_spec *fs,
+				  int num_hdrs)
 {
 	struct ethhdr *eth_m = (struct ethhdr *)&selector->mask;
 	struct ethhdr *eth_k = (struct ethhdr *)key;
@@ -6025,8 +6101,35 @@ static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
 	selector->type = VIRTIO_NET_FF_MASK_TYPE_ETH;
 	selector->length = sizeof(struct ethhdr);
 
-	memcpy(eth_m, &fs->m_u.ether_spec, sizeof(*eth_m));
-	memcpy(eth_k, &fs->h_u.ether_spec, sizeof(*eth_k));
+	if (num_hdrs > 1) {
+		eth_m->h_proto = cpu_to_be16(0xffff);
+		eth_k->h_proto = cpu_to_be16(ETH_P_IP);
+	} else {
+		memcpy(eth_m, &fs->m_u.ether_spec, sizeof(*eth_m));
+		memcpy(eth_k, &fs->h_u.ether_spec, sizeof(*eth_k));
+	}
+}
+
+static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
+			     u8 *key,
+			     const struct ethtool_rx_flow_spec *fs)
+{
+	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
+	struct iphdr *v4_k = (struct iphdr *)key;
+
+	selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
+	selector->length = sizeof(struct iphdr);
+
+	if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
+	    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
+	    fs->m_u.usr_ip4_spec.l4_4_bytes ||
+	    fs->m_u.usr_ip4_spec.ip_ver ||
+	    fs->m_u.usr_ip4_spec.proto)
+		return -EINVAL;
+
+	parse_ip4(v4_m, v4_k, fs);
+
+	return 0;
 }
 
 static int
@@ -6051,6 +6154,13 @@ validate_classifier_selectors(struct virtnet_ff *ff,
 	return 0;
 }
 
+static
+struct virtio_net_ff_selector *next_selector(struct virtio_net_ff_selector *sel)
+{
+	return (void *)sel + sizeof(struct virtio_net_ff_selector) +
+		sel->length;
+}
+
 static int build_and_insert(struct virtnet_ff *ff,
 			    struct virtnet_ethtool_rule *eth_rule)
 {
@@ -6088,7 +6198,15 @@ static int build_and_insert(struct virtnet_ff *ff,
 	classifier->count = num_hdrs;
 	selector = (void *)&classifier->selectors[0];
 
-	setup_eth_hdr_key_mask(selector, key, fs);
+	setup_eth_hdr_key_mask(selector, key, fs, num_hdrs);
+
+	if (has_ipv4(fs->flow_type)) {
+		selector = next_selector(selector);
+
+		err = setup_ip_key_mask(selector, key + sizeof(struct ethhdr), fs);
+		if (err)
+			goto err_classifier;
+	}
 
 	err = validate_classifier_selectors(ff, classifier, num_hdrs);
 	if (err)
-- 
2.49.0

[PATCH net-next v23 12/14] virtio_net: Add support for IPv6 ethtool steering

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Implement support for IPV6_USER_FLOW type rules.

Example:
$ ethtool -U ens9 flow-type ip6 src-ip fe80::2 dst-ip fe80::4 action 3
Added rule with ID 0

The example rule will forward packets with the specified source and
destination IP addresses to RX ring 3.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c | 105 +++++++++++++++++++++++++++++++++++----
 1 file changed, 94 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c33bbdda7559..bfe8f0bc2581 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5843,6 +5843,40 @@ static bool validate_ip4_mask(const struct virtnet_ff *ff,
 	return true;
 }
 
+static bool validate_ip6_mask(const struct virtnet_ff *ff,
+			      const struct virtio_net_ff_selector *sel,
+			      const struct virtio_net_ff_selector *sel_cap)
+{
+	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
+	struct in6_addr tmp;
+	struct ipv6hdr *cap, *mask;
+
+	cap = (struct ipv6hdr *)&sel_cap->mask;
+	mask = (struct ipv6hdr *)&sel->mask;
+
+	/* mask->saddr/daddr may be unaligned; copy to aligned tmp for
+	 * ipv6_addr_any().
+	 */
+	memcpy(&tmp, &mask->saddr, sizeof(tmp));
+	if (!ipv6_addr_any(&tmp) &&
+	    !check_mask_vs_cap(&mask->saddr, &cap->saddr,
+			       sizeof(cap->saddr), partial_mask))
+		return false;
+
+	memcpy(&tmp, &mask->daddr, sizeof(tmp));
+	if (!ipv6_addr_any(&tmp) &&
+	    !check_mask_vs_cap(&mask->daddr, &cap->daddr,
+			       sizeof(cap->daddr), partial_mask))
+		return false;
+
+	if (mask->nexthdr &&
+	    !check_mask_vs_cap(&mask->nexthdr, &cap->nexthdr,
+			       sizeof(cap->nexthdr), partial_mask))
+		return false;
+
+	return true;
+}
+
 static bool validate_mask(const struct virtnet_ff *ff,
 			  const struct virtio_net_ff_selector *sel)
 {
@@ -5857,6 +5891,9 @@ static bool validate_mask(const struct virtnet_ff *ff,
 
 	case VIRTIO_NET_FF_MASK_TYPE_IPV4:
 		return validate_ip4_mask(ff, sel, sel_cap);
+
+	case VIRTIO_NET_FF_MASK_TYPE_IPV6:
+		return validate_ip6_mask(ff, sel, sel_cap);
 	}
 
 	return false;
@@ -5884,11 +5921,33 @@ static void parse_ip4(struct iphdr *mask, struct iphdr *key,
 	}
 }
 
+static void parse_ip6(struct ipv6hdr *mask, struct ipv6hdr *key,
+		      const struct ethtool_rx_flow_spec *fs)
+{
+	const struct ethtool_usrip6_spec *l3_mask = &fs->m_u.usr_ip6_spec;
+	const struct ethtool_usrip6_spec *l3_val  = &fs->h_u.usr_ip6_spec;
+
+	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6src)) {
+		memcpy(&mask->saddr, l3_mask->ip6src, sizeof(mask->saddr));
+		memcpy(&key->saddr, l3_val->ip6src, sizeof(key->saddr));
+	}
+
+	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6dst)) {
+		memcpy(&mask->daddr, l3_mask->ip6dst, sizeof(mask->daddr));
+		memcpy(&key->daddr, l3_val->ip6dst, sizeof(key->daddr));
+	}
+}
+
 static bool has_ipv4(u32 flow_type)
 {
 	return flow_type == IP_USER_FLOW;
 }
 
+static bool has_ipv6(u32 flow_type)
+{
+	return flow_type == IPV6_USER_FLOW;
+}
+
 static int setup_classifier(struct virtnet_ff *ff,
 			    struct virtnet_classifier **c)
 {
@@ -6026,6 +6085,7 @@ static bool supported_flow_type(const struct ethtool_rx_flow_spec *fs)
 	switch (fs->flow_type) {
 	case ETHER_FLOW:
 	case IP_USER_FLOW:
+	case IPV6_USER_FLOW:
 		return true;
 	}
 
@@ -6076,6 +6136,8 @@ static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
 		++(*num_hdrs);
 		if (has_ipv4(fs->flow_type))
 			size += sizeof(struct iphdr);
+		else if (has_ipv6(fs->flow_type))
+			size += sizeof(struct ipv6hdr);
 	}
 
 	BUG_ON(size > 0xff);
@@ -6103,7 +6165,10 @@ static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
 
 	if (num_hdrs > 1) {
 		eth_m->h_proto = cpu_to_be16(0xffff);
-		eth_k->h_proto = cpu_to_be16(ETH_P_IP);
+		if (has_ipv4(fs->flow_type))
+			eth_k->h_proto = cpu_to_be16(ETH_P_IP);
+		else
+			eth_k->h_proto = cpu_to_be16(ETH_P_IPV6);
 	} else {
 		memcpy(eth_m, &fs->m_u.ether_spec, sizeof(*eth_m));
 		memcpy(eth_k, &fs->h_u.ether_spec, sizeof(*eth_k));
@@ -6114,20 +6179,38 @@ static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
 			     u8 *key,
 			     const struct ethtool_rx_flow_spec *fs)
 {
+	struct ipv6hdr *v6_m = (struct ipv6hdr *)&selector->mask;
 	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
+	struct ipv6hdr *v6_k = (struct ipv6hdr *)key;
 	struct iphdr *v4_k = (struct iphdr *)key;
 
-	selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
-	selector->length = sizeof(struct iphdr);
+	if (has_ipv6(fs->flow_type)) {
+		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV6;
+		selector->length = sizeof(struct ipv6hdr);
+
+		/* exclude tclass, it's not exposed directly in struct ipv6hdr */
+		if (fs->h_u.usr_ip6_spec.l4_4_bytes ||
+		    fs->m_u.usr_ip6_spec.l4_4_bytes ||
+		    fs->h_u.usr_ip6_spec.tclass ||
+		    fs->m_u.usr_ip6_spec.tclass ||
+		    fs->h_u.usr_ip6_spec.l4_proto ||
+		    fs->m_u.usr_ip6_spec.l4_proto)
+			return -EINVAL;
 
-	if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
-	    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
-	    fs->m_u.usr_ip4_spec.l4_4_bytes ||
-	    fs->m_u.usr_ip4_spec.ip_ver ||
-	    fs->m_u.usr_ip4_spec.proto)
-		return -EINVAL;
+		parse_ip6(v6_m, v6_k, fs);
+	} else {
+		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
+		selector->length = sizeof(struct iphdr);
+
+		if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
+		    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
+		    fs->m_u.usr_ip4_spec.l4_4_bytes ||
+		    fs->m_u.usr_ip4_spec.ip_ver ||
+		    fs->m_u.usr_ip4_spec.proto)
+			return -EINVAL;
 
-	parse_ip4(v4_m, v4_k, fs);
+		parse_ip4(v4_m, v4_k, fs);
+	}
 
 	return 0;
 }
@@ -6200,7 +6283,7 @@ static int build_and_insert(struct virtnet_ff *ff,
 
 	setup_eth_hdr_key_mask(selector, key, fs, num_hdrs);
 
-	if (has_ipv4(fs->flow_type)) {
+	if (has_ipv4(fs->flow_type) || has_ipv6(fs->flow_type)) {
 		selector = next_selector(selector);
 
 		err = setup_ip_key_mask(selector, key + sizeof(struct ethhdr), fs);
-- 
2.49.0

[PATCH net-next v23 13/14] virtio_net: Add support for TCP and UDP ethtool rules

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

Implement TCP and UDP V4/V6 ethtool flow types.

Examples:
$ ethtool -U ens9 flow-type udp4 dst-ip 192.168.5.2 dst-port\
4321 action 20
Added rule with ID 4

This example directs IPv4 UDP traffic with the specified address and
port to queue 20.

$ ethtool -U ens9 flow-type tcp6 src-ip 2001:db8::1 src-port 1234 dst-ip\
2001:db8::2 dst-port 4321 action 12
Added rule with ID 5

This example directs IPv6 TCP traffic with the specified address and
port to queue 12.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c | 223 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 209 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bfe8f0bc2581..a1e042f356f5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -31,6 +31,8 @@
 #include <net/ipv6.h>
 #include <net/ip.h>
 #include <uapi/linux/virtio_net_ff.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
 #include <linux/xarray.h>
 #include <linux/refcount.h>
 #include <linux/unaligned.h>
@@ -5877,6 +5879,52 @@ static bool validate_ip6_mask(const struct virtnet_ff *ff,
 	return true;
 }
 
+static bool validate_tcp_mask(const struct virtnet_ff *ff,
+			      const struct virtio_net_ff_selector *sel,
+			      const struct virtio_net_ff_selector *sel_cap)
+{
+	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
+	struct tcphdr *cap, *mask;
+
+	cap = (struct tcphdr *)&sel_cap->mask;
+	mask = (struct tcphdr *)&sel->mask;
+
+	if (get_unaligned(&mask->source) &&
+	    !check_mask_vs_cap(&mask->source, &cap->source,
+			       sizeof(cap->source), partial_mask))
+		return false;
+
+	if (get_unaligned(&mask->dest) &&
+	    !check_mask_vs_cap(&mask->dest, &cap->dest,
+			       sizeof(cap->dest), partial_mask))
+		return false;
+
+	return true;
+}
+
+static bool validate_udp_mask(const struct virtnet_ff *ff,
+			      const struct virtio_net_ff_selector *sel,
+			      const struct virtio_net_ff_selector *sel_cap)
+{
+	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
+	struct udphdr *cap, *mask;
+
+	cap = (struct udphdr *)&sel_cap->mask;
+	mask = (struct udphdr *)&sel->mask;
+
+	if (get_unaligned(&mask->source) &&
+	    !check_mask_vs_cap(&mask->source, &cap->source,
+			       sizeof(cap->source), partial_mask))
+		return false;
+
+	if (get_unaligned(&mask->dest) &&
+	    !check_mask_vs_cap(&mask->dest, &cap->dest,
+			       sizeof(cap->dest), partial_mask))
+		return false;
+
+	return true;
+}
+
 static bool validate_mask(const struct virtnet_ff *ff,
 			  const struct virtio_net_ff_selector *sel)
 {
@@ -5894,11 +5942,47 @@ static bool validate_mask(const struct virtnet_ff *ff,
 
 	case VIRTIO_NET_FF_MASK_TYPE_IPV6:
 		return validate_ip6_mask(ff, sel, sel_cap);
+
+	case VIRTIO_NET_FF_MASK_TYPE_TCP:
+		return validate_tcp_mask(ff, sel, sel_cap);
+
+	case VIRTIO_NET_FF_MASK_TYPE_UDP:
+		return validate_udp_mask(ff, sel, sel_cap);
 	}
 
 	return false;
 }
 
+static void set_tcp(struct tcphdr *mask, struct tcphdr *key,
+		    __be16 psrc_m, __be16 psrc_k,
+		    __be16 pdst_m, __be16 pdst_k)
+{
+	/* mask/key may be unaligned; use memcpy */
+	if (psrc_m) {
+		memcpy(&mask->source, &psrc_m, sizeof(mask->source));
+		memcpy(&key->source, &psrc_k, sizeof(key->source));
+	}
+	if (pdst_m) {
+		memcpy(&mask->dest, &pdst_m, sizeof(mask->dest));
+		memcpy(&key->dest, &pdst_k, sizeof(key->dest));
+	}
+}
+
+static void set_udp(struct udphdr *mask, struct udphdr *key,
+		    __be16 psrc_m, __be16 psrc_k,
+		    __be16 pdst_m, __be16 pdst_k)
+{
+	/* mask/key may be unaligned; use memcpy */
+	if (psrc_m) {
+		memcpy(&mask->source, &psrc_m, sizeof(mask->source));
+		memcpy(&key->source, &psrc_k, sizeof(key->source));
+	}
+	if (pdst_m) {
+		memcpy(&mask->dest, &pdst_m, sizeof(mask->dest));
+		memcpy(&key->dest, &pdst_k, sizeof(key->dest));
+	}
+}
+
 static void parse_ip4(struct iphdr *mask, struct iphdr *key,
 		      const struct ethtool_rx_flow_spec *fs)
 {
@@ -5940,12 +6024,26 @@ static void parse_ip6(struct ipv6hdr *mask, struct ipv6hdr *key,
 
 static bool has_ipv4(u32 flow_type)
 {
-	return flow_type == IP_USER_FLOW;
+	return flow_type == TCP_V4_FLOW ||
+	       flow_type == UDP_V4_FLOW ||
+	       flow_type == IP_USER_FLOW;
 }
 
 static bool has_ipv6(u32 flow_type)
 {
-	return flow_type == IPV6_USER_FLOW;
+	return flow_type == TCP_V6_FLOW ||
+	       flow_type == UDP_V6_FLOW ||
+	       flow_type == IPV6_USER_FLOW;
+}
+
+static bool has_tcp(u32 flow_type)
+{
+	return flow_type == TCP_V4_FLOW || flow_type == TCP_V6_FLOW;
+}
+
+static bool has_udp(u32 flow_type)
+{
+	return flow_type == UDP_V4_FLOW || flow_type == UDP_V6_FLOW;
 }
 
 static int setup_classifier(struct virtnet_ff *ff,
@@ -6086,6 +6184,10 @@ static bool supported_flow_type(const struct ethtool_rx_flow_spec *fs)
 	case ETHER_FLOW:
 	case IP_USER_FLOW:
 	case IPV6_USER_FLOW:
+	case TCP_V4_FLOW:
+	case TCP_V6_FLOW:
+	case UDP_V4_FLOW:
+	case UDP_V6_FLOW:
 		return true;
 	}
 
@@ -6138,6 +6240,12 @@ static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
 			size += sizeof(struct iphdr);
 		else if (has_ipv6(fs->flow_type))
 			size += sizeof(struct ipv6hdr);
+
+		if (has_tcp(fs->flow_type) || has_udp(fs->flow_type)) {
+			++(*num_hdrs);
+			size += has_tcp(fs->flow_type) ? sizeof(struct tcphdr) :
+							 sizeof(struct udphdr);
+		}
 	}
 
 	BUG_ON(size > 0xff);
@@ -6177,7 +6285,8 @@ static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
 
 static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
 			     u8 *key,
-			     const struct ethtool_rx_flow_spec *fs)
+			     const struct ethtool_rx_flow_spec *fs,
+			     int num_hdrs)
 {
 	struct ipv6hdr *v6_m = (struct ipv6hdr *)&selector->mask;
 	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
@@ -6189,27 +6298,99 @@ static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
 		selector->length = sizeof(struct ipv6hdr);
 
 		/* exclude tclass, it's not exposed directly in struct ipv6hdr */
-		if (fs->h_u.usr_ip6_spec.l4_4_bytes ||
-		    fs->m_u.usr_ip6_spec.l4_4_bytes ||
-		    fs->h_u.usr_ip6_spec.tclass ||
+		if (fs->h_u.usr_ip6_spec.tclass ||
 		    fs->m_u.usr_ip6_spec.tclass ||
-		    fs->h_u.usr_ip6_spec.l4_proto ||
-		    fs->m_u.usr_ip6_spec.l4_proto)
+		    (num_hdrs == 2 && (fs->h_u.usr_ip6_spec.l4_4_bytes ||
+				      fs->m_u.usr_ip6_spec.l4_4_bytes ||
+				      fs->h_u.usr_ip6_spec.l4_proto ||
+				      fs->m_u.usr_ip6_spec.l4_proto)))
 			return -EINVAL;
 
 		parse_ip6(v6_m, v6_k, fs);
+
+		if (num_hdrs > 2) {
+			v6_m->nexthdr = 0xff;
+			if (has_tcp(fs->flow_type))
+				v6_k->nexthdr = IPPROTO_TCP;
+			else
+				v6_k->nexthdr = IPPROTO_UDP;
+		}
 	} else {
 		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
 		selector->length = sizeof(struct iphdr);
 
-		if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
-		    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
-		    fs->m_u.usr_ip4_spec.l4_4_bytes ||
-		    fs->m_u.usr_ip4_spec.ip_ver ||
-		    fs->m_u.usr_ip4_spec.proto)
+		if (num_hdrs == 2 &&
+		    (fs->h_u.usr_ip4_spec.l4_4_bytes ||
+		     fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
+		     fs->m_u.usr_ip4_spec.l4_4_bytes ||
+		     fs->m_u.usr_ip4_spec.ip_ver ||
+		     fs->m_u.usr_ip4_spec.proto))
 			return -EINVAL;
 
 		parse_ip4(v4_m, v4_k, fs);
+
+		if (num_hdrs > 2) {
+			v4_m->protocol = 0xff;
+			if (has_tcp(fs->flow_type))
+				v4_k->protocol = IPPROTO_TCP;
+			else
+				v4_k->protocol = IPPROTO_UDP;
+		}
+	}
+
+	return 0;
+}
+
+static int setup_transport_key_mask(struct virtio_net_ff_selector *selector,
+				    u8 *key,
+				    struct ethtool_rx_flow_spec *fs)
+{
+	struct tcphdr *tcp_m = (struct tcphdr *)&selector->mask;
+	struct udphdr *udp_m = (struct udphdr *)&selector->mask;
+	const struct ethtool_tcpip6_spec *v6_l4_mask;
+	const struct ethtool_tcpip4_spec *v4_l4_mask;
+	const struct ethtool_tcpip6_spec *v6_l4_key;
+	const struct ethtool_tcpip4_spec *v4_l4_key;
+	struct tcphdr *tcp_k = (struct tcphdr *)key;
+	struct udphdr *udp_k = (struct udphdr *)key;
+
+	if (has_tcp(fs->flow_type)) {
+		selector->type = VIRTIO_NET_FF_MASK_TYPE_TCP;
+		selector->length = sizeof(struct tcphdr);
+
+		if (has_ipv6(fs->flow_type)) {
+			v6_l4_mask = &fs->m_u.tcp_ip6_spec;
+			v6_l4_key = &fs->h_u.tcp_ip6_spec;
+
+			set_tcp(tcp_m, tcp_k, v6_l4_mask->psrc, v6_l4_key->psrc,
+				v6_l4_mask->pdst, v6_l4_key->pdst);
+		} else {
+			v4_l4_mask = &fs->m_u.tcp_ip4_spec;
+			v4_l4_key = &fs->h_u.tcp_ip4_spec;
+
+			set_tcp(tcp_m, tcp_k, v4_l4_mask->psrc, v4_l4_key->psrc,
+				v4_l4_mask->pdst, v4_l4_key->pdst);
+		}
+
+	} else if (has_udp(fs->flow_type)) {
+		selector->type = VIRTIO_NET_FF_MASK_TYPE_UDP;
+		selector->length = sizeof(struct udphdr);
+
+		if (has_ipv6(fs->flow_type)) {
+			v6_l4_mask = &fs->m_u.udp_ip6_spec;
+			v6_l4_key = &fs->h_u.udp_ip6_spec;
+
+			set_udp(udp_m, udp_k, v6_l4_mask->psrc, v6_l4_key->psrc,
+				v6_l4_mask->pdst, v6_l4_key->pdst);
+		} else {
+			v4_l4_mask = &fs->m_u.udp_ip4_spec;
+			v4_l4_key = &fs->h_u.udp_ip4_spec;
+
+			set_udp(udp_m, udp_k, v4_l4_mask->psrc, v4_l4_key->psrc,
+				v4_l4_mask->pdst, v4_l4_key->pdst);
+		}
+	} else {
+		return -EOPNOTSUPP;
 	}
 
 	return 0;
@@ -6252,6 +6433,7 @@ static int build_and_insert(struct virtnet_ff *ff,
 	struct virtio_net_ff_selector *selector;
 	struct virtnet_classifier *c;
 	size_t classifier_size;
+	size_t key_offset;
 	int num_hdrs;
 	u8 key_size;
 	u8 *key;
@@ -6284,11 +6466,24 @@ static int build_and_insert(struct virtnet_ff *ff,
 	setup_eth_hdr_key_mask(selector, key, fs, num_hdrs);
 
 	if (has_ipv4(fs->flow_type) || has_ipv6(fs->flow_type)) {
+		key_offset = selector->length;
 		selector = next_selector(selector);
 
-		err = setup_ip_key_mask(selector, key + sizeof(struct ethhdr), fs);
+		err = setup_ip_key_mask(selector, key + key_offset,
+					fs, num_hdrs);
 		if (err)
 			goto err_classifier;
+
+		if (has_udp(fs->flow_type) || has_tcp(fs->flow_type)) {
+			key_offset += selector->length;
+			selector = next_selector(selector);
+
+			err = setup_transport_key_mask(selector,
+						       key + key_offset,
+						       fs);
+			if (err)
+				goto err_classifier;
+		}
 	}
 
 	err = validate_classifier_selectors(ff, classifier, num_hdrs);
-- 
2.49.0

[PATCH net-next v23 14/14] virtio_net: Add get ethtool flow rules ops

[Shahar Shitrit]

From: Daniel Jurgens <danielj@nvidia.com>

- Get total number of rules via ETHTOOL_GRXCLSRLCNT. It is
  used to allocate an appropriately sized buffer for getting all the
  rules.

- Get specific rule
$ ethtool -u ens9 rule 0
	Filter: 0
		Rule Type: UDP over IPv4
		Src IP addr: 0.0.0.0 mask: 255.255.255.255
		Dest IP addr: 192.168.5.2 mask: 0.0.0.0
		TOS: 0x0 mask: 0xff
		Src port: 0 mask: 0xffff
		Dest port: 4321 mask: 0x0
		Action: Direct to queue 16

- Get all rules:
$ ethtool -u ens9
31 RX rings available
Total 2 rules

Filter: 0
        Rule Type: UDP over IPv4
        Src IP addr: 0.0.0.0 mask: 255.255.255.255
        Dest IP addr: 192.168.5.2 mask: 0.0.0.0
...

Filter: 1
        Flow Type: Raw Ethernet
        Src MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF
        Dest MAC addr: 08:11:22:33:44:54 mask: 00:00:00:00:00:00

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
---
 drivers/net/virtio_net.c | 77 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a1e042f356f5..911a5142605b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6575,6 +6575,60 @@ static int virtnet_ethtool_flow_remove(struct virtnet_ff *ff, int location)
 	return err;
 }
 
+static int virtnet_ethtool_get_flow_count(struct virtnet_ff *ff,
+					  struct ethtool_rxnfc *info)
+{
+	if (!ff->ff_supported)
+		return -EOPNOTSUPP;
+
+	info->rule_cnt = ff->ethtool.num_rules;
+	info->data = min(le32_to_cpu(ff->ff_caps->rules_limit),
+			 le32_to_cpu(ff->ff_caps->rules_per_group_limit)) |
+		     RX_CLS_LOC_SPECIAL;
+
+	return 0;
+}
+
+static int virtnet_ethtool_get_flow(struct virtnet_ff *ff,
+				    struct ethtool_rxnfc *info)
+{
+	struct virtnet_ethtool_rule *eth_rule;
+
+	if (!ff->ff_supported)
+		return -EOPNOTSUPP;
+
+	eth_rule = xa_load(&ff->ethtool.rules, info->fs.location);
+	if (!eth_rule)
+		return -ENOENT;
+
+	info->fs = eth_rule->flow_spec;
+
+	return 0;
+}
+
+static int
+virtnet_ethtool_get_all_flows(struct virtnet_ff *ff,
+			      struct ethtool_rxnfc *info, u32 *rule_locs)
+{
+	struct virtnet_ethtool_rule *eth_rule;
+	unsigned long i = 0;
+	int idx = 0;
+
+	if (!ff->ff_supported)
+		return -EOPNOTSUPP;
+
+	xa_for_each(&ff->ethtool.rules, i, eth_rule) {
+		if (idx == info->rule_cnt)
+			return -EMSGSIZE;
+		rule_locs[idx++] = i;
+	}
+
+	info->data = le32_to_cpu(ff->ff_caps->rules_limit);
+	info->rule_cnt = idx;
+
+	return 0;
+}
+
 static size_t get_mask_size(u16 type)
 {
 	switch (type) {
@@ -6593,6 +6647,28 @@ static size_t get_mask_size(u16 type)
 	return 0;
 }
 
+static int virtnet_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info, u32 *rule_locs)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	int rc;
+
+	switch (info->cmd) {
+	case ETHTOOL_GRXCLSRLCNT:
+		rc = virtnet_ethtool_get_flow_count(&vi->ff, info);
+		break;
+	case ETHTOOL_GRXCLSRULE:
+		rc = virtnet_ethtool_get_flow(&vi->ff, info);
+		break;
+	case ETHTOOL_GRXCLSRLALL:
+		rc = virtnet_ethtool_get_all_flows(&vi->ff, info, rule_locs);
+		break;
+	default:
+		rc = -EOPNOTSUPP;
+	}
+
+	return rc;
+}
+
 static int virtnet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -6634,6 +6710,7 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
 	.get_rxfh_fields = virtnet_get_hashflow,
 	.set_rxfh_fields = virtnet_set_hashflow,
 	.get_rx_ring_count = virtnet_get_rx_ring_count,
+	.get_rxnfc = virtnet_get_rxnfc,
 	.set_rxnfc = virtnet_set_rxnfc,
 };
 
-- 
2.49.0

Re: [PATCH net-next v23 00/14] virtio_net: Add ethtool flow rules support

[Michael S. Tsirkin]

On Mon, Aug 31, 2026 at 07:10:55PM +0300, Shahar Shitrit wrote:
> This series implements ethtool flow rules support for virtio_net using the
> virtio flow filter (FF) specification. The implementation allows users to
> configure packet filtering rules through ethtool commands, directing
> packets to specific receive queues, or dropping them based on various
> header fields.
> 
> The series starts with infrastructure changes to expose virtio PCI admin
> capabilities and object management APIs. It then creates the virtio_net
> directory structure and implements the flow filter functionality with
> support for:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> - Layer 2 (Ethernet) flow rules
> - IPv4 and IPv6 flow rules
> - TCP and UDP flow rules (both IPv4 and IPv6)
> - Rule querying and management operations
> 
> Setting, deleting and viewing flow filters, -1 action is drop, positive
> integers steer to that RQ:
> 
> $ ethtool -u ens9
> 4 RX rings available
> Total 0 rules
> 
> $ ethtool -U ens9 flow-type ether src 1c:34:da:4a:33:dd action 0
> Added rule with ID 0
> $ ethtool -U ens9 flow-type udp4 dst-port 5001 action 3
> Added rule with ID 1
> $ ethtool -U ens9 flow-type tcp6 src-ip fc00::2 dst-port 5001 action 2
> Added rule with ID 2
> $ ethtool -U ens9 flow-type ip4 src-ip 192.168.51.101 action 1
> Added rule with ID 3
> $ ethtool -U ens9 flow-type ip6 dst-ip fc00::1 action -1
> Added rule with ID 4
> $ ethtool -U ens9 flow-type ip6 src-ip fc00::2 action -1
> Added rule with ID 5
> $ ethtool -U ens9 delete 4
> $ ethtool -u ens9
> 4 RX rings available
> Total 5 rules
> 
> Filter: 0
>         Flow Type: Raw Ethernet
>         Src MAC addr: 1C:34:DA:4A:33:DD mask: 00:00:00:00:00:00
>         Dest MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF
>         Ethertype: 0x0 mask: 0xFFFF
>         Action: Direct to queue 0
> 
> Filter: 1
>         Rule Type: UDP over IPv4
>         Src IP addr: 0.0.0.0 mask: 255.255.255.255
>         Dest IP addr: 0.0.0.0 mask: 255.255.255.255
>         TOS: 0x0 mask: 0xff
>         Src port: 0 mask: 0xffff
>         Dest port: 5001 mask: 0x0
>         Action: Direct to queue 3
> 
> Filter: 2
>         Rule Type: TCP over IPv6
>         Src IP addr: fc00::2 mask: ::
>         Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
>         Traffic Class: 0x0 mask: 0xff
>         Src port: 0 mask: 0xffff
>         Dest port: 5001 mask: 0x0
>         Action: Direct to queue 2
> 
> Filter: 3
>         Rule Type: Raw IPv4
>         Src IP addr: 192.168.51.101 mask: 0.0.0.0
>         Dest IP addr: 0.0.0.0 mask: 255.255.255.255
>         TOS: 0x0 mask: 0xff
>         Protocol: 0 mask: 0xff
>         L4 bytes: 0x0 mask: 0xffffffff
>         Action: Direct to queue 1
> 
> Filter: 5
>         Rule Type: Raw IPv6
>         Src IP addr: fc00::2 mask: ::
>         Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
>         Traffic Class: 0x0 mask: 0xff
>         Protocol: 0 mask: 0xff
>         L4 bytes: 0x0 mask: 0xffffffff
>         Action: Drop
> 
> ---
> v2: https://lore.kernel.org/netdev/20250908164046.25051-1-danielj@nvidia.com/
>   - Fix sparse warnings
>   - Fix memory leak on subsequent failure to allocate
>   - Fix some Typos
> 
> v3: https://lore.kernel.org/netdev/20250923141920.283862-1-danielj@nvidia.com/
>   - Added admin_ops to virtio_device kdoc.
> 
> v4:
>   - Fixed double free bug inserting flows
>   - Fixed incorrect protocol field check parsing ip4 headers.
>   - (u8 *) changed to (void *)
>   - Added kdoc comments to UAPI changes.
>   - No longer split up virtio_net.c
>   - Added config op to execute admin commands.
>       - virtio_pci assigns vp_modern_admin_cmd_exec to this callback.
>   - Moved admin command API to new core file virtio_admin_commands.c
> 
> v5:
>   - Fixed compile error
>   - Fixed static analysis warning on () after macro
>   - Added missing fields to kdoc comments
>   - Aligned parameter name between prototype and kdoc
> 
> v6:
>   - Fix sparse warning "array of flexible structures" Jakub K/Simon H
>   - Use new variable and validate ff_mask_size before set_cap. MST
> 
> v7:
>   - Change virtnet_ff_init to return a value. Allow -EOPNOTSUPP. Xuan
>   - Set ff->ff_{caps, mask, actions} NULL in error path. Paolo Abini
>   - Move for (int i removal hung back a patch. Paolo Abini
> 
> v8
>   - Removed unused num_classifiers. Jason Wang
>   - Use real_ff_mask_size when setting the selector caps. Jason Wang
> 
> v9:
>   - Set err to -ENOMEM after alloc failures in virtnet_ff_init. Simon H
> 
> v10:
>   - Return -EOPNOTSUPP in virnet_ff_init before allocing any memory.
>     Jason Wang/Paolo Abeni
> 
> v11:
>   - Return -EINVAL if any resource limit is 0. Simon Horman
>   - Ensure we don't overrun alloced space of ff->ff_mask by moving the
>     real_ff_mask_size > ff_mask_size check into the loop. Simon Horman
> 
> v12: Many comments by MST, thanks Michael. Only the most significant
>      listed here:
>   - Fixed leak of key in build_and_insert.
>   - Fixed setting ethhdr proto for IPv6.
>   - Added 2 byte pad to struct virtio_net_ff_cap_data.
>   - Use and set rule_cnt when querying all flows.
>   - Cleanup and reinit in freeze/restore path.
> 
> v13:
>   - Add private comment for reserved field in kdoc. Jakub
>   - Serveral comments from MST details in patches. Most significant:
> 	- Fixed bug in ip4, check l3_mask vs mask when setting addrs.
> 	- Changed ff_mask cap checking to not break on expanded
> 	  selector types
> 	- Changed virtio_admin_obj_destroy to return void.
> 	- Check tos field for ip4.
> 	- Don't accept tclass field for ip6.
> 	- If ip6 only flow check that l4_proto isn't set.
> 
> v14:
>   - Handle virtio_ff_init errors in freeze/restore. MST
>   - Don't set proto in parse_ip4/6. The casted struct may not have that
>     field, and the proto field was set explicitly anyway. Simon H/AI.
> 
> v15:
>   - In virtnet_restore_up only call virtnet_close in err path if
>     netif_running. AI
> 
> v16:
>   - Return 0 from virtnet_restore_up if virtnet_init_ff return not
>     supported. AI
>   - Rebased over removing series to remove delayed refill.
> 
> v17:
>   - Properly handle unaligned reads/writes. MST
>   - Fix use after free if init fails during virtnet_restor. AI
>   - Fix memory leak when validating the classifer vs caps fails. AI
>   - Added missing includes. MSTA
> 
> v18:
>   - Validate selector cap lengths, instead of just checking they don't
>     exceed a max. AI
>   - Add __count_by attribute to flexible arrays in UAPI definitions.
>     Paolo A.
> 
> v19:
>   - Style fixes. AI
> 
> v20:
>   - Added missing include
> 
> v21:
>   - Use le64_to_cpu() and BIT_ULL() instead of cpu_to_le64() for cap checking.
>   - Don't use __counted_by on flexible array of flexible structs.
>   - Replace UAPI header includes with kernel header includes.
>   - Add missing includes for linux/types.h and linux/byteorder/generic.h.
>   - Clamp the reported action count to the driver-supported maximum.
>   - Clamp the reported selector count to the driver-supported maximum.
>   - Validate sel->type is not 0.
>   - Reduce selectors' count in case selector's type is invalid.
>   - Move virtio_device_ready() before virtnet_ff_init() as the flow filter
>     initialization requires the device to be in ready state to issue admin
>     commands.
>   - Remove forward declarations.
>   - Validate action is supported before inserting rule.
>   - Convert ring_cookie to vq before assigning ff_rule->vq_index.
>   - reword a comment.
>   - Introduce a new patch that moves flow_type_mask() to include/linux/ethtool.h.
>   - Wrap __le32 limit fields in le32_to_cpu() to avoid sparse warnings.
>   - Use put_unaligned() in parse_ip4() to avoid misaligned 32-bit stores on
>     strict-alignment architectures.
> 
> v22:
>   - Reword/fix typo in commit messages.
>   - Remove include <uapi/linux/virtio_pci.h> from virtio_net.c.
>   - Verify also selectors_per_classifier_limit in virtnet_ff_init().
>   - Validate ff->ff_actions->count != 0.
>   - Remove WARN_ON_ONCE() and replace -EINVAL with -EPROTO for errors on
>     device side.
>   - Use ff->ff_mask->count after it was initialized.
>   - Add a patch to fix sleeping under spinlock in the admin command path.
>   - Document that callers must zero-initialize the capability structure.
>   - Convert macro VIRTIO_CAP_IN_LIST to be inline function.
>   - Add WARN_ON_ONCE if allocation fails in virtio_admin_obj_destroy().
>   - Change VIRTNET_FF_ETHTOOL_GROUP_PRIORITY to be 0.
>   - Reject flow rules that require more selectors than the device supports
>     (selectors_per_classifier_limit).
>   - Report min(rules_limit, rules_per_group_limit) as the effective rule limit,
>     since all rules reside in a single group.
> 
> v23:
>   - Make virtio_admin_obj_destroy() return int instead of void + internal
>     WARN_ON_ONCE, so callers can propagate the failure themselves.
> 
> Comments from internal Sashiko review:
> > +int virtio_admin_obj_create(struct virtio_device *vdev,
> > +			    u16 obj_type,
> > +			    u32 obj_id,
> > +			    u16 group_type,
> > +			    u64 group_member_id,
> > +			    const void *obj_specific_data,
> > +			    size_t obj_specific_data_size)
> > +{
> [ ... ]
> > +	obj_create_data->hdr.type = cpu_to_le16(obj_type);
> > +	obj_create_data->hdr.id = cpu_to_le32(obj_id);
> > +	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
> > +	       obj_specific_data_size);
> Can this memcpy trigger undefined behavior if callers pass NULL for
> obj_specific_data and 0 for obj_specific_data_size? In C, passing a NULL
> pointer to memcpy is undefined behavior even if the size is 0, which could
> cause UBSAN splats.
> 
> [SS] It's the caller responsibly not to pass NULL. Current callers don't pass NULL.
> 
> > +	sg_init_one(&data_sg, data, data_size);
> > +
> > +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
> > +	cmd.group_type = cpu_to_le16(group_type);
> > +	cmd.group_member_id = cpu_to_le64(group_member_id);
> > +	cmd.data_sg = &data_sg;
> > +
> > +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> This isn't a bug introduced by this patch, but does this execution path sleep
> while holding a spinlock with interrupts disabled?
> If this calls into virtqueue_exec_admin_cmd() (for example via
> vp_modern_admin_cmd_exec()), the call chain looks like:
> virtqueue_exec_admin_cmd()
>     spin_lock_irqsave(&admin_vq->lock, flags);
>     virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> Since GFP_KERNEL allows direct reclaim and sleeping, can this cause a deadlock
> or trigger a BUG if the virtqueue falls back to allocating indirect descriptors?
> 
> [SS] Right, but not related to this series. Can introduce a fix in a different patch.
> 
> > +void virtio_admin_obj_destroy(struct virtio_device *vdev,
> > +			      u16 obj_type,
> > +			      u32 obj_id,
> > +			      u16 group_type,
> > +			      u64 group_member_id)
> > +{
> > +	struct virtio_admin_cmd_resource_obj_cmd_hdr *data;
> > +	struct virtio_admin_cmd cmd = {};
> > +	struct scatterlist data_sg;
> > +	int err;
> > +
> > +	if (!vdev->config->admin_cmd_exec)
> > +		return;
> > +
> > +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return;
> If this kzalloc() fails, does this function return without sending the
> destroy command to the hardware?
> Since the return type is void, the caller has no way to detect this failure
> and will likely free its local software tracking objects. Will this leave
> the physical device and software tracking structures permanently
> desynchronized, leaking the object on the virtio device?
> 
> [SS] Not much to do here. There could be a firmware error also,
>      and then what we do? Destroy flow is best effort.
> 
> > +			break;
> > +
> > +		if (sel->length != get_mask_size(sel->type) ||
> > +		    test_and_set_bit(sel->type, &sel_types)) {
> > +			WARN_ON_ONCE(true);
> Is it safe to use WARN_ON_ONCE() for validating device input? A malicious
> or buggy virtio device could intentionally trigger this by providing an
> invalid length or duplicate selector types, potentially leading to a kernel
> panic if panic_on_warn is enabled. Would it be better to just return the
> error without the warning?
> > +			err = -EINVAL;
> > +			goto err_ff_action;
> > +		}
> > +		real_ff_mask_size += sizeof(struct virtio_net_ff_selector) + sel->length;
> > +		if (real_ff_mask_size > ff_mask_size) {
> > +			WARN_ON_ONCE(true);
> Similarly, could this WARN_ON_ONCE() be triggered by a malicious device
> providing a larger size than expected, leading to a panic if panic_on_warn
> is enabled?
> 
> [SS] These WARN_ON_ONCE() were requested by MST during review.
> 
> [ ... ]
> > +static const struct ethtool_ops virtnet_ethtool_ops = {
> [ ... ]
> > +	.get_rxfh_fields = virtnet_get_hashflow,
> > +	.set_rxfh_fields = virtnet_set_hashflow,
> > +	.get_rx_ring_count = virtnet_get_rx_ring_count,
> > +	.set_rxnfc = virtnet_set_rxnfc,
> > +};
> Does this also need an implementation of .get_rxnfc to prevent invalid
> channel configurations?
> When .get_rxnfc is missing, the ethtool core function
> ethtool_get_max_rxnfc_channel() returns -EOPNOTSUPP. This causes
> ethtool_check_max_channel() to silently skip validating the new channel
> count against active flow rules.
> If a user adds a flow rule targeting a specific RX queue, and then reduces
> the active RX queues via ethtool -L, virtnet_set_channels() will update
> the queue count without checking the active rules stored in
> vi->ff.ethtool.rules.
> Could this leave hardware rules pointing to disabled queues and cause
> silent packet drops?
> 
> [SS] .get_rxnfc exists (introduced in a later patch).
> 
> > @@ -1258,6 +1259,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> >  	.get_shm_region  = vp_get_shm_region,
> >  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> >  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> > +	.admin_cmd_exec = vp_modern_admin_cmd_exec,
> >  };
> This isn't a bug introduced by this patch, but by exposing this function to
> general drivers, can a driver trigger a NULL pointer dereference or
> use-after-free?
> If a driver calls the newly exposed config->admin_cmd_exec() early in probe
> before find_vqs(), or late in remove after del_vqs(),
> virtqueue_exec_admin_cmd() unconditionally dereferences admin_vq->info:
>     vq = admin_vq->info->vq;
> In vp_del_vqs(), the info structure is freed via kfree(info), but
> vp_dev->admin_vq.info is never set to NULL, leaving a dangling pointer.
> 
> [SS] Current code is written in correct order. Future callers are responsible
>      to call this when VQs are initialized.
> 
> This isn't a bug introduced by this patch, but does this error path lead to
> stack corruption?
> In virtqueue_exec_admin_cmd(), if virtqueue_kick() fails, it returns -EIO
> without waiting for completion or detaching the exposed buffer:
>     if (!virtqueue_kick(vq))
>             goto unlock_err;
> If the hardware later processes the descriptor and triggers
> vp_modern_avq_done(), it will call complete(&cmd->completion). Since the cmd
> structure was likely allocated on the caller's stack, this can overwrite
> out-of-scope stack memory.
> 
> [SS] The vq->broken flag set by virtqueue_notify() causes virtqueue_get_buf()
>      to return NULL in vp_modern_avq_done(), so complete() is never called on
>      the freed cmd. No stack corruption.
> 
> This isn't a bug introduced by this patch, but can this function return an
> uninitialized variable?
> virtqueue_exec_admin_cmd() returns cmd->ret at the end of the function:
>     return cmd->ret;
> However, on a successful command execution, vp_modern_avq_done() never
> explicitly sets cmd->ret to 0. If a driver allocates struct virtio_admin_cmd
> on the stack without zero-initializing it, could this return stack garbage
> and erroneously indicate failure?
> 
> [SS] cmd is initialized in all callers struct with virtio_admin_cmd cmd = {};
> 
> If the device responds with fewer bytes than the driver's provided data
> buffer, does this leave the rest of the output buffer uninitialized?
> The wrapper discards cmd.result_sg_size without zero-initializing data and
> returns 0, which could expose uninitialized memory to callers.
> 
> [SS] Caller's responsibility to initialize the buffer.
> 
> Second, if virtqueue_add_sgs() returns -ENOSPC, the code loops infinitely
> with cpu_relax() without a timeout or sleep:
> drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
> ...
> 		if (ret == -ENOSPC) {
> 			spin_unlock_irqrestore(&admin_vq->lock, flags);
> 			cpu_relax();
> 			goto again;
> 		}
> ...
> }
> Can this soft-lockup the CPU if the virtqueue remains full and the device is
> unresponsive?
> 
> [SS] Pre-existing, not introduced by this series, and bounded in practice by
>      the virtqueue_is_broken() check. Not something to fix here.
> 
> Third, if virtqueue_kick() fails, the code jumps to unlock_err without
> detaching the buffer from the virtqueue:
> drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
> ...
> 	if (!virtqueue_kick(vq))
> 		goto unlock_err;
> 	spin_unlock_irqrestore(&admin_vq->lock, flags);
> 	wait_for_completion(&cmd->completion);
> 	return cmd->ret;
> unlock_err:
> 	spin_unlock_irqrestore(&admin_vq->lock, flags);
> 	return -EIO;
> ...
> }
> Does this leave a dangling pointer on the virtqueue?
> If the device later processes the command or the virtqueue is torn down,
> could it write to the freed heap memory and invoke complete() on a dangling
> stack pointer?
> 
> [SS] Not a bug. vq->broken prevents complete() from firing.
> 
> > +	sg_init_one(&data_sg, data, sizeof(*data));
> > +	sg_init_one(&result_sg, caps, cap_size);
> > +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DEVICE_CAP_GET);
> > +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
> > +	cmd.data_sg = &data_sg;
> > +	cmd.result_sg = &result_sg;
> > +
> > +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> Similar to virtio_admin_cap_id_list_query above, does this execution discard
> cmd.result_sg_size without zeroing the caps buffer, potentially returning
> uninitialized memory to the caller on a short response?
> 
> [SS] Standard kernel API contract, caller owns the buffer, current callers
>      handle it correctly.
> 
> > +	data->id = cpu_to_le16(id);
> > +	memcpy(data->cap_specific_data, caps, cap_size);
> If a caller invokes virtio_admin_cap_set() to enable a capability with no
> extra data (cap_size == 0 and caps == NULL), will this unconditionally pass a
> NULL pointer to memcpy(), invoking undefined behavior?
> 
> [SS] Caller's responsibility.
> 
> > +/**
> > + * VIRTIO_CAP_IN_LIST - Check if a capability is supported in the capability list
> > + * @cap_list: Pointer to capability list structure containing supported_caps array
> > + * @cap: Capability ID to check
> > + *
> > + * The cap_list contains a supported_caps array of little-endian 64-bit integers
> > + * where each bit represents a capability. Bit 0 of the first element represents
> > + * capability ID 0, bit 1 represents capability ID 1, and so on.
> > + *
> > + * Return: 1 if capability is supported, 0 otherwise
> > + */
> > +#define VIRTIO_CAP_IN_LIST(cap_list, cap) \
> > +	(!!(1 & (le64_to_cpu(cap_list->supported_caps[(cap) / 64]) >> (cap) % 64)))
> Could this macro trigger an out-of-bounds read on the supported_caps array if
> a caller passes a cap value greater than VIRTIO_ADMIN_MAX_CAP?
> Additionally, could evaluating cap multiple times cause unexpected side effects
> if passed an expression like cap++?
> 
> [SS] All callers use named cap constants defined in the uapi headers, which are all
>      within bounds by construction. Passing side-effecting expressions to macros is
>      standard caller responsibility. No fix needed.
> 
> >  static void virtnet_ff_cleanup(struct virtnet_ff *ff)
> >  {
> > +	struct virtnet_ethtool_rule *eth_rule;
> > +	unsigned long i;
> > +
> >  	if (!ff->ff_supported)
> >  		return;
> >
> > +	xa_for_each(&ff->ethtool.rules, i, eth_rule)
> > +		destroy_ethtool_rule(ff, eth_rule);
> > +
> Will this cause user-configured flow filter rules to be permanently lost
> across system suspend/resume or device migration?
> During suspend, device reset, or device freeze, virtnet_freeze_down() calls
> virtnet_ff_cleanup(). This loop iterates over all configured flow rules,
> sends the destroy commands to the hardware, and then calls kfree() via
> destroy_ethtool_rule(), permanently destroying the software representation of
> the rules.
> Upon resume, virtnet_restore_up() invokes virtnet_ff_init(), which
> initializes the flow filters as completely empty. The driver appears to make
> no attempt to retain the software state of the rules during suspend or
> replay them to the device during restore, meaning users must manually
> recreate all flow filter rules every time the system resumes or the device
> is migrated.
> 
> [SS] This is intentional for now. Could be a follow up feature.
> 
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
> -- 
> 2.49.0

Re: [PATCH net-next v23 00/14] virtio_net: Add ethtool flow rules support

[Michael S. Tsirkin]

On Mon, Aug 31, 2026 at 07:10:55PM +0300, Shahar Shitrit wrote:
> This series implements ethtool flow rules support for virtio_net using the
> virtio flow filter (FF) specification. The implementation allows users to
> configure packet filtering rules through ethtool commands, directing
> packets to specific receive queues, or dropping them based on various
> header fields.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> The series starts with infrastructure changes to expose virtio PCI admin
> capabilities and object management APIs. It then creates the virtio_net
> directory structure and implements the flow filter functionality with
> support for:
> 
> - Layer 2 (Ethernet) flow rules
> - IPv4 and IPv6 flow rules
> - TCP and UDP flow rules (both IPv4 and IPv6)
> - Rule querying and management operations
> 
> Setting, deleting and viewing flow filters, -1 action is drop, positive
> integers steer to that RQ:
> 
> $ ethtool -u ens9
> 4 RX rings available
> Total 0 rules
> 
> $ ethtool -U ens9 flow-type ether src 1c:34:da:4a:33:dd action 0
> Added rule with ID 0
> $ ethtool -U ens9 flow-type udp4 dst-port 5001 action 3
> Added rule with ID 1
> $ ethtool -U ens9 flow-type tcp6 src-ip fc00::2 dst-port 5001 action 2
> Added rule with ID 2
> $ ethtool -U ens9 flow-type ip4 src-ip 192.168.51.101 action 1
> Added rule with ID 3
> $ ethtool -U ens9 flow-type ip6 dst-ip fc00::1 action -1
> Added rule with ID 4
> $ ethtool -U ens9 flow-type ip6 src-ip fc00::2 action -1
> Added rule with ID 5
> $ ethtool -U ens9 delete 4
> $ ethtool -u ens9
> 4 RX rings available
> Total 5 rules
> 
> Filter: 0
>         Flow Type: Raw Ethernet
>         Src MAC addr: 1C:34:DA:4A:33:DD mask: 00:00:00:00:00:00
>         Dest MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF
>         Ethertype: 0x0 mask: 0xFFFF
>         Action: Direct to queue 0
> 
> Filter: 1
>         Rule Type: UDP over IPv4
>         Src IP addr: 0.0.0.0 mask: 255.255.255.255
>         Dest IP addr: 0.0.0.0 mask: 255.255.255.255
>         TOS: 0x0 mask: 0xff
>         Src port: 0 mask: 0xffff
>         Dest port: 5001 mask: 0x0
>         Action: Direct to queue 3
> 
> Filter: 2
>         Rule Type: TCP over IPv6
>         Src IP addr: fc00::2 mask: ::
>         Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
>         Traffic Class: 0x0 mask: 0xff
>         Src port: 0 mask: 0xffff
>         Dest port: 5001 mask: 0x0
>         Action: Direct to queue 2
> 
> Filter: 3
>         Rule Type: Raw IPv4
>         Src IP addr: 192.168.51.101 mask: 0.0.0.0
>         Dest IP addr: 0.0.0.0 mask: 255.255.255.255
>         TOS: 0x0 mask: 0xff
>         Protocol: 0 mask: 0xff
>         L4 bytes: 0x0 mask: 0xffffffff
>         Action: Direct to queue 1
> 
> Filter: 5
>         Rule Type: Raw IPv6
>         Src IP addr: fc00::2 mask: ::
>         Dest IP addr: :: mask: ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
>         Traffic Class: 0x0 mask: 0xff
>         Protocol: 0 mask: 0xff
>         L4 bytes: 0x0 mask: 0xffffffff
>         Action: Drop
> 
> ---
> v2: https://lore.kernel.org/netdev/20250908164046.25051-1-danielj@nvidia.com/
>   - Fix sparse warnings
>   - Fix memory leak on subsequent failure to allocate
>   - Fix some Typos
> 
> v3: https://lore.kernel.org/netdev/20250923141920.283862-1-danielj@nvidia.com/
>   - Added admin_ops to virtio_device kdoc.
> 
> v4:
>   - Fixed double free bug inserting flows
>   - Fixed incorrect protocol field check parsing ip4 headers.
>   - (u8 *) changed to (void *)
>   - Added kdoc comments to UAPI changes.
>   - No longer split up virtio_net.c
>   - Added config op to execute admin commands.
>       - virtio_pci assigns vp_modern_admin_cmd_exec to this callback.
>   - Moved admin command API to new core file virtio_admin_commands.c
> 
> v5:
>   - Fixed compile error
>   - Fixed static analysis warning on () after macro
>   - Added missing fields to kdoc comments
>   - Aligned parameter name between prototype and kdoc
> 
> v6:
>   - Fix sparse warning "array of flexible structures" Jakub K/Simon H
>   - Use new variable and validate ff_mask_size before set_cap. MST
> 
> v7:
>   - Change virtnet_ff_init to return a value. Allow -EOPNOTSUPP. Xuan
>   - Set ff->ff_{caps, mask, actions} NULL in error path. Paolo Abini
>   - Move for (int i removal hung back a patch. Paolo Abini
> 
> v8
>   - Removed unused num_classifiers. Jason Wang
>   - Use real_ff_mask_size when setting the selector caps. Jason Wang
> 
> v9:
>   - Set err to -ENOMEM after alloc failures in virtnet_ff_init. Simon H
> 
> v10:
>   - Return -EOPNOTSUPP in virnet_ff_init before allocing any memory.
>     Jason Wang/Paolo Abeni
> 
> v11:
>   - Return -EINVAL if any resource limit is 0. Simon Horman
>   - Ensure we don't overrun alloced space of ff->ff_mask by moving the
>     real_ff_mask_size > ff_mask_size check into the loop. Simon Horman
> 
> v12: Many comments by MST, thanks Michael. Only the most significant
>      listed here:
>   - Fixed leak of key in build_and_insert.
>   - Fixed setting ethhdr proto for IPv6.
>   - Added 2 byte pad to struct virtio_net_ff_cap_data.
>   - Use and set rule_cnt when querying all flows.
>   - Cleanup and reinit in freeze/restore path.
> 
> v13:
>   - Add private comment for reserved field in kdoc. Jakub
>   - Serveral comments from MST details in patches. Most significant:
> 	- Fixed bug in ip4, check l3_mask vs mask when setting addrs.
> 	- Changed ff_mask cap checking to not break on expanded
> 	  selector types
> 	- Changed virtio_admin_obj_destroy to return void.
> 	- Check tos field for ip4.
> 	- Don't accept tclass field for ip6.
> 	- If ip6 only flow check that l4_proto isn't set.
> 
> v14:
>   - Handle virtio_ff_init errors in freeze/restore. MST
>   - Don't set proto in parse_ip4/6. The casted struct may not have that
>     field, and the proto field was set explicitly anyway. Simon H/AI.
> 
> v15:
>   - In virtnet_restore_up only call virtnet_close in err path if
>     netif_running. AI
> 
> v16:
>   - Return 0 from virtnet_restore_up if virtnet_init_ff return not
>     supported. AI
>   - Rebased over removing series to remove delayed refill.
> 
> v17:
>   - Properly handle unaligned reads/writes. MST
>   - Fix use after free if init fails during virtnet_restor. AI
>   - Fix memory leak when validating the classifer vs caps fails. AI
>   - Added missing includes. MSTA
> 
> v18:
>   - Validate selector cap lengths, instead of just checking they don't
>     exceed a max. AI
>   - Add __count_by attribute to flexible arrays in UAPI definitions.
>     Paolo A.
> 
> v19:
>   - Style fixes. AI
> 
> v20:
>   - Added missing include
> 
> v21:
>   - Use le64_to_cpu() and BIT_ULL() instead of cpu_to_le64() for cap checking.
>   - Don't use __counted_by on flexible array of flexible structs.
>   - Replace UAPI header includes with kernel header includes.
>   - Add missing includes for linux/types.h and linux/byteorder/generic.h.
>   - Clamp the reported action count to the driver-supported maximum.
>   - Clamp the reported selector count to the driver-supported maximum.
>   - Validate sel->type is not 0.
>   - Reduce selectors' count in case selector's type is invalid.
>   - Move virtio_device_ready() before virtnet_ff_init() as the flow filter
>     initialization requires the device to be in ready state to issue admin
>     commands.
>   - Remove forward declarations.
>   - Validate action is supported before inserting rule.
>   - Convert ring_cookie to vq before assigning ff_rule->vq_index.
>   - reword a comment.
>   - Introduce a new patch that moves flow_type_mask() to include/linux/ethtool.h.
>   - Wrap __le32 limit fields in le32_to_cpu() to avoid sparse warnings.
>   - Use put_unaligned() in parse_ip4() to avoid misaligned 32-bit stores on
>     strict-alignment architectures.
> 
> v22:
>   - Reword/fix typo in commit messages.
>   - Remove include <uapi/linux/virtio_pci.h> from virtio_net.c.
>   - Verify also selectors_per_classifier_limit in virtnet_ff_init().
>   - Validate ff->ff_actions->count != 0.
>   - Remove WARN_ON_ONCE() and replace -EINVAL with -EPROTO for errors on
>     device side.
>   - Use ff->ff_mask->count after it was initialized.
>   - Add a patch to fix sleeping under spinlock in the admin command path.
>   - Document that callers must zero-initialize the capability structure.
>   - Convert macro VIRTIO_CAP_IN_LIST to be inline function.
>   - Add WARN_ON_ONCE if allocation fails in virtio_admin_obj_destroy().
>   - Change VIRTNET_FF_ETHTOOL_GROUP_PRIORITY to be 0.
>   - Reject flow rules that require more selectors than the device supports
>     (selectors_per_classifier_limit).
>   - Report min(rules_limit, rules_per_group_limit) as the effective rule limit,
>     since all rules reside in a single group.
> 
> v23:
>   - Make virtio_admin_obj_destroy() return int instead of void + internal
>     WARN_ON_ONCE, so callers can propagate the failure themselves.
> 
> Comments from internal Sashiko review:
> > +int virtio_admin_obj_create(struct virtio_device *vdev,
> > +			    u16 obj_type,
> > +			    u32 obj_id,
> > +			    u16 group_type,
> > +			    u64 group_member_id,
> > +			    const void *obj_specific_data,
> > +			    size_t obj_specific_data_size)
> > +{
> [ ... ]
> > +	obj_create_data->hdr.type = cpu_to_le16(obj_type);
> > +	obj_create_data->hdr.id = cpu_to_le32(obj_id);
> > +	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
> > +	       obj_specific_data_size);
> Can this memcpy trigger undefined behavior if callers pass NULL for
> obj_specific_data and 0 for obj_specific_data_size? In C, passing a NULL
> pointer to memcpy is undefined behavior even if the size is 0, which could
> cause UBSAN splats.
> 
> [SS] It's the caller responsibly not to pass NULL. Current callers don't pass NULL.
> 
> > +	sg_init_one(&data_sg, data, data_size);
> > +
> > +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
> > +	cmd.group_type = cpu_to_le16(group_type);
> > +	cmd.group_member_id = cpu_to_le64(group_member_id);
> > +	cmd.data_sg = &data_sg;
> > +
> > +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> This isn't a bug introduced by this patch, but does this execution path sleep
> while holding a spinlock with interrupts disabled?
> If this calls into virtqueue_exec_admin_cmd() (for example via
> vp_modern_admin_cmd_exec()), the call chain looks like:
> virtqueue_exec_admin_cmd()
>     spin_lock_irqsave(&admin_vq->lock, flags);
>     virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> Since GFP_KERNEL allows direct reclaim and sleeping, can this cause a deadlock
> or trigger a BUG if the virtqueue falls back to allocating indirect descriptors?
> 
> [SS] Right, but not related to this series. Can introduce a fix in a different patch.
> 
> > +void virtio_admin_obj_destroy(struct virtio_device *vdev,
> > +			      u16 obj_type,
> > +			      u32 obj_id,
> > +			      u16 group_type,
> > +			      u64 group_member_id)
> > +{
> > +	struct virtio_admin_cmd_resource_obj_cmd_hdr *data;
> > +	struct virtio_admin_cmd cmd = {};
> > +	struct scatterlist data_sg;
> > +	int err;
> > +
> > +	if (!vdev->config->admin_cmd_exec)
> > +		return;
> > +
> > +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return;
> If this kzalloc() fails, does this function return without sending the
> destroy command to the hardware?
> Since the return type is void, the caller has no way to detect this failure
> and will likely free its local software tracking objects. Will this leave
> the physical device and software tracking structures permanently
> desynchronized, leaking the object on the virtio device?
> 
> [SS] Not much to do here. There could be a firmware error also,
>      and then what we do? Destroy flow is best effort.
> 
> > +			break;
> > +
> > +		if (sel->length != get_mask_size(sel->type) ||
> > +		    test_and_set_bit(sel->type, &sel_types)) {
> > +			WARN_ON_ONCE(true);
> Is it safe to use WARN_ON_ONCE() for validating device input? A malicious
> or buggy virtio device could intentionally trigger this by providing an
> invalid length or duplicate selector types, potentially leading to a kernel
> panic if panic_on_warn is enabled. Would it be better to just return the
> error without the warning?
> > +			err = -EINVAL;
> > +			goto err_ff_action;
> > +		}
> > +		real_ff_mask_size += sizeof(struct virtio_net_ff_selector) + sel->length;
> > +		if (real_ff_mask_size > ff_mask_size) {
> > +			WARN_ON_ONCE(true);
> Similarly, could this WARN_ON_ONCE() be triggered by a malicious device
> providing a larger size than expected, leading to a panic if panic_on_warn
> is enabled?
> 
> [SS] These WARN_ON_ONCE() were requested by MST during review.
> 
> [ ... ]
> > +static const struct ethtool_ops virtnet_ethtool_ops = {
> [ ... ]
> > +	.get_rxfh_fields = virtnet_get_hashflow,
> > +	.set_rxfh_fields = virtnet_set_hashflow,
> > +	.get_rx_ring_count = virtnet_get_rx_ring_count,
> > +	.set_rxnfc = virtnet_set_rxnfc,
> > +};
> Does this also need an implementation of .get_rxnfc to prevent invalid
> channel configurations?
> When .get_rxnfc is missing, the ethtool core function
> ethtool_get_max_rxnfc_channel() returns -EOPNOTSUPP. This causes
> ethtool_check_max_channel() to silently skip validating the new channel
> count against active flow rules.
> If a user adds a flow rule targeting a specific RX queue, and then reduces
> the active RX queues via ethtool -L, virtnet_set_channels() will update
> the queue count without checking the active rules stored in
> vi->ff.ethtool.rules.
> Could this leave hardware rules pointing to disabled queues and cause
> silent packet drops?
> 
> [SS] .get_rxnfc exists (introduced in a later patch).
> 
> > @@ -1258,6 +1259,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> >  	.get_shm_region  = vp_get_shm_region,
> >  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> >  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> > +	.admin_cmd_exec = vp_modern_admin_cmd_exec,
> >  };
> This isn't a bug introduced by this patch, but by exposing this function to
> general drivers, can a driver trigger a NULL pointer dereference or
> use-after-free?
> If a driver calls the newly exposed config->admin_cmd_exec() early in probe
> before find_vqs(), or late in remove after del_vqs(),
> virtqueue_exec_admin_cmd() unconditionally dereferences admin_vq->info:
>     vq = admin_vq->info->vq;
> In vp_del_vqs(), the info structure is freed via kfree(info), but
> vp_dev->admin_vq.info is never set to NULL, leaving a dangling pointer.
> 
> [SS] Current code is written in correct order. Future callers are responsible
>      to call this when VQs are initialized.
> 
> This isn't a bug introduced by this patch, but does this error path lead to
> stack corruption?
> In virtqueue_exec_admin_cmd(), if virtqueue_kick() fails, it returns -EIO
> without waiting for completion or detaching the exposed buffer:
>     if (!virtqueue_kick(vq))
>             goto unlock_err;
> If the hardware later processes the descriptor and triggers
> vp_modern_avq_done(), it will call complete(&cmd->completion). Since the cmd
> structure was likely allocated on the caller's stack, this can overwrite
> out-of-scope stack memory.
> 
> [SS] The vq->broken flag set by virtqueue_notify() causes virtqueue_get_buf()
>      to return NULL in vp_modern_avq_done(), so complete() is never called on
>      the freed cmd. No stack corruption.
> 
> This isn't a bug introduced by this patch, but can this function return an
> uninitialized variable?
> virtqueue_exec_admin_cmd() returns cmd->ret at the end of the function:
>     return cmd->ret;
> However, on a successful command execution, vp_modern_avq_done() never
> explicitly sets cmd->ret to 0. If a driver allocates struct virtio_admin_cmd
> on the stack without zero-initializing it, could this return stack garbage
> and erroneously indicate failure?
> 
> [SS] cmd is initialized in all callers struct with virtio_admin_cmd cmd = {};
> 
> If the device responds with fewer bytes than the driver's provided data
> buffer, does this leave the rest of the output buffer uninitialized?
> The wrapper discards cmd.result_sg_size without zero-initializing data and
> returns 0, which could expose uninitialized memory to callers.
> 
> [SS] Caller's responsibility to initialize the buffer.
> 
> Second, if virtqueue_add_sgs() returns -ENOSPC, the code loops infinitely
> with cpu_relax() without a timeout or sleep:
> drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
> ...
> 		if (ret == -ENOSPC) {
> 			spin_unlock_irqrestore(&admin_vq->lock, flags);
> 			cpu_relax();
> 			goto again;
> 		}
> ...
> }
> Can this soft-lockup the CPU if the virtqueue remains full and the device is
> unresponsive?
> 
> [SS] Pre-existing, not introduced by this series, and bounded in practice by
>      the virtqueue_is_broken() check. Not something to fix here.
> 
> Third, if virtqueue_kick() fails, the code jumps to unlock_err without
> detaching the buffer from the virtqueue:
> drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
> ...
> 	if (!virtqueue_kick(vq))
> 		goto unlock_err;
> 	spin_unlock_irqrestore(&admin_vq->lock, flags);
> 	wait_for_completion(&cmd->completion);
> 	return cmd->ret;
> unlock_err:
> 	spin_unlock_irqrestore(&admin_vq->lock, flags);
> 	return -EIO;
> ...
> }
> Does this leave a dangling pointer on the virtqueue?
> If the device later processes the command or the virtqueue is torn down,
> could it write to the freed heap memory and invoke complete() on a dangling
> stack pointer?
> 
> [SS] Not a bug. vq->broken prevents complete() from firing.
> 
> > +	sg_init_one(&data_sg, data, sizeof(*data));
> > +	sg_init_one(&result_sg, caps, cap_size);
> > +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DEVICE_CAP_GET);
> > +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
> > +	cmd.data_sg = &data_sg;
> > +	cmd.result_sg = &result_sg;
> > +
> > +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> Similar to virtio_admin_cap_id_list_query above, does this execution discard
> cmd.result_sg_size without zeroing the caps buffer, potentially returning
> uninitialized memory to the caller on a short response?
> 
> [SS] Standard kernel API contract, caller owns the buffer, current callers
>      handle it correctly.
> 
> > +	data->id = cpu_to_le16(id);
> > +	memcpy(data->cap_specific_data, caps, cap_size);
> If a caller invokes virtio_admin_cap_set() to enable a capability with no
> extra data (cap_size == 0 and caps == NULL), will this unconditionally pass a
> NULL pointer to memcpy(), invoking undefined behavior?
> 
> [SS] Caller's responsibility.
> 
> > +/**
> > + * VIRTIO_CAP_IN_LIST - Check if a capability is supported in the capability list
> > + * @cap_list: Pointer to capability list structure containing supported_caps array
> > + * @cap: Capability ID to check
> > + *
> > + * The cap_list contains a supported_caps array of little-endian 64-bit integers
> > + * where each bit represents a capability. Bit 0 of the first element represents
> > + * capability ID 0, bit 1 represents capability ID 1, and so on.
> > + *
> > + * Return: 1 if capability is supported, 0 otherwise
> > + */
> > +#define VIRTIO_CAP_IN_LIST(cap_list, cap) \
> > +	(!!(1 & (le64_to_cpu(cap_list->supported_caps[(cap) / 64]) >> (cap) % 64)))
> Could this macro trigger an out-of-bounds read on the supported_caps array if
> a caller passes a cap value greater than VIRTIO_ADMIN_MAX_CAP?
> Additionally, could evaluating cap multiple times cause unexpected side effects
> if passed an expression like cap++?
> 
> [SS] All callers use named cap constants defined in the uapi headers, which are all
>      within bounds by construction. Passing side-effecting expressions to macros is
>      standard caller responsibility. No fix needed.
> 
> >  static void virtnet_ff_cleanup(struct virtnet_ff *ff)
> >  {
> > +	struct virtnet_ethtool_rule *eth_rule;
> > +	unsigned long i;
> > +
> >  	if (!ff->ff_supported)
> >  		return;
> >
> > +	xa_for_each(&ff->ethtool.rules, i, eth_rule)
> > +		destroy_ethtool_rule(ff, eth_rule);
> > +
> Will this cause user-configured flow filter rules to be permanently lost
> across system suspend/resume or device migration?
> During suspend, device reset, or device freeze, virtnet_freeze_down() calls
> virtnet_ff_cleanup(). This loop iterates over all configured flow rules,
> sends the destroy commands to the hardware, and then calls kfree() via
> destroy_ethtool_rule(), permanently destroying the software representation of
> the rules.
> Upon resume, virtnet_restore_up() invokes virtnet_ff_init(), which
> initializes the flow filters as completely empty. The driver appears to make
> no attempt to retain the software state of the rules during suspend or
> replay them to the device during restore, meaning users must manually
> recreate all flow filter rules every time the system resumes or the device
> is migrated.
> 
> [SS] This is intentional for now. Could be a follow up feature.
> 
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>
> -- 
> 2.49.0

Re: [net-next,v23,02/14] virtio_pci: Fix sleeping under spinlock in admin command path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 2 (2) · Medium: 2 · Low: 1

- [High] The patch adds a new `vp_dev->admin_vq.info = NULL` store in
  vp_del_vq() (drivers/virtio/virtio_pci_common.c:261) but does not…
- [High] Forward-progress regression / livelock in the -ENOSPC retry loop
  of virtqueue_exec_admin_cmd().
- [Medium] Missing `Fixes:` tag and `Cc: stable` for a live sleep-in-
  atomic defect.
- [Medium] vp_modern_avq_work() dereferences admin_vq->info at function
  entry (drivers/virtio/virtio_pci_modern.c:59) with no NULL check, but…
- [Low] vp_modern_avq_done() no longer touches the virtqueue at all - it
  only calls schedule_work() (drivers/virtio/virtio_pci_modern.c:47-52).

Pre-existing issues:
- [High] Admin-command submission, reset cleanup and queue deletion do
  not share the admin_vq lock, so teardown can run concurrently with…
- [High] The admin-queue completion path converts the device-supplied
  used length into a result size without bounding it against the…

--

virtio_pci: Fix sleeping under spinlock in admin command path

Replaces the admin virtqueue spinlock with a mutex so that
virtqueue_add_sgs() may use GFP_KERNEL, and moves the admin virtqueue
completion handling out of the interrupt callback into a work item, since a
mutex cannot be taken from interrupt context.

No Link: tags present in the commit header.

A few questions below about the new teardown/NULL handling, the -ENOSPC
retry loop, and the missing Fixes: tag.

> virtqueue_exec_admin_cmd() calls virtqueue_add_sgs() with GFP_KERNEL
> while holding a spinlock. GFP_KERNEL allocations can sleep, which is
> illegal in atomic context.
>
> The issue becomes more significant with the next commit, which exposes
> vp_modern_admin_cmd_exec() through virtio_config_ops. This makes the
> admin command path available to device drivers and potentially reachable
> from userspace, increasing the likelihood of hitting the invalid sleep
> under memory pressure.

[Severity: Medium]
Should this patch carry a Fixes: tag and a stable Cc?

Both the spin_lock_irqsave(&admin_vq->lock, flags) and the
virtqueue_add_sgs(..., GFP_KERNEL) it wraps in virtqueue_exec_admin_cmd()
were added by:

  4c3b54af907e ("virtio_pci_modern: use completion instead of busy loop to
  wait on admin cmd result")

The sleep looks reachable on released kernels rather than only after the
follow-up commit: virtqueue_add_split() allocates before the -ENOSPC check,

drivers/virtio/virtio_ring.c:virtqueue_add_split() {
	...
	if (virtqueue_use_indirect(vq, total_sg))
		desc = alloc_indirect_split(vq, total_sg, gfp);
	...
}

and alloc_indirect_split() does a kmalloc(size, gfp), so any admin command
with total_sg > 1 on a device that negotiated VIRTIO_RING_F_INDIRECT_DESC
does a GFP_KERNEL kmalloc with interrupts disabled and the spinlock held.
The vfio-virtio migration ioctls already drive that path today. Would the
wording "becomes more significant with the next commit" be better phrased
as the bug being reachable now, with a Fixes: tag so backporting tooling
can pick it up?

> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 10371ecbc054c..6bc0c0f6ac503 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -256,6 +256,11 @@ static void vp_del_vq(struct virtqueue *vq, struct virtio_pci_vq_info *info)
>  		spin_unlock_irqrestore(&vp_dev->lock, flags);
>  	}
>  
> +	if (vp_is_avq(vq->vdev, vq->index)) {
> +		cancel_work_sync(&vp_dev->admin_vq.work);
> +		vp_dev->admin_vq.info = NULL;
> +	}
> +

[Severity: High]
Can this NULL store oops the next reset?

All three consumers of admin_vq.info dereference it and only test the
member, not the container pointer:

drivers/virtio/virtio_pci_modern.c:vp_modern_avq_cleanup() {
	...
	vq = vp_dev->admin_vq.info->vq;
	if (!vq)
		return;
	...
}

drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	...
	vq = admin_vq->info->vq;
	if (!vq)
		return -EIO;
	...
}

drivers/virtio/virtio_pci_modern.c:vp_modern_avq_work() {
	...
	struct virtqueue *vq = admin_vq->info->vq;
	...
}

The suspend/reset flows appear to reach the first one with info == NULL:

virtblk_freeze_priv()			(also used for .reset_prepare)
    virtio_reset_device()
    vdev->config->del_vqs()
        vp_del_vqs() -> vp_del_vq() -> admin_vq.info = NULL

then on restore/reset_done:

virtio_device_restore_priv()
    virtio_reset_device()
        vp_reset() -> vp_modern_avq_cleanup() -> admin_vq.info->vq

virtio_reset_device() only writes status 0, so vdev->features still has
VIRTIO_F_ADMIN_VQ set and the feature-bit guard at the top of
vp_modern_avq_cleanup() does not stop it. virtnet_freeze() ->
remove_vq_common() has the same shape.

Would it make sense to check info itself at every consumer, and to
publish/clear it under the same lock the readers use? Right now the store
is done with no lock at all while the completion worker and command
submitters can be reading it.

Also, the reordering of vp_synchronize_vectors() and
vp_modern_avq_cleanup() in vp_reset() is explained only in the added code
comment. Could the commit message mention that behavioural change too?

>  	vp_dev->del_vq(info);
>  	kfree(info);
>  }

[ ... ]

> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index ef4a64eb99824..dab63b044a312 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -47,13 +47,20 @@ static int vp_avq_index(struct virtio_device *vdev, u16 *index, u16 *num)
>  void vp_modern_avq_done(struct virtqueue *vq)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> -	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
> +
> +	schedule_work(&vp_dev->admin_vq.work);
> +}

[Severity: Low]
This isn't a bug, but should the callback suppress further notifications
before deferring?

virtqueue_disable_cb() now runs only inside vp_modern_avq_work(), so
VRING_AVAIL_F_NO_INTERRUPT (or used_event with VIRTIO_RING_F_EVENT_IDX) is
not armed for the whole window between the device notification and the
worker being picked up. During that window every completed admin command
raises an interrupt whose only effect is a schedule_work() that returns
false because the work is already pending.

Since the admin queue normally shares the config/slow-path MSI-X vector
(vp_find_one_vq_msix() with slow_path=true uses VP_MSIX_CONFIG_VECTOR),
those extra interrupts also re-walk slow_virtqueues under vp_dev->lock with
interrupts disabled.

The usual virtio pattern is to disable the callback first, as
virtqueue_napi_schedule() does in virtio_net:

	if (napi_schedule_prep(napi)) {
		virtqueue_disable_cb(vq);
		...
	}

> +
> +static void vp_modern_avq_work(struct work_struct *work)
> +{
>  	unsigned int status_size = sizeof(struct virtio_admin_cmd_status);
> +	struct virtio_pci_admin_vq *admin_vq =
> +		container_of(work, struct virtio_pci_admin_vq, work);
> +	struct virtqueue *vq = admin_vq->info->vq;

[Severity: Medium]
Can this dereference run before admin_vq.info has been published?

vp_setup_vq() enables the queue and links it into the dispatch list before
assigning the admin info pointer:

drivers/virtio/virtio_pci_common.c:vp_setup_vq() {
	...
		if (!vp_is_slow_path_vector(msix_vec))
			list_add(&info->node, &vp_dev->virtqueues);
		else
			list_add(&info->node, &vp_dev->slow_virtqueues);
	...
	*p_info = info;
	...
}

In the shared-vector and INTx configurations the handler is registered
before any virtqueue exists, so vp_vring_interrupt() /
vp_vring_slow_path_interrupt() can call vring_interrupt() on the admin vq
inside that window if the device has advanced the used index. That runs
vp_modern_avq_done() -> schedule_work(), and the worker then reads
admin_vq->info->vq while info is still NULL (vp_dev is zero-allocated).

The old callback was immune because it received the vq as its argument and
never touched admin_vq->info. Would a NULL check on info here, or
publishing info before the queue is exposed to interrupt dispatch, be
preferable?

>  	struct virtio_admin_cmd *cmd;
> -	unsigned long flags;
>  	unsigned int len;
>  
> -	spin_lock_irqsave(&admin_vq->lock, flags);
> +	mutex_lock(&admin_vq->lock);
>  	do {
>  		virtqueue_disable_cb(vq);
>  		while ((cmd = virtqueue_get_buf(vq, &len))) {

[Severity: High]
This isn't a bug introduced by this patch, since the length handling was
moved verbatim from vp_modern_avq_done() into the new work item, but can
this leak uninitialized page contents to userspace?

The length written by the device is only checked against a lower bound, and
never against the size of the result scatterlist that was submitted:

drivers/virtio/virtio_pci_modern.c:vp_modern_avq_work() {
	...
			if (len < status_size)
				cmd->result_sg_size = 0;
			else
				cmd->result_sg_size = len - status_size;
	...
}

virtqueue_get_buf() validates the descriptor id, not the length, and
virtio_pci_admin_dev_parts_get() passes the value straight out:

	ret = vp_modern_admin_cmd_exec(virtio_dev, &cmd);
	if (!ret)
		*res_size = cmd.result_sg_size;

drivers/vfio/pci/virtio/migrate.c then trusts it:

virtiovf_read_device_context_chunk() {
	...
	buf->length = res_size;
	...
}

virtiovf_buf_read() {
	...
	copy_len = min_t(size_t,
			 vhca_buf->start_pos + vhca_buf->length - *pos, *len);
	...
}

and the backing pages are not zeroed:

virtiovf_add_migration_pages() {
	...
		filled = alloc_pages_bulk(GFP_KERNEL_ACCOUNT, to_fill,
					  page_list);
	...
}

So a device reporting a used length larger than what it actually wrote gets
stale kernel page contents copied to the migration fd reader; the page
lookup only fails past allocated_length, after earlier copy_to_user() calls
have already run. Should result_sg_size be clamped to the length of the
result scatterlist that was submitted?

[ ... ]

> @@ -100,11 +106,11 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
>  	if (virtqueue_is_broken(vq))
>  		return -EIO;
>  
> -	spin_lock_irqsave(&admin_vq->lock, flags);
> +	mutex_lock(&admin_vq->lock);
>  	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>  	if (ret < 0) {
>  		if (ret == -ENOSPC) {
> -			spin_unlock_irqrestore(&admin_vq->lock, flags);
> +			mutex_unlock(&admin_vq->lock);
>  			cpu_relax();
>  			goto again;
>  		}

[Severity: High]
Can this retry loop stop making progress now?

Before the patch, descriptors were reclaimed inline in the interrupt
callback, so the hard interrupt preempted the spinning task and freed ring
slots. After the patch the only remaining caller of virtqueue_get_buf() on
the admin vq is vp_modern_avq_work(), which has to be picked up by a
per-CPU bound worker on system_percpu_wq:

vp_modern_avq_done()
    schedule_work(&vp_dev->admin_vq.work)	/* system_percpu_wq */
        vp_modern_avq_work()
            virtqueue_get_buf()			/* only reclaim site */

The retry path has no guaranteed scheduling point: virtqueue_is_broken() is
a READ_ONCE, the uncontended mutex_lock()/mutex_unlock() fast paths only
reach might_sleep(), and cpu_relax() does not yield. With
CONFIG_PREEMPT_NONE=y (or PREEMPT_DYNAMIC booted with preempt=none), if the
work item is queued to the CPU running the busy loop, the bound worker
never gets to run, no descriptors are freed, and virtqueue_add_sgs() keeps
returning -ENOSPC. On a single-vCPU guest that CPU match is automatic.

The loop's only exits are success, virtqueue_is_broken() and a non-ENOSPC
error, none of which can become true without the worker running. Would a
cond_resched() here, or waiting on an event signalled by the completion
worker, or an unbound/WQ_MEM_RECLAIM workqueue, address this? Related: the
untimed wait_for_completion() below now also depends on worker
availability, and system_percpu_wq is not WQ_MEM_RECLAIM.

[ ... ]

> @@ -350,6 +356,8 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
>  	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
>  		return;
>  
> +	cancel_work_sync(&vp_dev->admin_vq.work);
> +
>  	vq = vp_dev->admin_vq.info->vq;
>  	if (!vq)
>  		return;

[Severity: High]
This is a pre-existing issue, as the cleanup path and vp_del_vq() ignored
the former spinlock too, but does the new cancel_work_sync() cover enough?
It serializes the completion worker only, not command submitters.

Nothing holds admin_vq->lock on the teardown side:

drivers/virtio/virtio_pci_modern.c:vp_modern_avq_cleanup() {
	...
	while ((cmd = virtqueue_detach_unused_buf(vq))) {
		cmd->ret = -EIO;
		complete(&cmd->completion);
	}
}

drivers/virtio/virtio_pci_common.c:vp_del_vq() {
	...
	vp_dev->del_vq(info);
	kfree(info);
}

while a submitter caches the queue pointer outside the lock:

drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	...
	vq = admin_vq->info->vq;
	if (!vq)
		return -EIO;
	...
	mutex_lock(&admin_vq->lock);
	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
	...
}

So a vp_reset() (suspend, FLR, unbind) on one CPU can detach descriptors
while another CPU is inside virtqueue_add_sgs() on the same vring, or free
the virtqueue and info after a submitter cached vq, or leave a command
waiting forever on a queue that was reset. The "caller must serialize
access for the given device" note on virtio_pci_admin_dev_parts_*() only
covers admin-command callers among themselves, not the PM/reset path.
Should the teardown side take admin_vq->lock as well?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,03/14] virtio: Add config_op for admin commands

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 (3) · Medium: 1 · Low: 1

- [Medium] The newly published `->admin_cmd_exec` transport op has no
  lifecycle precondition check or documentation:…
- [Low] The kernel-doc added for `@admin_cmd_exec` in
  include/linux/virtio_config.h states no calling-context constraint,…

Pre-existing issues:
- [High] Free-while-in-flight / use-after-free on the admin-command kick-
  failure path.
- [High] The admin-command path accesses `admin_vq->info` with no
  synchronization against admin-queue teardown, and the newly published…
- [High] `struct virtio_admin_cmd::result_sg_size`, now part of the
  generic `->admin_cmd_exec` contract, is filled in…

--

virtio: Add config_op for admin commands

This adds a new optional admin_cmd_exec callback to struct
virtio_config_ops and points it at vp_modern_admin_cmd_exec() in both
modern virtio-pci config op tables, so that virtio device drivers can
issue admin commands through the transport.

No Link: tags in the commit header.

A few questions about the newly published op below.

> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index dab63b044a312..3b2515e29728a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -1248,6 +1248,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>  	.get_shm_region  = vp_get_shm_region,
>  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
>  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> +	.admin_cmd_exec = vp_modern_admin_cmd_exec,
>  };

[Severity: Medium]
Now that any bound driver can reach this path, is there anything that keeps
admin_vq.info valid at the entry point?  virtqueue_exec_admin_cmd() starts
with:

	vq = admin_vq->info->vq;
	if (!vq)
		return -EIO;

The guard tests the inner vq pointer, so it does not catch info == NULL.
The only precondition tested by vp_modern_admin_cmd_exec() is:

	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
		return -EOPNOTSUPP;

That feature bit is set by vp_finalize_features() before the driver's probe
runs and it stays set across del_vqs(), while vp_del_vq() in
virtio_pci_common.c does:

	if (vp_is_avq(vq->vdev, vq->index)) {
		cancel_work_sync(&vp_dev->admin_vq.work);
		vp_dev->admin_vq.info = NULL;
	}

	vp_dev->del_vq(info);
	kfree(info);

So admin_vq.info is NULL before find_vqs() creates the admin queue, on the
find_vqs() error path, and after del_vqs().  Can a call made outside the
find_vqs()..del_vqs() window dereference NULL here?

The virtio_net consumers added later in this series do appear to stay inside
that window, so this may not be reachable in-tree today.  Would it still be
worth checking admin_vq->info and spelling out the required device/VQ state
in the new kernel-doc?

[Severity: High]
This isn't a bug introduced by this patch, but the kick-failure path in
virtqueue_exec_admin_cmd() looks like it leaves a descriptor in flight:

	mutex_lock(&admin_vq->lock);
	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
	...
	if (!virtqueue_kick(vq))
		goto unlock_err;
	...
unlock_err:
	mutex_unlock(&admin_vq->lock);
	return -EIO;

virtqueue_kick() returns false when the vring is broken, and
virtio_break_device() can be called asynchronously, for example from
virtio_pci_remove():

	if (!pci_device_is_present(pci_dev))
		virtio_break_device(&vp_dev->vdev);

so the state can change between the virtqueue_is_broken() pre-check and the
kick.  The descriptor added by virtqueue_add_sgs() is not reclaimed on that
path, and vp_modern_admin_cmd_exec() then frees the buffers it published:

err_cmd_exec:
	kfree(va_hdr);
err_alloc:
	kfree(va_status);
	return ret;

The token handed to virtqueue_add_sgs() is the caller's struct
virtio_admin_cmd, which goes out of scope when the caller returns.  When the
following reset runs vp_modern_avq_cleanup():

	while ((cmd = virtqueue_detach_unused_buf(vq))) {
		cmd->ret = -EIO;
		complete(&cmd->completion);
	}

does this write through a stale token belonging to another task's stack
frame, and can the device still DMA into the freed, device-writable
va_status?  Should the error path detach the descriptor (or defer the frees)
before this op is exposed to more drivers?

[Severity: High]
This is a pre-existing issue, but the admin queue accesses do not share a
lock with admin queue teardown, and this patch widens the set of contexts
that can submit commands without adding a serialization rule.

Reader side, with no lock held:

virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	vq = admin_vq->info->vq;
	if (!vq)
		return -EIO;
}

virtio_pci_modern.c:vp_modern_avq_cleanup() {
	vq = vp_dev->admin_vq.info->vq;
	if (!vq)
		return;

	while ((cmd = virtqueue_detach_unused_buf(vq))) {
}

vp_modern_avq_work() also reads admin_vq->info->vq before taking the mutex.

Writer side, holding only vp_dev->lock for the list removal:

virtio_pci_common.c:vp_del_vq() {
	if (vp_is_avq(vq->vdev, vq->index)) {
		cancel_work_sync(&vp_dev->admin_vq.work);
		vp_dev->admin_vq.info = NULL;
	}

	vp_dev->del_vq(info);
	kfree(info);
}

Since admin_vq->lock is not held by the cleanup or the teardown, can a
command submitted from an unrelated driver context concurrently with
del_vqs() or reset dereference NULL or the freed struct
virtio_pci_vq_info, and then operate on a destroyed vring?

The concrete unsynchronized submitter today is the existing
virtio_pci_admin_* path used by virtio-vfio-pci, which reaches a PF admin
queue while the PF's virtio driver can be unbound independently.  Should the
new kernel-doc carry an analogue of the @reset rule about not running in
parallel with virtqueues being added or removed?

> 
>  static const struct virtio_config_ops virtio_pci_config_ops = {
> @@ -1268,6 +1269,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  	.get_shm_region  = vp_get_shm_region,
>  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
>  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> +	.admin_cmd_exec = vp_modern_admin_cmd_exec,
>  };
>  
>  /* the PCI probing function */
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 69f84ea85d71a..e36a32e0a20c8 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -108,6 +108,10 @@ struct virtqueue_info {
>   *	Returns 0 on success or error status
>   *	If disable_vq_and_reset is set, then enable_vq_after_reset must also be
>   *	set.
> + * @admin_cmd_exec: Execute an admin VQ command (optional).
> + *	vdev: the virtio_device
> + *	cmd: the command to execute
> + *	Returns 0 on success or error status

[Severity: Low]
Could this kernel-doc also state the calling context?  The implementation
sleeps in several places: two kzalloc_obj() GFP_KERNEL allocations in
vp_modern_admin_cmd_exec(), and in virtqueue_exec_admin_cmd():

	mutex_lock(&admin_vq->lock);
	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
	...
	wait_for_completion(&cmd->completion);

The wait has no timeout and is only woken by vp_modern_avq_work() or by
vp_modern_avq_cleanup() on reset.

The header of this struct is otherwise explicit about context, for example
that @get/@set, @get_status/@set_status and @get_features/@finalize_features
are not safe from atomic context, and that a device must not be reset from
its vq or config callbacks.

No in-tree caller violates this today.  Would it help to note that
admin_cmd_exec may sleep, must be called from process context, and can block
until the device completes the command or the device is reset?

[Severity: High]
This is a pre-existing issue, but the cmd->result_sg_size field that this
contract now exposes to arbitrary drivers is filled straight from the
device-written used ring length.  In vp_modern_avq_work():

		while ((cmd = virtqueue_get_buf(vq, &len))) {
			if (len < status_size)
				cmd->result_sg_size = 0;
			else
				cmd->result_sg_size = len - status_size;

virtio_ring does not clamp that length against the posted buffer; in
virtqueue_get_buf_ctx_split() and virtqueue_get_buf_ctx_packed() only the
descriptor id is range-checked while the length is taken verbatim:

	*len = vring_read_split_used_len(vq, last_used);

Is result_sg_size therefore fully device-controlled and able to exceed the
caller's result scatterlist?  The existing consumer already trusts it:
virtio_pci_admin_dev_parts_get() does *res_size = cmd.result_sg_size; and
drivers/vfio/pci/virtio/migrate.c does buf->length = res_size; for a buffer
whose real size is ctx_size, which the migration file then reads from.

Should the transport clamp result_sg_size to the total length of the
caller's result_sg before completing the command, and should the new
kernel-doc say that this field is untrusted device input?

>   */
>  struct virtio_config_ops {
>  	void (*get)(struct virtio_device *vdev, unsigned offset,
> @@ -137,6 +141,8 @@ struct virtio_config_ops {
>  			       struct virtio_shm_region *region, u8 id);
>  	int (*disable_vq_and_reset)(struct virtqueue *vq);
>  	int (*enable_vq_after_reset)(struct virtqueue *vq);
> +	int (*admin_cmd_exec)(struct virtio_device *vdev,
> +			      struct virtio_admin_cmd *cmd);
>  };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,04/14] virtio: Expose generic device capability operations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 3 · Low: 3

- [Medium] virtio_admin_cap_id_list_query() and virtio_admin_cap_get()
  expose a caller-owned buffer to the device as a DMA-writable result…
- [Medium] virtio_cap_in_list() (include/linux/virtio_admin.h:27-33)
  guards the array index with BUILD_BUG_ON(cap > VIRTIO_ADMIN_MAX_CAP)…
- [Medium] drivers/virtio/Makefile adds virtio_admin_commands.o directly
  to obj-$(CONFIG_VIRTIO), which makes it an independent module…
- [Low] The patch removes the macro MAX_CAP_ID from the installed uapi
  header include/uapi/linux/virtio_pci.h and replaces it with…
- [Low] The kernel-doc for virtio_admin_cap_set() states '@caps: Pointer
  to capability data structure (must be heap allocated)', but the…
- [Low] The new exported wrappers only check that
  vdev->config->admin_cmd_exec is non-NULL before issuing an admin…

Pre-existing issues:
- [High] In virtqueue_exec_admin_cmd()
  (drivers/virtio/virtio_pci_modern.c:119-129), if virtqueue_kick()…

--

virtio: Expose generic device capability operations

This adds a new virtio_admin_commands.c with exported helpers to query the
capability id list and to get/set individual capabilities, plus a new
include/linux/virtio_admin.h.  It also grows
struct virtio_admin_cmd_query_cap_id_result from 8 to 512 bytes and renames
MAX_CAP_ID.

A few questions below.

> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index eefcfe90d6b8b..2b4a204dde331 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

[Severity: Medium]
Does this build with CONFIG_VIRTIO=m?  Adding virtio_admin_commands.o
directly to obj-$(CONFIG_VIRTIO) with no matching <name>-y makes it its own
module, and the new drivers/virtio/virtio_admin_commands.c has no
MODULE_LICENSE() (nor MODULE_DESCRIPTION()).

The sibling objects each carry their own license, which is what makes them
separate modules:

  drivers/virtio/virtio.c:      MODULE_LICENSE("GPL");
  drivers/virtio/virtio_ring.c: MODULE_LICENSE("GPL");

and modpost treats the missing license as an error rather than a warning:

  scripts/mod/modpost.c:check_modinfo() {
      license = get_modinfo(&info, "license");
      if (!license)
          mod_error(mod, "missing MODULE_LICENSE()\n");
  }

Was the intent to fold the file into the virtio module instead, e.g.
virtio-y := virtio.o virtio_admin_commands.o?  I did not find a later patch
in the series that adds the module metadata or changes this line.

>  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 0000000000000..60d92875da233
> --- /dev/null
> +++ b/drivers/virtio/virtio_admin_commands.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_admin.h>
> +#include <linux/overflow.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));

[Severity: Medium]
The commit message says "Since the API does not report how much of the
result was written, callers must zero-initialize the structure before use."
The transport does know that length though:

  drivers/virtio/virtio_pci_modern.c:vp_modern_avq_work() {
      ...
      if (len < status_size)
          cmd->result_sg_size = 0;
      else
          cmd->result_sg_size = len - status_size;
      complete(&cmd->completion);
  }

Since cmd lives on the stack here, result_sg_size is discarded.  Would it be
better for these helpers, which are the only place that knows both the buffer
and its size, to memset() the result buffer themselves or to return the
written length, rather than making it a caller contract?

Related: on an error return from admin_cmd_exec() the result buffer may
already be partially DMA-written, so a caller that zeroed once cannot safely
reuse the same buffer for a retry.  Is that expectation documented anywhere?

> +	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)
> +{
> +	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_obj(*data);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->id = cpu_to_le16(id);
> +	sg_init_one(&data_sg, data, sizeof(*data));
> +	sg_init_one(&result_sg, caps, cap_size);

[Severity: Medium]
Same partial-write question for virtio_admin_cap_get(): a device that writes
only a prefix of cap_size leaves the tail of the caller's buffer untouched
and this returns 0.  The kernel-doc for virtio_admin_cap_id_list_query() says
"(must be zero-initialized and heap allocated)" but the kernel-doc for
virtio_admin_cap_get() only says "(must be heap allocated)".  Should the
zero-initialization requirement be stated for both, given they share the same
hazard?

> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DEVICE_CAP_GET);
> +	cmd.group_type = cpu_to_le16(VIRTIO_ADMIN_GROUP_TYPE_SELF);
> +	cmd.data_sg = &data_sg;
> +	cmd.result_sg = &result_sg;
> +
> +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> +	kfree(data);

[Severity: High]
This isn't a bug introduced by this patch, but the transport error path this
new exported API now feeds looks like it can leave the virtqueue holding
&cmd and the just-kfree()d buffers.

In virtqueue_exec_admin_cmd(), if virtqueue_kick() fails after
virtqueue_add_sgs() already succeeded, it neither waits for the completion
nor detaches the buffer:

  drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
      ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
      ...
      if (!virtqueue_kick(vq))
              goto unlock_err;
      ...
  unlock_err:
      mutex_unlock(&admin_vq->lock);
      return -EIO;
  }

virtqueue_kick() can return false after the descriptor was added, since
virtqueue_notify() bails on a broken vq:

  drivers/virtio/virtio_ring.c:virtqueue_notify() {
      if (unlikely(vq->broken))
              return false;

and virtio_break_device() is called on surprise removal in
virtio_pci_remove(), i.e. after the virtqueue_is_broken() pre-check in
virtqueue_exec_admin_cmd().

On that return the helper here does kfree(data) and cmd goes out of scope,
while the queue still references both.  Later:

  drivers/virtio/virtio_pci_modern.c:vp_modern_avq_cleanup() {
      while ((cmd = virtqueue_detach_unused_buf(vq))) {
              cmd->ret = -EIO;
              complete(&cmd->completion);

writes through the stale token into a dead stack frame, and the device may
still DMA into the freed data buffer.  All existing callers use the same
stack-cmd pattern, so this is not new, but should the kick-failure path
detach the buffer before returning now that arbitrary virtio drivers can
reach it?

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtio_admin_cap_get);
> +
> +int virtio_admin_cap_set(struct virtio_device *vdev,
> +			 u16 id,
> +			 const void *caps,
> +			 size_t cap_size)
> +{

[ ... ]

> +	data->id = cpu_to_le16(id);
> +	memcpy(data->cap_specific_data, caps, cap_size);
> +	sg_init_one(&data_sg, data, data_size);

[Severity: Low]
This isn't a bug, but the kernel-doc for virtio_admin_cap_set() says
"@caps: Pointer to capability data structure (must be heap allocated)" while
the implementation copies caps into its own kzalloc()ed buffer and only
sg_init_one()s that private allocation.  The caller's buffer is never mapped
or retained, so a stack or static buffer works fine here.  Should the "must
be heap allocated" note be dropped for cap_set?

> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_DRIVER_CAP_SET);

[ ... ]

> +EXPORT_SYMBOL_GPL(virtio_admin_cap_set);
> diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
> new file mode 100644
> index 0000000000000..1177c4a20e53c
> --- /dev/null
> +++ b/include/linux/virtio_admin.h
> @@ -0,0 +1,89 @@

[ ... ]

> +/**
> + * virtio_cap_in_list - Check if a capability is supported in the capability list
> + * @cap_list: Pointer to capability list structure containing supported_caps array
> + * @cap: Capability ID to check
> + *
> + * The cap_list contains a supported_caps array of little-endian 64-bit integers
> + * where each bit represents a capability. Bit 0 of the first element represents
> + * capability ID 0, bit 1 represents capability ID 1, and so on.
> + *
> + * Return: true if capability is supported, false otherwise
> + */
> +static inline bool virtio_cap_in_list(
> +	const struct virtio_admin_cmd_query_cap_id_result *cap_list, u16 cap)
> +{
> +	BUILD_BUG_ON(cap > VIRTIO_ADMIN_MAX_CAP);
> +	return !!(1 & (le64_to_cpu(cap_list->supported_caps[cap / 64]) >>
> +		       (cap % 64)));
> +}

[Severity: Medium]
Is BUILD_BUG_ON() the right check here?  cap is a runtime u16 parameter, and

  include/linux/build_bug.h:
  #define BUILD_BUG_ON(condition) \
      BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)

is a compile-time assertion, so it puts no runtime bound on
supported_caps[cap / 64].  That array has VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE == 64
elements, so a cap in 0x1000..0xffff would index past the 512-byte struct.

The other direction is a build issue: for any caller where the compiler cannot
constant-fold cap, the __compiletime_assert() with __attribute__((error))
survives and the build fails with "call to __compiletime_assert_NN declared
with attribute error".

The kernel-doc advertises an unrestricted "@cap: Capability ID to check" with
no mention that only compile-time constants are accepted.  All callers in this
series pass literal macros (VIRTIO_NET_FF_RESOURCE_CAP and friends), so nothing
misbehaves today.  Would a plain runtime bounds check, or documenting the
constant-only requirement, be preferable?

> +
> +/**
> + * virtio_admin_cap_id_list_query - Query the list of available capability IDs
> + * @vdev: The virtio device to query
> + * @data: Pointer to result structure (must be zero-initialized and heap allocated)
> + *
> + * This function queries the virtio device for the list of available capability
> + * IDs that can be used with virtio_admin_cap_get() and virtio_admin_cap_set().
> + * The result is stored in the provided data structure.
> + *
> + * Return: 0 on success, -EOPNOTSUPP if the device doesn't support admin
> + * operations or capability queries, or a negative error code on other failures.
> + */
> +int virtio_admin_cap_id_list_query(struct virtio_device *vdev,
> +				   struct virtio_admin_cmd_query_cap_id_result *data);

[Severity: Low]
Would it help to add a "Context: process context, may sleep" line to the
kernel-doc of all three new helpers?  virtio_admin_cap_get() and
virtio_admin_cap_set() allocate with GFP_KERNEL, and
virtqueue_exec_admin_cmd() takes admin_vq->lock and blocks in
wait_for_completion(), but none of the new documentation states a context
requirement and there is no might_sleep().

The in-tree callers added later in the series run from probe and under
rtnl_lock() after init_vqs()/virtio_device_ready(), so they are fine; this is
just about the exported contract.

> +
> +/**
> + * virtio_admin_cap_get - Get capability data for a specific capability ID
> + * @vdev: The virtio device
> + * @id: Capability ID to retrieve
> + * @caps: Pointer to capability data structure (must be heap allocated)
> + * @cap_size: Size of the capability data structure

[ ... ]

> +#endif /* _LINUX_VIRTIO_ADMIN_H */
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index e732e3456e275..96d097d3757e4 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -315,15 +315,17 @@ struct virtio_admin_cmd_notify_info_result {

[ ... ]

> -#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 + 1, 64)

[Severity: Low]
This drops MAX_CAP_ID from an installed uapi header.  Nothing in the tree
uses it outside this file, so no in-tree build breaks, but out-of-tree or
userspace code that sized or iterated supported_caps[] with it will no longer
compile.

The commit message reasons only about the struct size change:

  "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."

Could the macro removal be mentioned in the changelog too, or a
#define MAX_CAP_ID VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE alias kept?

>  struct virtio_admin_cmd_query_cap_id_result {
> -	__le64 supported_caps[MAX_CAP_ID];
> +	__le64 supported_caps[VIRTIO_ADMIN_CAP_ID_ARRAY_SIZE];
>  };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,05/14] virtio: Expose object create and destroy API

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] virtio_admin_obj_create()/virtio_admin_obj_destroy()
  (drivers/virtio/virtio_admin_commands.c:98 and :142) forward a caller-…

Pre-existing issues:
- [Medium] Both new helpers kfree() the command payload (and let the
  stack-resident 'struct virtio_admin_cmd cmd' go out of scope)…

--

virtio: Expose object create and destroy API

This adds two exported helpers, virtio_admin_obj_create() and
virtio_admin_obj_destroy(), which build the resource object create/destroy
admin commands generically instead of only for dev parts objects.  The new
prototypes and kernel-doc go into include/linux/virtio_admin.h.

> diff --git a/drivers/virtio/virtio_admin_commands.c b/drivers/virtio/virtio_admin_commands.c
> index 60d92875da233..24b06075d944f 100644
> --- a/drivers/virtio/virtio_admin_commands.c
> +++ b/drivers/virtio/virtio_admin_commands.c
> @@ -94,3 +94,80 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(virtio_admin_cap_set);
> +
> +int virtio_admin_obj_create(struct virtio_device *vdev,
> +			    u16 obj_type,
> +			    u32 obj_id,
> +			    u16 group_type,
> +			    u64 group_member_id,
> +			    const void *obj_specific_data,
> +			    size_t obj_specific_data_size)
> +{

[ ... ]

> +	obj_create_data = data;
> +	obj_create_data->hdr.type = cpu_to_le16(obj_type);
> +	obj_create_data->hdr.id = cpu_to_le32(obj_id);
> +	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
> +	       obj_specific_data_size);
> +	sg_init_one(&data_sg, data, data_size);
> +
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
> +	cmd.group_type = cpu_to_le16(group_type);
> +	cmd.group_member_id = cpu_to_le64(group_member_id);
> +	cmd.data_sg = &data_sg;
> +
> +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> +	kfree(data);

[Severity: Medium]

This isn't a bug introduced by this patch, but is the unconditional
kfree(data) here safe for every return value of admin_cmd_exec()?

There is one path in virtqueue_exec_admin_cmd() where the descriptors have
already been published and the function returns without waiting and without
reclaiming them:

drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	...
	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
	if (ret < 0) {
		...
		goto unlock_err;
	}
	if (!virtqueue_kick(vq))
		goto unlock_err;
	mutex_unlock(&admin_vq->lock);

	wait_for_completion(&cmd->completion);

	return cmd->ret;

unlock_err:
	mutex_unlock(&admin_vq->lock);
	return -EIO;
}

On that -EIO return the buffer is still referenced by the virtqueue, and
virtio_admin_obj_create() then frees data and lets the stack-resident
struct virtio_admin_cmd cmd go out of scope.  The device-writable
va_status allocation in vp_modern_admin_cmd_exec() is freed on the same
error path:

drivers/virtio/virtio_pci_modern.c:vp_modern_admin_cmd_exec() {
	...
err_cmd_exec:
	kfree(va_hdr);
err_alloc:
	kfree(va_status);
	return ret;
}

In practice vp_notify() always returns true, so virtqueue_kick() can only
fail through the vq->broken check inside virtqueue_notify(), which returns
before notifying the device, and virtqueue_get_buf() returns NULL on a
broken vq, so vp_modern_avq_work() will not complete into the recycled
stack frame.  The same pattern exists at the baseline for
virtio_admin_cap_set(), virtio_pci_admin_mode_set() and
virtio_pci_admin_obj_create(); this patch adds two more callers of it.

Would the reclaim (a wait, or virtqueue_detach_unused_buf()) belong in
virtqueue_exec_admin_cmd() rather than in each caller?

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtio_admin_obj_create);
> +
> +int virtio_admin_obj_destroy(struct virtio_device *vdev,
> +			     u16 obj_type,
> +			     u32 obj_id,
> +			     u16 group_type,
> +			     u64 group_member_id)
> +{

[ ... ]

> +	data->type = cpu_to_le16(obj_type);
> +	data->id = cpu_to_le32(obj_id);
> +	sg_init_one(&data_sg, data, sizeof(*data));
> +	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_DESTROY);
> +	cmd.group_type = cpu_to_le16(group_type);
> +	cmd.group_member_id = cpu_to_le64(group_member_id);
> +	cmd.data_sg = &data_sg;
> +
> +	err = vdev->config->admin_cmd_exec(vdev, &cmd);
> +	kfree(data);

The kfree(data) note above applies here in virtio_admin_obj_destroy() as
well.

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtio_admin_obj_destroy);
> diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
> index 1177c4a20e53c..ebd4106835d68 100644
> --- a/include/linux/virtio_admin.h
> +++ b/include/linux/virtio_admin.h
> @@ -86,4 +86,48 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
>  			 const void *caps,
>  			 size_t cap_size);
>  
> +/**
> + * virtio_admin_obj_create - Create an object on a virtio device
> + * @vdev: the virtio device
> + * @obj_type: type of object to create
> + * @obj_id: ID for the new object
> + * @group_type: administrative group type for the operation
> + * @group_member_id: member identifier within the administrative group
> + * @obj_specific_data: object-specific data for creation
> + * @obj_specific_data_size: size of the object-specific data in bytes
> + *
> + * Creates a new object on the virtio device with the specified type and ID.
> + * The object may require object-specific data for proper initialization.

[Severity: Low]

This isn't a bug, but could the kernel-doc for virtio_admin_obj_create()
and virtio_admin_obj_destroy() say who owns the obj_id space?

The pre-existing dev parts helper keeps the id space in the core:

drivers/virtio/virtio_pci_modern.c:virtio_pci_admin_obj_create() {
	...
	id = ida_alloc_range(&avq->dev_parts_ida, 0,
			     avq->max_dev_parts_objects - 1, GFP_KERNEL);
	...
}

and it still rejects any obj_type other than VIRTIO_RESOURCE_OBJ_DEV_PARTS,
so nothing in tree can currently desync that IDA through the new generic
API.  The generic helpers, though, forward obj_type/obj_id/group_type/
group_member_id straight to the device with no statement that the caller
owns the id space for a given (group_type, group_member_id, obj_type)
tuple, and no equivalent of the "caller must serialize" note carried by the
dev parts helpers.

Would adding that contract to the kernel-doc make the expectations for new
callers clearer?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,06/14] virtio_net: Query and set flow filter caps

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 3 · Low: 1

- [High] virtnet_ff_init() (drivers/net/virtio_net.c:5760) converts
  device-reported capability data into hard errors (-EINVAL at 5809 when…
- [Medium] In virtnet_ff_init() the bail-out 'if (!ff->ff_actions->count)
  goto err_ff_action;' (drivers/net/virtio_net.c:5852-5853) is taken…
- [Medium] virtnet_ff_init() commits driver capabilities to the device
  with up to three VIRTIO_ADMIN_CMD_DRIVER_CAP_SET commands — RESOURCE…
- [Medium] virtnet_ff_init() issues up to seven virtio admin commands (1x
  cap_id_list_query, 3x cap_get, 3x cap_set) with the global RTNL mutex…
- [Low] The patch's recorded author is Shahar Shitrit, but the Signed-
  off-by chain starts with Daniel Jurgens and there is no Co-developed-…

--

virtio_net: Query and set flow filter caps

This patch queries the virtio_net flow filter resource, selector and action
capabilities with admin commands during probe and resume, and writes the
values back to the device with capability set commands.  It also adds the
new uapi header include/uapi/linux/virtio_net_ff.h.

> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Shahar Shitrit <shshitrit@nvidia.com>

[Severity: Low]
The recorded author of the commit is Shahar Shitrit, but the
Signed-off-by chain begins with Daniel Jurgens and there is no
Co-developed-by: line for him.

Documentation/process/submitting-patches.rst asks that the chain of
custody start with the author, and that a non-author co-author be
recorded with Co-developed-by: immediately followed by their
Signed-off-by:.

Should this carry "Co-developed-by: Daniel Jurgens <danielj@nvidia.com>"
above his Signed-off-by:, or a From: line naming him as author?

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..4f78b881e493b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5724,6 +5739,197 @@ static const struct netdev_stat_ops virtnet_stat_ops = {
>  	.get_base_stats		= virtnet_get_base_stats,
>  };
>  
> +static size_t get_mask_size(u16 type)
> +{
> +	switch (type) {
> +	case VIRTIO_NET_FF_MASK_TYPE_ETH:
> +		return sizeof(struct ethhdr);
> +	case VIRTIO_NET_FF_MASK_TYPE_IPV4:
> +		return sizeof(struct iphdr);
> +	case VIRTIO_NET_FF_MASK_TYPE_IPV6:
> +		return sizeof(struct ipv6hdr);
> +	case VIRTIO_NET_FF_MASK_TYPE_TCP:
> +		return sizeof(struct tcphdr);
> +	case VIRTIO_NET_FF_MASK_TYPE_UDP:
> +		return sizeof(struct udphdr);
> +	}
> +
> +	return 0;
> +}
> +
> +static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
> +{

[ ... ]

> +	if (!le32_to_cpu(ff->ff_caps->groups_limit) ||
> +	    !le32_to_cpu(ff->ff_caps->classifiers_limit) ||
> +	    !le32_to_cpu(ff->ff_caps->rules_limit) ||
> +	    !le32_to_cpu(ff->ff_caps->rules_per_group_limit) ||
> +	    !ff->ff_caps->selectors_per_classifier_limit) {
> +		err = -EINVAL;
> +		goto err_ff;
> +	}

[Severity: High]
Should values reported by the device be fatal to probe here?

virtnet_ff_init() returns -EINVAL whenever the device reports any of
groups_limit, classifiers_limit, rules_limit, rules_per_group_limit or
selectors_per_classifier_limit as zero, and -EPROTO from the selector
walk further down when a known selector type has a length other than
get_mask_size(type) or the reply is truncated.  The three kzalloc()s can
also return -ENOMEM.

Both callers only tolerate 0 and -EOPNOTSUPP:

virtnet_probe()
	err = virtnet_ff_init(&vi->ff, vi->vdev);
	if (err && err != -EOPNOTSUPP) {
		rtnl_unlock();
		goto free_unregister_netdev;
	}

virtnet_restore_up()
	err = virtnet_ff_init(&vi->ff, vi->vdev);
	if (err && err != -EOPNOTSUPP) {
		rtnl_unlock();
		virtnet_freeze_down(vi->vdev);
		remove_vq_common(vi);
		return err;
	}

So a device that advertises the three capability ids but reports zero
limits, or reports a selector mask length that does not match the Linux
header sizes, makes probe fail and unregister the netdev, and on resume
tears down an already restored device.  For a guest whose only interface
is virtio-net that removes the whole data path.

The comment above both call sites says "Not supported is an acceptable and
common return code", which suggests the feature is optional.  Would it be
better to leave ff_supported false and continue, the way an unknown
selector type is already handled with a break out of the loop, instead of
failing probe and resume?

> +
> +	/* VIRTIO_NET_FF_MASK_TYPE start at 1 */
> +	for (i = 1; i <= VIRTIO_NET_FF_MASK_TYPE_MAX; i++)
> +		ff_mask_size += get_mask_size(i);
> +

[ ... ]

> +	ff->ff_actions->count = min_t(u8, ff->ff_actions->count,
> +				      VIRTIO_NET_FF_ACTION_MAX);
> +	if (!ff->ff_actions->count)
> +		goto err_ff_action;

[Severity: Medium]
Is err left at 0 on this branch?

The preceding virtio_admin_cap_get() succeeded, which is what "if (err)
goto err_ff_action;" just established, so err is still 0 when the zero
action count is detected.  The shared unwind path ends in:

	err_ff_action:
		kfree(ff->ff_actions);
		ff->ff_actions = NULL;
	...
		return err;

which returns 0 while ff_supported stays false and ff_caps/ff_mask/
ff_actions are all NULL.  Every other failure branch in the function
assigns -EOPNOTSUPP, -ENOMEM, -EINVAL or -EPROTO, so this looks like a
missing assignment.  The count byte comes straight from the device reply,
so a device reporting zero actions makes virtnet_ff_init() report success
to both callers, which test only the return value.

> +
> +	err = virtio_admin_cap_set(vdev,
> +				   VIRTIO_NET_FF_RESOURCE_CAP,
> +				   ff->ff_caps,
> +				   sizeof(*ff->ff_caps));
> +	if (err)
> +		goto err_ff_action;

[Severity: Medium]
What happens on the device side when the second or third capability set
fails after this one succeeded?

virtnet_ff_init() commits three capabilities in sequence (resource,
selector, action), but the error labels only free the driver side copies:

	err_ff_action:
		kfree(ff->ff_actions);
		ff->ff_actions = NULL;
	err_ff_mask:
		kfree(ff->ff_mask);
		ff->ff_mask = NULL;
	err_ff:
		kfree(ff->ff_caps);
		ff->ff_caps = NULL;

No compensating capability set and no device reset is issued, so the
device keeps the flow filter resource capability committed while the
driver has discarded the limits it agreed to and leaves ff_supported
false.

vp_modern_admin_cmd_exec() turns the device reported admin status
directly into the errno:

drivers/virtio/virtio_pci_modern.c:vp_modern_admin_cmd_exec() {
	...
	ret = -status;
	...
}

so a device answering the selector or action set with status 95 produces
-EOPNOTSUPP, which both call sites deliberately swallow.  Probe and resume
then complete without reaching virtio_reset_device(), leaving the
half negotiated state in place.  Should the unwind path undo the
capabilities that were already accepted?

> +
> +	real_ff_mask_size = sizeof(struct virtio_net_ff_cap_mask_data);
> +	sel = (void *)&ff->ff_mask->selectors;
> +
> +	for (i = 0; i < ff->ff_mask->count; i++) {
> +		/* If the selector type is unknown it may indicate the spec
> +		 * has been revised to include new types of selectors
> +		 */
> +		if (!sel->type || sel->type > VIRTIO_NET_FF_MASK_TYPE_MAX)
> +			break;
> +
> +		if (sel->length != get_mask_size(sel->type) ||
> +		    test_and_set_bit(sel->type, &sel_types)) {
> +			err = -EPROTO;
> +			goto err_ff_action;
> +		}
> +		real_ff_mask_size += sizeof(*sel) + sel->length;
> +		if (real_ff_mask_size > ff_mask_size) {
> +			err = -EPROTO;
> +			goto err_ff_action;
> +		}
> +		sel = (void *)sel + sizeof(*sel) + sel->length;
> +	}
> +	ff->ff_mask->count = i;

[ ... ]

> @@ -5771,9 +5981,24 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>  			goto err_destroy_pools;
>  	}
>  
> +	/*
> +	 * Initialize flow filters. Not supported is an acceptable and common
> +	 * return code
> +	 */
> +	rtnl_lock();
> +	err = virtnet_ff_init(&vi->ff, vi->vdev);
> +	if (err && err != -EOPNOTSUPP) {
> +		rtnl_unlock();
> +		virtnet_freeze_down(vi->vdev);
> +		remove_vq_common(vi);
> +		return err;
> +	}
> +	rtnl_unlock();

[Severity: Medium]
Does this need to run with RTNL held?

virtnet_ff_init() issues up to seven admin commands (one cap id list
query, three cap gets, three cap sets), and each one ends in an
uninterruptible wait with no timeout:

drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	...
again:
	if (virtqueue_is_broken(vq))
		...
	...
	wait_for_completion(&cmd->completion);
	...
}

virtqueue_is_broken() is only checked before the kick, and there is no
interruptible variant or fallback wake after it, so a device that never
completes the command never returns.  This hunk adds a new
rtnl_lock()/rtnl_unlock() section around the call on the resume path, and
virtnet_probe() calls it inside the existing RTNL section, so the global
RTNL mutex is held across those device round trips.  While it is held,
every netdev register/unregister and ip/ethtool operation on all
interfaces blocks, not just this device.

For comparison, the pre-existing control vq path in this driver
(virtnet_send_command) polls with a virtqueue_is_broken() escape.  Is
there something in virtnet_ff_init() that requires RTNL at these two
points?

>  	netif_tx_lock_bh(vi->dev);
>  	netif_device_attach(vi->dev);
>  	netif_tx_unlock_bh(vi->dev);
> +
>  	return 0;
>  
>  err_destroy_pools:

[ ... ]

> @@ -7057,6 +7282,15 @@ static int virtnet_probe(struct virtio_device *vdev)
>  
>  	virtio_device_ready(vdev);
>  
> +	/* Initialize flow filters. Not supported is an acceptable and common
> +	 * return code
> +	 */
> +	err = virtnet_ff_init(&vi->ff, vi->vdev);
> +	if (err && err != -EOPNOTSUPP) {
> +		rtnl_unlock();
> +		goto free_unregister_netdev;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,07/14] virtio_net: Create a FF group for ethtool steering

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] virtnet_ff_cleanup() (drivers/net/virtio_net.c:5945-5962)
  issues the new…

--

virtio_net: Create a FF group for ethtool steering

This adds a single flow filter group object, created during
virtnet_ff_init() with a hard-coded object ID, and destroyed in
virtnet_ff_cleanup().  A new uapi struct and object type define the
group payload.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f78b881e493b..294cfd70662d3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5897,6 +5907,19 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
>  	if (err)
>  		goto err_ff_action;
>  
> +	ethtool_group.group_priority = cpu_to_le16(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);
> +
> +	/* Use priority for the object ID. */
> +	err = virtio_admin_obj_create(vdev,
> +				      VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
> +				      VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
> +				      VIRTIO_ADMIN_GROUP_TYPE_SELF,
> +				      0,
> +				      &ethtool_group,
> +				      sizeof(ethtool_group));
> +	if (err)
> +		goto err_ff_action;
> +
>  	ff->vdev = vdev;
>  	ff->ff_supported = true;
>  
> @@ -5921,9 +5944,18 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
>  
>  static void virtnet_ff_cleanup(struct virtnet_ff *ff)
>  {
> +	int err;
> +
>  	if (!ff->ff_supported)
>  		return;
>  
> +	err = virtio_admin_obj_destroy(ff->vdev,
> +				       VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
> +				       VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
> +				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
> +				       0);
> +	WARN_ON_ONCE(err);
> +
>  	kfree(ff->ff_actions);
>  	kfree(ff->ff_mask);
>  	kfree(ff->ff_caps);

[Severity: Medium]

Is WARN_ON_ONCE() the right reporting mechanism for this error, given that
err here is not a kernel invariant?  Looking at virtio_admin_obj_destroy()
in drivers/virtio/virtio_admin_commands.c, err comes from either an
allocation failure or the device-supplied admin command status:

	data = kzalloc_obj(*data);
	if (!data)
		return -ENOMEM;
	...
	err = vdev->config->admin_cmd_exec(vdev, &cmd);
	kfree(data);

	return err;

virtnet_ff_cleanup() runs from virtnet_remove(), from the virtnet_probe()
error path, and from virtnet_freeze_down() on suspend.  An already reset or
NEEDS_RESET device, a surprise removal, or a misbehaving backend would then
produce a warning splat on an ordinary teardown path, and a guest panic when
panic_on_warn is set.  Would a rate-limited netdev_warn() or dev_warn()
convey the same information here?

The second part of the question is what happens to the device-side object
when the destroy does fail.  The error is discarded and the driver then
unconditionally does:

	kfree(ff->ff_actions);
	kfree(ff->ff_mask);
	kfree(ff->ff_caps);
	ff->ff_supported = false;

so no record of the group object is kept, while the device may still hold
it.  Since virtnet_ff_init() always re-creates the same hard-coded ID:

	err = virtio_admin_obj_create(vdev,
				      VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
				      VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
				      ...);
	if (err)
		goto err_ff_action;

can a group object left behind by a failed destroy make the next create of
that same ID fail, and thereby turn an ignorable teardown error into a
probe or resume failure on re-bind?  There is no retry, no alternate ID, and
no state retained that would allow either.

Note that this depends on whether a device reset already releases admin
resource objects, which is not something that can be determined from the
tree.  Would it be worth documenting that expectation alongside the destroy
call?

This code is unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,09/14] virtio_net: Implement layer 2 ethtool flow rules

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 6 · Low: 2

- [Medium] Installed flow-filter rules are never revalidated when the
  number of active receive virtqueue pairs shrinks.
- [Medium] Rule precedence does not follow the ethtool uAPI contract.
- [Medium] virtio_net_ff_cap_data::rules_per_group_limit ("maximum number
  of rules allowed in a single group") is validated as non-zero in…
- [Medium] All user-installed ethtool flow rules are silently destroyed
  and never restored across a freeze/restore (suspend/resume) cycle.
- [Medium] Failed object-destroy admin commands are treated as success,
  leaving the device with state the driver no longer tracks.
- [Medium] New teardown paths convert device-supplied error codes into
  kernel WARN splats.
- [Low] DISCARDED AS FALSE POSITIVE (retained for policy reasons):
  'virtnet_ethtool_ops adds .set_rxnfc but no .get_rxnfc, so rules…
- [Low] validate_flow_input() returns -EOPNOTSUPP for every fs->location
  other than RX_CLS_LOC_ANY, including unrecognised RX_CLS_LOC_SPECIAL…

--

virtio_net: Implement layer 2 ethtool flow rules

This adds ethtool ntuple rule insertion and removal for ETHER_FLOW by
creating VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER and
VIRTIO_NET_RESOURCE_OBJ_FF_RULE administrative objects, tracked in two
xarrays hanging off struct virtnet_ff.  A few questions below.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 294cfd70662d3..6217c5bcfee9f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5742,6 +5722,445 @@ static const struct netdev_stat_ops virtnet_stat_ops = {
>  	.get_base_stats		= virtnet_get_base_stats,
>  };
>  

[ ... ]

> +static void destroy_classifier(struct virtnet_ff *ff,
> +			       u32 classifier_id)
> +{
> +	struct virtnet_classifier *c;
> +	int err;
> +
> +	c = xa_load(&ff->classifiers, classifier_id);
> +	if (c) {
> +		err = virtio_admin_obj_destroy(ff->vdev,
> +					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
> +					       c->id,
> +					       VIRTIO_ADMIN_GROUP_TYPE_SELF,
> +					       0);
> +		WARN_ON_ONCE(err);

[Severity: Medium]
Is WARN_ON_ONCE() the right reaction to an error that the device chooses?
virtio_admin_obj_destroy() returns whatever vdev->config->admin_cmd_exec()
returns, so a non-zero command status from the device (or -EOPNOTSUPP, or
the -ENOMEM from its own kzalloc) lands directly in this WARN_ON_ONCE().
The same value is turned into a WARN in virtnet_ff_cleanup():

	xa_for_each(&ff->ethtool.rules, i, eth_rule)
		WARN_ON_ONCE(destroy_ethtool_rule(ff, eth_rule));

A device that accepts OBJ_CREATE and then fails OBJ_DESTROY therefore
produces a splat on plain teardown or suspend, and with panic_on_warn=1 that
is a guest panic.  Would a rate-limited netdev_warn() be more appropriate
for device-reported errors here?

> +
> +		xa_erase(&ff->classifiers, c->id);
> +		kfree(c);
> +	}
> +}
> +
> +static int destroy_ethtool_rule(struct virtnet_ff *ff,
> +				struct virtnet_ethtool_rule *eth_rule)
> +{
> +	int err;
> +
> +	ff->ethtool.num_rules--;
> +
> +	err = virtio_admin_obj_destroy(ff->vdev,
> +				       VIRTIO_NET_RESOURCE_OBJ_FF_RULE,
> +				       eth_rule->flow_spec.location,
> +				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
> +				       0);
> +
> +	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
> +	destroy_classifier(ff, eth_rule->classifier_id);
> +	kfree(eth_rule);
> +
> +	return err;
> +}

[Severity: Medium]
Can the driver and the device diverge here when the destroy fails?  err is
saved but the xa_erase(), destroy_classifier() and kfree(eth_rule) run
unconditionally, so on failure:

  - the device keeps an armed FF_RULE that the driver can no longer list or
    delete, and virtnet_ff_cleanup() will not retry it because the xarray
    entry is gone
  - destroy_classifier() then tears down the classifier that the still-live
    rule references
  - the location id and the classifier id go back to their xa_alloc
    allocators, so a later ethtool -U can pick an id the device still has
    allocated and the following OBJ_CREATE collides with the stale object

virtnet_ethtool_flow_remove() returns that err to userspace after the record
is already gone, so the user sees a failure but has nothing left to retry
against.

Note this does not need a malicious device: virtio_admin_obj_destroy() can
fail before the command is sent, since its own kzalloc() can return -ENOMEM.

Would it be better to keep the rule record and its classifier when the
device refuses the destroy?  The same applies to the rollback in
build_and_insert(), which uses destroy_classifier() and so cannot tell that
the classifier is still allocated in the device.

> +
> +static int insert_rule(struct virtnet_ff *ff,
> +		       struct virtnet_ethtool_rule *eth_rule,
> +		       u32 classifier_id,
> +		       const u8 *key,
> +		       u8 key_size)
> +{
> +	struct ethtool_rx_flow_spec *fs = &eth_rule->flow_spec;
> +	struct virtio_net_resource_obj_ff_rule *ff_rule;
> +	int err;
> +
> +	ff_rule = kzalloc(sizeof(*ff_rule) + key_size, GFP_KERNEL);
> +	if (!ff_rule)
> +		return -ENOMEM;
> +
> +	/* Intentionally leave the priority as 0. All rules have the same
> +	 * priority.
> +	 */

[Severity: Medium]
With every rule created at the same device priority, what decides which of
two overlapping rules wins?  include/uapi/linux/ethtool.h documents
struct ethtool_rx_flow_spec::location as ordering-significant:

 * @location: Location of rule in the table.  Locations must be
 *	numbered such that a flow matching multiple rules will be
 *	classified according to the first (lowest numbered) rule.

Since rule_priority stays 0 for all rules, and last_rule_priority from
struct virtio_net_ff_cap_data is never referenced in the driver, two
overlapping rules resolve in a device-defined order rather than in location
order.  For ETHER_FLOW alone one rule masking h_dest and another masking
h_source already match the same frame.  Should the location ordering be
mapped onto rule_priority, or should the documented ordering guarantee be
called out as unsupported?

> +	ff_rule->group_id = cpu_to_le32(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);
> +	ff_rule->classifier_id = cpu_to_le32(classifier_id);
> +	ff_rule->key_length = key_size;
> +	ff_rule->action = fs->ring_cookie == RX_CLS_FLOW_DISC ?
> +					     VIRTIO_NET_FF_ACTION_DROP :
> +					     VIRTIO_NET_FF_ACTION_RX_VQ;
> +	ff_rule->vq_index = fs->ring_cookie != RX_CLS_FLOW_DISC ?
> +					       cpu_to_le16(rxq2vq(fs->ring_cookie)) : 0;
> +	memcpy(&ff_rule->keys, key, key_size);
> +

[ ... ]

> +static int validate_flow_input(struct virtnet_ff *ff,
> +			       const struct ethtool_rx_flow_spec *fs,
> +			       u16 curr_queue_pairs)
> +{
> +	u8 required_action = fs->ring_cookie == RX_CLS_FLOW_DISC ?
> +			     VIRTIO_NET_FF_ACTION_DROP :
> +			     VIRTIO_NET_FF_ACTION_RX_VQ;
> +	int i;
> +
> +	/* Force users to use RX_CLS_LOC_ANY - don't allow specific locations */
> +	if (fs->location != RX_CLS_LOC_ANY)
> +		return -EOPNOTSUPP;

[Severity: Low]
Should an unrecognised RX_CLS_LOC_SPECIAL value return -EINVAL rather than
-EOPNOTSUPP?  The struct ethtool_rxnfc documentation in
include/uapi/linux/ethtool.h says:

 * defined in future and drivers must return -%EINVAL for any
 * unrecognised value.

This blanket check returns -EOPNOTSUPP for every location other than
RX_CLS_LOC_ANY, including those unrecognised special values.

> +
> +	if (fs->ring_cookie != RX_CLS_FLOW_DISC &&
> +	    fs->ring_cookie >= curr_queue_pairs)
> +		return -EINVAL;

[Severity: Medium]
This is the only bound on the target queue, and insert_rule() then commits
an absolute receive virtqueue index into the device object:

	ff_rule->vq_index = fs->ring_cookie != RX_CLS_FLOW_DISC ?
					       cpu_to_le16(rxq2vq(fs->ring_cookie)) : 0;

What happens to that rule when curr_queue_pairs later shrinks?  The
ethtool -L path is covered once .get_rxnfc exists later in the series, since
ethtool_check_max_channel() then refuses reductions below the highest in-use
ring, but virtnet_xdp_set() lowers the count without any such check:

virtnet_xdp_set()
	curr_qp = vi->curr_queue_pairs - vi->xdp_queue_pairs;
	...
	err = virtnet_set_queues(vi, curr_qp + xdp_qp);

So a rule inserted while an XDP program is attached, with ring_cookie in
[curr_qp, curr_qp + xdp_qp), survives program detach and then names a
receiveq the driver has just told the device to stop using via
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET.  Does matched traffic get silently dropped
while the rule is still listed as valid?

> +
> +	for (i = 0; i < ff->ff_actions->count; i++)
> +		if (ff->ff_actions->actions[i] == required_action)
> +			goto action_ok;
> +	return -EOPNOTSUPP;
> +

[ ... ]

> +static int virtnet_ethtool_flow_insert(struct virtnet_ff *ff,
> +				       struct ethtool_rx_flow_spec *fs,
> +				       u16 curr_queue_pairs)
> +{
> +	struct virtnet_ethtool_rule *eth_rule;
> +	int err;
> +
> +	if (!ff->ff_supported)
> +		return -EOPNOTSUPP;
> +
> +	err = validate_flow_input(ff, fs, curr_queue_pairs);
> +	if (err)
> +		return err;
> +
> +	eth_rule = kzalloc(sizeof(*eth_rule), GFP_KERNEL);
> +	if (!eth_rule)
> +		return -ENOMEM;
> +
> +	err = xa_alloc(&ff->ethtool.rules, &fs->location, eth_rule,
> +		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->rules_limit) - 1),
> +		       GFP_KERNEL);

[Severity: Medium]
Where is rules_per_group_limit enforced?  Every ethtool rule is created in
the single ethtool group:

	ff_rule->group_id = cpu_to_le32(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);

but the id allocation here is bounded only by rules_limit.  On a device that
reports rules_per_group_limit smaller than rules_limit, repeated ethtool -U
insertions keep allocating locations and issuing OBJ_CREATE past the
per-group limit, and the failure then comes back from the device as an opaque
error instead of -ENOSPC.

Later in the series this also contradicts what the driver advertises, since
virtnet_ethtool_get_flow_count() reports the table size as
min(rules_limit, rules_per_group_limit) | RX_CLS_LOC_SPECIAL while this path
can still hand out higher locations.

> +	if (err)
> +		goto err_rule;
> +
> +	eth_rule->flow_spec = *fs;
> +

[ ... ]

> @@ -5944,11 +6409,19 @@ static int virtnet_ff_init(struct virtnet_ff *ff, struct virtio_device *vdev)
>  
>  static void virtnet_ff_cleanup(struct virtnet_ff *ff)
>  {
> +	struct virtnet_ethtool_rule *eth_rule;
> +	unsigned long i;
>  	int err;
>  
>  	if (!ff->ff_supported)
>  		return;
>  
> +	xa_for_each(&ff->ethtool.rules, i, eth_rule)
> +		WARN_ON_ONCE(destroy_ethtool_rule(ff, eth_rule));
> +
> +	xa_destroy(&ff->ethtool.rules);
> +	xa_destroy(&ff->classifiers);

[Severity: Medium]
Are the installed rules meant to survive a freeze/restore cycle?  This
teardown is reached from the suspend path:

virtnet_freeze_down()
	rtnl_lock();
	virtnet_ff_cleanup(&vi->ff);
	rtnl_unlock();

and destroy_ethtool_rule() kfree()s the only software copy of each
flow_spec, after which both xarrays are destroyed.  On the way back up the
only flow filter work is:

virtnet_restore_up()
	err = virtnet_ff_init(&vi->ff, vi->vdev);

which ends in xa_init_flags() on two empty xarrays, with nothing replaying
the classifier and rule creates.

After resume, does userspace keep believing rules exist that the device no
longer enforces?  A rule with RX_CLS_FLOW_DISC would start delivering
traffic again, and ETHTOOL_SRXCLSRLDEL for a previously reported location
takes the !eth_rule path in virtnet_ethtool_flow_remove() and returns
-ENOENT.  Before this patch only the group object was recreated, so no
user-visible configuration was lost.

> +
>  	err = virtio_admin_obj_destroy(ff->vdev,
>  				       VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
>  				       VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,10/14] virtio_net: Use existing classifier if possible

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] `try_destroy_classifier()`
  (drivers/net/virtio_net.c:5871-5888) treats a failed…

--

virtio_net: Use existing classifier if possible

This adds a refcount to struct virtnet_classifier and makes
setup_classifier() scan ff->classifiers for an identical classifier
object, reusing it instead of creating a duplicate device object.  The
release side becomes try_destroy_classifier(), which only destroys the
device object when the last reference goes away.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6217c5bcfee9f..6c07eab047afe 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5823,11 +5825,24 @@ static bool validate_mask(const struct virtnet_ff *ff,
>  	return false;
>  }
>  
> -static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
> +static int setup_classifier(struct virtnet_ff *ff,
> +			    struct virtnet_classifier **c)
>  {
> +	struct virtnet_classifier *tmp;
> +	unsigned long i;
>  	int err;
>  
> -	err = xa_alloc(&ff->classifiers, &c->id, c,
> +	xa_for_each(&ff->classifiers, i, tmp) {
> +		if ((*c)->size == tmp->size &&
> +		    !memcmp(&tmp->obj, &(*c)->obj, tmp->size)) {
> +			refcount_inc(&tmp->refcount);
> +			kfree(*c);
> +			*c = tmp;
> +			goto out;
> +		}
> +	}
> +
> +	err = xa_alloc(&ff->classifiers, &(*c)->id, *c,
>  		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->classifiers_limit) - 1),
>  		       GFP_KERNEL);
>  	if (err)

[ ... ]

> @@ -5835,30 +5850,31 @@ static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
>  
> -static void destroy_classifier(struct virtnet_ff *ff,
> -			       u32 classifier_id)
> +static void try_destroy_classifier(struct virtnet_ff *ff, u32 classifier_id)
>  {
>  	struct virtnet_classifier *c;
>  	int err;
>  
>  	c = xa_load(&ff->classifiers, classifier_id);
> -	if (c) {
> +	if (c && refcount_dec_and_test(&c->refcount)) {
>  		err = virtio_admin_obj_destroy(ff->vdev,
>  					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
>  					       c->id,

[Severity: Medium]
With this patch ff->classifiers becomes the authoritative mirror of the
classifier objects that exist in the device, since setup_classifier() now
scans it with memcmp() to decide whether an equivalent device object is
already present.

Given that, is the unconditional cleanup in the tail of
try_destroy_classifier() still correct?

drivers/net/virtio_net.c:try_destroy_classifier() {
	c = xa_load(&ff->classifiers, classifier_id);
	if (c && refcount_dec_and_test(&c->refcount)) {
		err = virtio_admin_obj_destroy(ff->vdev,
					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
					       c->id,
					       VIRTIO_ADMIN_GROUP_TYPE_SELF,
					       0);
		WARN_ON_ONCE(err);

		xa_erase(&ff->classifiers, c->id);
		kfree(c);
	}
}

virtio_admin_obj_destroy() is synchronous and returns non-zero when the
object was not removed, and it can fail without any device malfunction:

drivers/virtio/virtio_admin_commands.c:virtio_admin_obj_destroy() {
	if (!vdev->config->admin_cmd_exec)
		return -EOPNOTSUPP;

	data = kzalloc_obj(*data);
	if (!data)
		return -ENOMEM;
	...
	err = vdev->config->admin_cmd_exec(vdev, &cmd);

After such a failure the device still holds the classifier object while the
driver has erased and freed its tracking entry.  Does this leak one of the
device's ff_caps->classifiers_limit slots permanently?

Two follow-on effects also look reachable from that state:

- the id is immediately available to xa_alloc() again, so a later
  virtio_admin_obj_create() for that id can be rejected by the device and
  userspace sees an ETHTOOL_SRXCLSRLINS failure the driver cannot explain.

- an identical classifier requested afterwards no longer matches anything in
  the xarray, so a second device object is created for it, which is the
  duplicate-classifier case the commit message says this change avoids.

Would it be better to leave the entry in ff->classifiers (and restore the
refcount) when virtio_admin_obj_destroy() reports an error?

Separately, in destroy_ethtool_rule() the FF_RULE destroy result is saved but
not acted on before the classifier reference is dropped:

drivers/net/virtio_net.c:destroy_ethtool_rule() {
	err = virtio_admin_obj_destroy(ff->vdev,
				       VIRTIO_NET_RESOURCE_OBJ_FF_RULE,
				       eth_rule->flow_spec.location,
				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
				       0);

	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
	try_destroy_classifier(ff, eth_rule->classifier_id);
	kfree(eth_rule);

	return err;
}

If the rule destroy fails, can the now shared classifier be destroyed on the
device while a device-side rule still references its id?

>  @@ -5885,7 +5901,7 @@ static int destroy_ethtool_rule(struct virtnet_ff *ff,
>  				       0);
>  
>  	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
> -	destroy_classifier(ff, eth_rule->classifier_id);
> +	try_destroy_classifier(ff, eth_rule->classifier_id);
>  	kfree(eth_rule);
>  
>  	return err;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,11/14] virtio_net: Implement IPv4 ethtool flow rules

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] IP_USER_FLOW support silently ignores the ethtool `proto`
  match field.
- [Low] While adding the `int num_hdrs` parameter, the patch also de-
  indents the two pre-existing continuation lines of…

--

virtio_net: Implement IPv4 ethtool flow rules

This adds support for ethtool IP_USER_FLOW rules to the virtio_net flow
filter code, building an extra IPv4 selector after the ethernet selector
and matching on source/destination address and tos. The ethernet selector
is forced to match ethertype IPv4 whenever an IPv4 header follows.

(No Link: tags in the commit header.)

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6c07eab047afe..c33bbdda75590 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5809,6 +5810,39 @@ static bool validate_eth_mask(const struct virtnet_ff *ff,
>  	return true;
>  }
>  
> +static bool validate_ip4_mask(const struct virtnet_ff *ff,
> +			      const struct virtio_net_ff_selector *sel,
> +			      const struct virtio_net_ff_selector *sel_cap)
> +{

[ ... ]

> +	if (mask->protocol &&
> +	    !check_mask_vs_cap(&mask->protocol, &cap->protocol,
> +			       sizeof(u8), partial_mask))
> +		return false;
> +

[ ... ]

> @@ -5820,11 +5854,41 @@ static bool validate_mask(const struct virtnet_ff *ff,

[ ... ]

> +static void parse_ip4(struct iphdr *mask, struct iphdr *key,
> +		      const struct ethtool_rx_flow_spec *fs)
> +{
> +	const struct ethtool_usrip4_spec *l3_mask = &fs->m_u.usr_ip4_spec;
> +	const struct ethtool_usrip4_spec *l3_val  = &fs->h_u.usr_ip4_spec;
> +
> +	if (l3_mask->ip4src) {
> +		put_unaligned(l3_mask->ip4src, &mask->saddr);
> +		put_unaligned(l3_val->ip4src, &key->saddr);
> +	}
> +
> +	if (l3_mask->ip4dst) {
> +		put_unaligned(l3_mask->ip4dst, &mask->daddr);
> +		put_unaligned(l3_val->ip4dst, &key->daddr);
> +	}
> +
> +	if (l3_mask->tos) {
> +		mask->tos = l3_mask->tos;
> +		key->tos = l3_val->tos;
> +	}
> +}

[ ... ]

> @@ -6016,8 +6091,9 @@ static void calculate_flow_sizes(struct ethtool_rx_flow_spec *fs,
>  }
>  
>  static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,
> -				   u8 *key,
> -				   const struct ethtool_rx_flow_spec *fs)
> +				  u8 *key,
> +				  const struct ethtool_rx_flow_spec *fs,
> +				  int num_hdrs)

[Severity: Low]
This isn't a bug, but the two pre-existing continuation lines lost one
space of indentation here while the new num_hdrs parameter was added.
"static void setup_eth_hdr_key_mask(" puts the open parenthesis one column
further right, so the continuation lines no longer line up with it and
checkpatch reports "Alignment should match open parenthesis". Was the
re-indent intentional? Only the "int num_hdrs" line seems to need
touching. The same alignment persists through the end of the series.

>  {
>  	struct ethhdr *eth_m = (struct ethhdr *)&selector->mask;
>  	struct ethhdr *eth_k = (struct ethhdr *)key;
> @@ -6025,8 +6101,35 @@ static void setup_eth_hdr_key_mask(struct virtio_net_ff_selector *selector,

[ ... ]

> +static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
> +			     u8 *key,
> +			     const struct ethtool_rx_flow_spec *fs)
> +{
> +	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
> +	struct iphdr *v4_k = (struct iphdr *)key;
> +
> +	selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> +	selector->length = sizeof(struct iphdr);
> +
> +	if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
> +	    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
> +	    fs->m_u.usr_ip4_spec.l4_4_bytes ||
> +	    fs->m_u.usr_ip4_spec.ip_ver ||
> +	    fs->m_u.usr_ip4_spec.proto)
> +		return -EINVAL;
> +
> +	parse_ip4(v4_m, v4_k, fs);
> +
> +	return 0;
>  }

[Severity: Medium]
What happens to h_u.usr_ip4_spec.proto here? Every other unsupported
IPv4 field is rejected, but a non-zero h_u.usr_ip4_spec.proto is neither
rejected nor programmed.

include/uapi/linux/ethtool.h documents the field for struct
ethtool_usrip4_spec as:

 * @proto: Transport protocol number; mask must be 0

So a conforming userspace passes the protocol in h_u with a zero mask in
m_u, which is exactly the combination that passes all five checks above.
parse_ip4() then only copies ip4src, ip4dst and tos, leaving
key->protocol and mask->protocol zero in the zeroed key/classifier
buffers.

Does this mean

  ethtool -U ethX flow-type ip4 src-ip 192.168.51.101 l4proto 6 action -1

installs a rule that matches ethertype IPv4 plus src-ip only, and
therefore drops every protocol from that address rather than just TCP,
without returning an error? Would it be better to either program
iphdr->protocol from h_u.usr_ip4_spec.proto, or return -EINVAL when it is
non-zero?

There also seems to be a disagreement between the two new functions: the
protocol check in validate_ip4_mask()

	if (mask->protocol &&
	    !check_mask_vs_cap(&mask->protocol, &cap->protocol,
			       sizeof(u8), partial_mask))
		return false;

looks unreachable for IP_USER_FLOW, since a non-zero
m_u.usr_ip4_spec.proto always returns -EINVAL in setup_ip_key_mask()
first and parse_ip4() never sets mask->protocol. Is that intended?

Checking the rest of the series, at 04f0a1ab the behaviour looks
unchanged: setup_ip_key_mask() still returns -EINVAL for
m_u.usr_ip4_spec.proto with num_hdrs == 2, parse_ip4() still never writes
iphdr->protocol, and the protocol key is only filled in for
TCP_V4_FLOW/UDP_V4_FLOW.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,12/14] virtio_net: Add support for IPv6 ethtool steering

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] The new IPv6 code overlays `struct ipv6hdr` (alignment 4, from
  the embedded struct in6_addr / __be32 s6_addr32[]) on byte-packed…
- [Low] Commit message claims "Implement support for IPV6_USER_FLOW type
  rules" but the implementation only supports src-ip/dst-ip matching:…

--

virtio_net: Add support for IPv6 ethtool steering

This adds IPV6_USER_FLOW support to the virtio_net ethtool flow filter
code: a new validate_ip6_mask() for the IPv6 selector mask, a new
parse_ip6() to build the key/mask pair, and IPv6 handling in
setup_ip_key_mask(), supported_flow_type() and calculate_flow_sizes().

Two questions below, one about alignment of the new IPv6 accesses and one
about the scope described in the changelog.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index c33bbdda75590..bfe8f0bc2581a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5843,6 +5843,40 @@ static bool validate_ip4_mask(const struct virtnet_ff *ff,
>  	return true;
>  }
>  
> +static bool validate_ip6_mask(const struct virtnet_ff *ff,
> +			      const struct virtio_net_ff_selector *sel,
> +			      const struct virtio_net_ff_selector *sel_cap)
> +{
> +	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
> +	struct in6_addr tmp;
> +	struct ipv6hdr *cap, *mask;
> +
> +	cap = (struct ipv6hdr *)&sel_cap->mask;
> +	mask = (struct ipv6hdr *)&sel->mask;
> +
> +	/* mask->saddr/daddr may be unaligned; copy to aligned tmp for
> +	 * ipv6_addr_any().
> +	 */
> +	memcpy(&tmp, &mask->saddr, sizeof(tmp));
> +	if (!ipv6_addr_any(&tmp) &&
> +	    !check_mask_vs_cap(&mask->saddr, &cap->saddr,
> +			       sizeof(cap->saddr), partial_mask))
> +		return false;
> +
> +	memcpy(&tmp, &mask->daddr, sizeof(tmp));
> +	if (!ipv6_addr_any(&tmp) &&
> +	    !check_mask_vs_cap(&mask->daddr, &cap->daddr,
> +			       sizeof(cap->daddr), partial_mask))
> +		return false;
> +
> +	if (mask->nexthdr &&
> +	    !check_mask_vs_cap(&mask->nexthdr, &cap->nexthdr,
> +			       sizeof(cap->nexthdr), partial_mask))
> +		return false;
> +
> +	return true;
> +}
> +

[ ... ]

> @@ -5884,11 +5921,33 @@ static void parse_ip4(struct iphdr *mask, struct iphdr *key,
>  	}
>  }
>  
> +static void parse_ip6(struct ipv6hdr *mask, struct ipv6hdr *key,
> +		      const struct ethtool_rx_flow_spec *fs)
> +{
> +	const struct ethtool_usrip6_spec *l3_mask = &fs->m_u.usr_ip6_spec;
> +	const struct ethtool_usrip6_spec *l3_val  = &fs->h_u.usr_ip6_spec;
> +
> +	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6src)) {
> +		memcpy(&mask->saddr, l3_mask->ip6src, sizeof(mask->saddr));
> +		memcpy(&key->saddr, l3_val->ip6src, sizeof(key->saddr));
> +	}
> +
> +	if (!ipv6_addr_any((struct in6_addr *)l3_mask->ip6dst)) {
> +		memcpy(&mask->daddr, l3_mask->ip6dst, sizeof(mask->daddr));
> +		memcpy(&key->daddr, l3_val->ip6dst, sizeof(key->daddr));
> +	}
> +}

[Severity: Medium]
Are the destinations of these copies guaranteed to be 4-byte aligned?

struct in6_addr contains __be32 s6_addr32[4], so struct ipv6hdr has
4-byte alignment, but the buffers these pointers overlay are byte packed
in the virtio flow filter layout:

  build_and_insert() -> setup_ip_key_mask(next_selector(selector),
                                          key + sizeof(struct ethhdr), fs)

For the classifier mask, the first selector is an 8-byte
struct virtio_net_ff_selector header plus a 14-byte struct ethhdr mask,
so the second selector starts at obj+30 and its mask[] at obj+38, which
makes &v6_m->saddr land at obj+46, i.e. 2 mod 4. The key buffer is the
same: it is offset by sizeof(struct ethhdr), so &v6_k->saddr is
base+22, again 2 mod 4.

The IPv4 sibling in the same file goes through the unaligned helpers for
exactly these members:

drivers/net/virtio_net.c:parse_ip4() {
	if (l3_mask->ip4src) {
		put_unaligned(l3_mask->ip4src, &mask->saddr);
		put_unaligned(l3_val->ip4src, &key->saddr);
	}
	...
}

and validate_ip4_mask() uses get_unaligned(&mask->saddr) for the read
side. Since the pointer expressions here are typed struct in6_addr *,
can the compiler expand these 16-byte copies into word or multi-word
accesses (for example LDRD/STRD or LDM/STM on 32-bit ARM) on an address
that is 2 mod 4, and fault on strict-alignment architectures?

The comment in validate_ip6_mask() already notes "mask->saddr/daddr may
be unaligned", but the aligned tmp only fixes the destination of that one
copy; the source (&mask->saddr) and the copies in parse_ip6() are still
misaligned accesses through a 4-byte-aligned pointer type. Would using
the unaligned accessors, or plain u8 pointers, for these fields keep the
IPv6 path consistent with the IPv4 one?

[ ... ]

> @@ -6114,20 +6179,38 @@ static int setup_ip_key_mask(struct virtio_net_ff_selector *selector,
>  			     u8 *key,
>  			     const struct ethtool_rx_flow_spec *fs)
>  {
> +	struct ipv6hdr *v6_m = (struct ipv6hdr *)&selector->mask;
>  	struct iphdr *v4_m = (struct iphdr *)&selector->mask;
> +	struct ipv6hdr *v6_k = (struct ipv6hdr *)key;
>  	struct iphdr *v4_k = (struct iphdr *)key;
>  
> -	selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> -	selector->length = sizeof(struct iphdr);
> +	if (has_ipv6(fs->flow_type)) {
> +		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV6;
> +		selector->length = sizeof(struct ipv6hdr);
> +
> +		/* exclude tclass, it's not exposed directly in struct ipv6hdr */
> +		if (fs->h_u.usr_ip6_spec.l4_4_bytes ||
> +		    fs->m_u.usr_ip6_spec.l4_4_bytes ||
> +		    fs->h_u.usr_ip6_spec.tclass ||
> +		    fs->m_u.usr_ip6_spec.tclass ||
> +		    fs->h_u.usr_ip6_spec.l4_proto ||
> +		    fs->m_u.usr_ip6_spec.l4_proto)
> +			return -EINVAL;
>  
> -	if (fs->h_u.usr_ip4_spec.l4_4_bytes ||
> -	    fs->h_u.usr_ip4_spec.ip_ver != ETH_RX_NFC_IP4 ||
> -	    fs->m_u.usr_ip4_spec.l4_4_bytes ||
> -	    fs->m_u.usr_ip4_spec.ip_ver ||
> -	    fs->m_u.usr_ip4_spec.proto)
> -		return -EINVAL;
> +		parse_ip6(v6_m, v6_k, fs);

[Severity: Low]
This isn't a bug, but should the changelog say which ip6 match fields are
actually implemented?

The commit message says:

    Implement support for IPV6_USER_FLOW type rules.

but setup_ip_key_mask() rejects any non-zero tclass, l4_proto or
l4_4_bytes, in the value as well as in the mask, and parse_ip6() only
ever fills in saddr/daddr. So of the struct ethtool_usrip6_spec fields
only the addresses work, while supported_flow_type() advertises
IPV6_USER_FLOW as a whole:

	case IPV6_USER_FLOW:
		return true;

A request such as:

    ethtool -U ens9 flow-type ip6 src-ip fe80::2 tclass 8 action 3

fails with "Invalid argument" and there is nothing in the log describing
the limitation.

There is also an asymmetry with the IPv4 support added in the previous
patch: parse_ip4() does translate the IPv4 tos field:

	if (l3_mask->tos) {
		mask->tos = l3_mask->tos;
		key->tos = l3_val->tos;
	}

while the IPv6 path refuses the equivalent tclass, even though
struct ipv6hdr carries the traffic class in the priority/flow_lbl bits.
Could the changelog mention that only src-ip/dst-ip matching is
supported for ip6 flows, and why tclass parity with the IPv4 path is not
provided?

> +	} else {
> +		selector->type = VIRTIO_NET_FF_MASK_TYPE_IPV4;
> +		selector->length = sizeof(struct iphdr);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,14/14] virtio_net: Add get ethtool flow rules ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The two new getters report the UAPI-documented "size of the
  rule table" (struct ethtool_rxnfc::data) with two contradictory values.

--

virtio_net: Add get ethtool flow rules ops

This adds a get_rxnfc ethtool op to virtio_net handling
ETHTOOL_GRXCLSRLCNT, ETHTOOL_GRXCLSRULE and ETHTOOL_GRXCLSRLALL, so
userspace can query the rule count, a single rule, and the list of rule
locations.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a1e042f356f5e..911a5142605bd 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6575,6 +6575,60 @@ static int virtnet_ethtool_flow_remove(struct virtnet_ff *ff, int location)
>  	return err;
>  }
>  
> +static int virtnet_ethtool_get_flow_count(struct virtnet_ff *ff,
> +					  struct ethtool_rxnfc *info)
> +{
> +	if (!ff->ff_supported)
> +		return -EOPNOTSUPP;
> +
> +	info->rule_cnt = ff->ethtool.num_rules;
> +	info->data = min(le32_to_cpu(ff->ff_caps->rules_limit),
> +			 le32_to_cpu(ff->ff_caps->rules_per_group_limit)) |
> +		     RX_CLS_LOC_SPECIAL;
> +
> +	return 0;
> +}
> +

[ ... ]

> +static int
> +virtnet_ethtool_get_all_flows(struct virtnet_ff *ff,
> +			      struct ethtool_rxnfc *info, u32 *rule_locs)
> +{
> +	struct virtnet_ethtool_rule *eth_rule;
> +	unsigned long i = 0;
> +	int idx = 0;
> +
> +	if (!ff->ff_supported)
> +		return -EOPNOTSUPP;
> +
> +	xa_for_each(&ff->ethtool.rules, i, eth_rule) {
> +		if (idx == info->rule_cnt)
> +			return -EMSGSIZE;
> +		rule_locs[idx++] = i;
> +	}
> +
> +	info->data = le32_to_cpu(ff->ff_caps->rules_limit);

[Severity: Medium]
Should these two handlers report the same value in info->data?

include/uapi/linux/ethtool.h documents the field the same way for both
commands:

 * For %ETHTOOL_GRXCLSRLCNT, ... If @data is non-zero on return then it is the
 * size of the rule table, plus the flag %RX_CLS_LOC_SPECIAL ...
 * For %ETHTOOL_GRXCLSRLALL, ... On return, @data is the size of the rule table

virtnet_ethtool_get_flow_count() reports:

	info->data = min(le32_to_cpu(ff->ff_caps->rules_limit),
			 le32_to_cpu(ff->ff_caps->rules_per_group_limit)) |
		     RX_CLS_LOC_SPECIAL;

while virtnet_ethtool_get_all_flows() reports the unclamped
ff_caps->rules_limit.  On a device that advertises
rules_per_group_limit < rules_limit these two answers differ for the same
table.

Since every ethtool rule goes into one group (VIRTNET_FF_MAX_GROUPS is 1
and insert_rule() hardcodes the group), isn't the usable capacity
min(rules_limit, rules_per_group_limit)?

	ff_rule->group_id = cpu_to_le32(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);

If so, does ETHTOOL_GRXCLSRLALL overstate the table size here, so that
inserts past rules_per_group_limit get refused by the device?

There is a related question about the location space itself.
virtnet_ethtool_flow_insert() allocates locations from the unclamped
limit:

	err = xa_alloc(&ff->ethtool.rules, &fs->location, eth_rule,
		       XA_LIMIT(0, le32_to_cpu(ff->ff_caps->rules_limit) - 1),
		       GFP_KERNEL);

so locations handed back in fs.location, and then reported in
rule_locs[] by virtnet_ethtool_get_all_flows(), can exceed the table size
now advertised by ETHTOOL_GRXCLSRLCNT.  Should all three sites use the
same limit?

> +	info->rule_cnt = idx;
> +
> +	return 0;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com

Re: [net-next,v23,02/14] virtio_pci: Fix sleeping under spinlock in admin command path

[Paolo Abeni]

On 9/2/26 3:12 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 (2) · Medium: 2 · Low: 1
> 
> - [High] The patch adds a new `vp_dev->admin_vq.info = NULL` store in
>   vp_del_vq() (drivers/virtio/virtio_pci_common.c:261) but does not…

At least this one looks real and worth addressing to me.

/P