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 = ð_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 = ð_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;
>> +}
>> +
>
