[PATCH net-next v21 00/13] virtio_net: Add ethtool flow rules support
Shahar Shitrit <[email protected]> Mon, 3 Aug 2026 17:07:08 +0300
| Newsgroups | dev.linux.lists.virtualization,org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
This is v21 of the series previously posted by Daniel Jurgens.
I'll be taking over from this version onward.
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/[email protected]/
- Fix sparse warnings
- Fix memory leak on subsequent failure to allocate
- Fix some Typos
v3: https://lore.kernel.org/netdev/[email protected]/
- 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.
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.
Daniel Jurgens (11):
virtio_pci: Remove supported_cap size build assert
virtio: Add config_op for admin commands
virtio: Expose generic device capability operations
virtio: Expose object create and destroy API
virtio_net: Create a FF group for ethtool steering
virtio_net: Implement layer 2 ethtool flow rules
virtio_net: Use existing classifier if possible
virtio_net: Implement IPv4 ethtool flow rules
virtio_net: Add support for IPv6 ethtool steering
virtio_net: Add support for TCP and UDP ethtool rules
virtio_net: Add get ethtool flow rules ops
Shahar Shitrit (2):
virtio_net: Query and set flow filter caps
ethtool: Introduce ethtool_flow_type_mask()
.../mellanox/mlx5/core/en_fs_ethtool.c | 17 +-
.../mellanox/mlx5/core/ipoib/ethtool.c | 7 +-
drivers/net/virtio_net.c | 1557 +++++++++++++++--
drivers/virtio/Makefile | 2 +-
drivers/virtio/virtio_admin_commands.c | 173 ++
drivers/virtio/virtio_pci_common.h | 1 -
drivers/virtio/virtio_pci_modern.c | 12 +-
include/linux/ethtool.h | 6 +
include/linux/virtio_admin.h | 124 ++
include/linux/virtio_config.h | 6 +
include/uapi/linux/virtio_net_ff.h | 156 ++
include/uapi/linux/virtio_pci.h | 6 +-
12 files changed, 1871 insertions(+), 196 deletions(-)
create mode 100644 drivers/virtio/virtio_admin_commands.c
create mode 100644 include/linux/virtio_admin.h
create mode 100644 include/uapi/linux/virtio_net_ff.h
--
2.49.0