On Wed, Nov 19, 2025 at 01:18:56AM -0600, Dan Jurgens wrote:
> On 11/19/25 12:35 AM, Michael S. Tsirkin wrote:
> > 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?
>
> There's no compiler padding in virtio_net_ff_selector. There are
> reserved fields between the count and selector array.
I might be missing something here, but are not the
structures this code compares of the type struct virtnet_classifier
not virtio_net_ff_selector ?
and that one is:
struct virtnet_classifier {
size_t size;
+ refcount_t refcount;
u32 id;
struct virtio_net_resource_obj_ff_classifier classifier;
};
which seems to have some padding depending on the architecture.
> >
> >>>
> >>>
> >>>> + 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
> >>>
> >
