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
