On 11/18/25 3:55 PM, Michael S. Tsirkin wrote:
> On Tue, Nov 18, 2025 at 08:38:58AM -0600, Daniel Jurgens wrote:
>> Classifiers can be used by more than one rule. If there is an existing
>> classifier, use it instead of creating a new one.
>> + 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->classifier, &(*c)->classifier, tmp->size)) {
>
> note that classifier has padding bytes.
> comparing these with memcmp is not safe, is it?
The reserved bytes are set to 0, this is fine.
>
>
>> + 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)
>
> what kind of locking prevents two threads racing in this code?
The ethtool calls happen under rtnl_lock.
>
>
>> @@ -6932,29 +6945,30 @@ static int setup_classifier(struct virtnet_ff *ff,
>> struct virtnet_classifier *c)
>> (*c)->size);
>> if (err)
>> goto err_xarray;
>>
>> + refcount_set(&(*c)->refcount, 1);
>
>
> so you insert uninitialized refcount? can't another thread find it
> meanwhile?
Again, rtnl_lock.
>>
>> err = insert_rule(ff, eth_rule, c->id, key, key_size);
>> if (err) {
>> /* destroy_classifier will free the classifier */
>
> will free is no longer correct, is it?
Clarified the comment.
>
>> - destroy_classifier(ff, c->id);
>> + try_destroy_classifier(ff, c->id);
>> goto err_key;
>> }
>>
>> --
>> 2.50.1
>
