Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] virtnet_ff_cleanup() (drivers/net/virtio_net.c:5945-5962)
issues the new…
--
virtio_net: Create a FF group for ethtool steering
This adds a single flow filter group object, created during
virtnet_ff_init() with a hard-coded object ID, and destroyed in
virtnet_ff_cleanup(). A new uapi struct and object type define the
group payload.
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f78b881e493b..294cfd70662d3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
[ ... ]
> @@ -5897,6 +5907,19 @@ static int virtnet_ff_init(struct virtnet_ff *ff,
> struct virtio_device *vdev)
> if (err)
> goto err_ff_action;
>
> + ethtool_group.group_priority =
> cpu_to_le16(VIRTNET_FF_ETHTOOL_GROUP_PRIORITY);
> +
> + /* Use priority for the object ID. */
> + err = virtio_admin_obj_create(vdev,
> + VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
> + VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
> + VIRTIO_ADMIN_GROUP_TYPE_SELF,
> + 0,
> + ðtool_group,
> + sizeof(ethtool_group));
> + if (err)
> + goto err_ff_action;
> +
> ff->vdev = vdev;
> ff->ff_supported = true;
>
> @@ -5921,9 +5944,18 @@ static int virtnet_ff_init(struct virtnet_ff *ff,
> struct virtio_device *vdev)
>
> static void virtnet_ff_cleanup(struct virtnet_ff *ff)
> {
> + int err;
> +
> if (!ff->ff_supported)
> return;
>
> + err = virtio_admin_obj_destroy(ff->vdev,
> + VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
> + VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
> + VIRTIO_ADMIN_GROUP_TYPE_SELF,
> + 0);
> + WARN_ON_ONCE(err);
> +
> kfree(ff->ff_actions);
> kfree(ff->ff_mask);
> kfree(ff->ff_caps);
[Severity: Medium]
Is WARN_ON_ONCE() the right reporting mechanism for this error, given that
err here is not a kernel invariant? Looking at virtio_admin_obj_destroy()
in drivers/virtio/virtio_admin_commands.c, err comes from either an
allocation failure or the device-supplied admin command status:
data = kzalloc_obj(*data);
if (!data)
return -ENOMEM;
...
err = vdev->config->admin_cmd_exec(vdev, &cmd);
kfree(data);
return err;
virtnet_ff_cleanup() runs from virtnet_remove(), from the virtnet_probe()
error path, and from virtnet_freeze_down() on suspend. An already reset or
NEEDS_RESET device, a surprise removal, or a misbehaving backend would then
produce a warning splat on an ordinary teardown path, and a guest panic when
panic_on_warn is set. Would a rate-limited netdev_warn() or dev_warn()
convey the same information here?
The second part of the question is what happens to the device-side object
when the destroy does fail. The error is discarded and the driver then
unconditionally does:
kfree(ff->ff_actions);
kfree(ff->ff_mask);
kfree(ff->ff_caps);
ff->ff_supported = false;
so no record of the group object is kept, while the device may still hold
it. Since virtnet_ff_init() always re-creates the same hard-coded ID:
err = virtio_admin_obj_create(vdev,
VIRTIO_NET_RESOURCE_OBJ_FF_GROUP,
VIRTNET_FF_ETHTOOL_GROUP_PRIORITY,
...);
if (err)
goto err_ff_action;
can a group object left behind by a failed destroy make the next create of
that same ID fail, and thereby turn an ignorable teardown error into a
probe or resume failure on re-bind? There is no retry, no alternate ID, and
no state retained that would allow either.
Note that this depends on whether a device reset already releases admin
resource objects, which is not something that can be determined from the
tree. Would it be worth documenting that expectation alongside the destroy
call?
This code is unchanged at the end of the series.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com
