Re: [net-next,v23,10/14] virtio_net: Use existing classifier if possible

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] `try_destroy_classifier()`
  (drivers/net/virtio_net.c:5871-5888) treats a failed…

--

virtio_net: Use existing classifier if possible

This adds a refcount to struct virtnet_classifier and makes
setup_classifier() scan ff->classifiers for an identical classifier
object, reusing it instead of creating a duplicate device object.  The
release side becomes try_destroy_classifier(), which only destroys the
device object when the last reference goes away.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6217c5bcfee9f..6c07eab047afe 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5823,11 +5825,24 @@ static bool validate_mask(const struct virtnet_ff *ff,
>  	return false;
>  }
>  
> -static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
> +static int setup_classifier(struct virtnet_ff *ff,
> +			    struct virtnet_classifier **c)
>  {
> +	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->obj, &(*c)->obj, tmp->size)) {
> +			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)

[ ... ]

> @@ -5835,30 +5850,31 @@ static int setup_classifier(struct virtnet_ff *ff, struct virtnet_classifier *c)
>  
> -static void destroy_classifier(struct virtnet_ff *ff,
> -			       u32 classifier_id)
> +static void try_destroy_classifier(struct virtnet_ff *ff, u32 classifier_id)
>  {
>  	struct virtnet_classifier *c;
>  	int err;
>  
>  	c = xa_load(&ff->classifiers, classifier_id);
> -	if (c) {
> +	if (c && refcount_dec_and_test(&c->refcount)) {
>  		err = virtio_admin_obj_destroy(ff->vdev,
>  					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
>  					       c->id,

[Severity: Medium]
With this patch ff->classifiers becomes the authoritative mirror of the
classifier objects that exist in the device, since setup_classifier() now
scans it with memcmp() to decide whether an equivalent device object is
already present.

Given that, is the unconditional cleanup in the tail of
try_destroy_classifier() still correct?

drivers/net/virtio_net.c:try_destroy_classifier() {
	c = xa_load(&ff->classifiers, classifier_id);
	if (c && refcount_dec_and_test(&c->refcount)) {
		err = virtio_admin_obj_destroy(ff->vdev,
					       VIRTIO_NET_RESOURCE_OBJ_FF_CLASSIFIER,
					       c->id,
					       VIRTIO_ADMIN_GROUP_TYPE_SELF,
					       0);
		WARN_ON_ONCE(err);

		xa_erase(&ff->classifiers, c->id);
		kfree(c);
	}
}

virtio_admin_obj_destroy() is synchronous and returns non-zero when the
object was not removed, and it can fail without any device malfunction:

drivers/virtio/virtio_admin_commands.c:virtio_admin_obj_destroy() {
	if (!vdev->config->admin_cmd_exec)
		return -EOPNOTSUPP;

	data = kzalloc_obj(*data);
	if (!data)
		return -ENOMEM;
	...
	err = vdev->config->admin_cmd_exec(vdev, &cmd);

After such a failure the device still holds the classifier object while the
driver has erased and freed its tracking entry.  Does this leak one of the
device's ff_caps->classifiers_limit slots permanently?

Two follow-on effects also look reachable from that state:

- the id is immediately available to xa_alloc() again, so a later
  virtio_admin_obj_create() for that id can be rejected by the device and
  userspace sees an ETHTOOL_SRXCLSRLINS failure the driver cannot explain.

- an identical classifier requested afterwards no longer matches anything in
  the xarray, so a second device object is created for it, which is the
  duplicate-classifier case the commit message says this change avoids.

Would it be better to leave the entry in ff->classifiers (and restore the
refcount) when virtio_admin_obj_destroy() reports an error?

Separately, in destroy_ethtool_rule() the FF_RULE destroy result is saved but
not acted on before the classifier reference is dropped:

drivers/net/virtio_net.c:destroy_ethtool_rule() {
	err = virtio_admin_obj_destroy(ff->vdev,
				       VIRTIO_NET_RESOURCE_OBJ_FF_RULE,
				       eth_rule->flow_spec.location,
				       VIRTIO_ADMIN_GROUP_TYPE_SELF,
				       0);

	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
	try_destroy_classifier(ff, eth_rule->classifier_id);
	kfree(eth_rule);

	return err;
}

If the rule destroy fails, can the now shared classifier be destroyed on the
device while a device-side rule still references its id?

>  @@ -5885,7 +5901,7 @@ static int destroy_ethtool_rule(struct virtnet_ff *ff,
>  				       0);
>  
>  	xa_erase(&ff->ethtool.rules, eth_rule->flow_spec.location);
> -	destroy_classifier(ff, eth_rule->classifier_id);
> +	try_destroy_classifier(ff, eth_rule->classifier_id);
>  	kfree(eth_rule);
>  
>  	return err;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com