On Wed, Nov 19, 2025 at 12:26:23AM -0600, Dan Jurgens wrote:
> 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.
I mean the compiler padding. set to 0 where?
> >
> >
> >> + 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
> >
