netdev - Re: [PATCH net-next v11 07/12] virtio_net: Implement layer 2 ethtool flow rules

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <50af344e-9a3b-4e07-966f-d44784ee0752@nvidia.com>
Date: Wed, 19 Nov 2025 10:51:14 -0600
From: Dan Jurgens <danielj@...dia.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, jasowang@...hat.com, pabeni@...hat.com,
 virtualization@...ts.linux.dev, parav@...dia.com, shshitrit@...dia.com,
 yohadt@...dia.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
 jgg@...pe.ca, kevin.tian@...el.com, kuba@...nel.org, andrew+netdev@...n.ch,
 edumazet@...gle.com
Subject: Re: [PATCH net-next v11 07/12] virtio_net: Implement layer 2 ethtool
 flow rules

On 11/19/25 3:26 AM, Michael S. Tsirkin wrote:
> On Tue, Nov 18, 2025 at 08:38:57AM -0600, Daniel Jurgens wrote:
> 
> ...
> 
>> +static int insert_rule(struct virtnet_ff *ff,
>> +		       struct virtnet_ethtool_rule *eth_rule,
>> +		       u32 classifier_id,
>> +		       const u8 *key,
>> +		       size_t 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 = (u8)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(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);
>> +
>> +	return 0;
>> +
>> +err_ff_rule:
>> +	kfree(ff_rule);
>> +
>> +	return err;
>> +}
> 
> 
> ...
> 
>> +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;
>> +	size_t key_size;
>> +	int num_hdrs;
>> +	u8 *key;
>> +	int err;
>> +
>> +	calculate_flow_sizes(fs, &key_size, &classifier_size, &num_hdrs);
>> +
>> +	key = kzalloc(key_size, GFP_KERNEL);
>> +	if (!key)
>> +		return -ENOMEM;
> 
> So key is allocated here ...
> 
> 
>> +
>> +	/*
>> +	 * virtio_net_ff_obj_ff_classifier is already included in the
>> +	 * classifier_size.
>> +	 */

>> +
>> +	err = insert_rule(ff, eth_rule, c->id, key, key_size);
> 
> 
> ... copied by insert_rule
> 
> 
>> +	if (err) {
>> +		/* destroy_classifier will free the classifier */
>> +		destroy_classifier(ff, c->id);
>> +		goto err_key;
>> +	}
>> +
> 
> 
> ... and apparently never freed?
> 
> 
> I think it's because the API of insert_rule is confusing...
> 

Nice catch. I changed insert_rule to free key when it's successful.

build_and_insert will handle freeing it when it fails.

>> +	return 0;
>> +
>> +err_classifier:
>> +	kfree(c);
>> +err_key:
>> +	kfree(key);
>> +
>> +	return err;
>> +}
>> +
>