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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Aug 31, 2026 at 10:57:53AM +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:

A small comment on patch 5, but it's minor. Besides that looks good!


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