netdev - Re: [PATCH net-next v11 08/12] virtio_net: Use existing classifier if possible

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: <cb64732c-294e-49e7-aeb5-f8f2f082837e@nvidia.com>
Date: Wed, 19 Nov 2025 00:26:23 -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 08/12] virtio_net: Use existing classifier if
 possible

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
>