Re: [PATCH net-next v16 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-17 16:11:26
Also in:
driver-core, linux-doc, lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] Installing `.flash_update = dsa_devlink_flash_update` unconditionally in the single shared `dsa_devlink_ops` removes the… -- net: dsa: add devlink flash_update callback to dsa_switch_ops This adds a devlink_flash_update callback to dsa_switch_ops and a trampoline in net/dsa/devlink.c that forwards to it, returning -EOPNOTSUPP when the driver does not implement the callback. The .flash_update op is installed unconditionally in the shared dsa_devlink_ops table.
quoted hunk
diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c index ed342f345692b..25311a87cbc56 100644 --- a/net/dsa/devlink.c +++ b/net/dsa/devlink.c@@ -20,6 +20,18 @@ static int dsa_devlink_info_get(struct devlink *dl, return -EOPNOTSUPP; } +static int dsa_devlink_flash_update(struct devlink *dl, + struct devlink_flash_update_params *params, + struct netlink_ext_ack *extack) +{ + struct dsa_switch *ds = dsa_devlink_to_ds(dl); + + if (!ds->ops->devlink_flash_update) + return -EOPNOTSUPP; + + return ds->ops->devlink_flash_update(ds, params, extack); +} +
[ ... ]
quoted hunk
@@ -169,6 +181,7 @@ dsa_devlink_sb_occ_tc_port_bind_get(struct devlink_port *dlp, static const struct devlink_ops dsa_devlink_ops = { .info_get = dsa_devlink_info_get, + .flash_update = dsa_devlink_flash_update,
[Severity: Low]
This isn't a bug as such, since the commit message states the trade-off,
but is the full extent of the behaviour change on switches without the
callback covered?
Since dsa_devlink_ops is shared by every DSA switch, the core gate in
devlink_nl_flash_update_doit() now always passes:
net/devlink/dev.c:devlink_nl_flash_update_doit() {
if (!devlink->ops->flash_update)
return -EOPNOTSUPP;
...
file_name = nla_data(nla_file_name);
ret = request_firmware_direct(¶ms.fw, file_name, devlink->dev);
...
devlink_flash_update_begin_notify(devlink);
ret = devlink->ops->flash_update(devlink, ¶ms, info->extack);
devlink_flash_update_end_notify(devlink);
}
So on a switch that does not set ds->ops->devlink_flash_update, a flash
request now reads the entire userspace-named file from the firmware search
paths into kernel memory before dsa_devlink_flash_update() gets to return
-EOPNOTSUPP.
Beyond the -ENOENT versus -EOPNOTSUPP change already described, does this
also mean that on such switches a flash request becomes a file-existence
probe of the firmware search paths, performs an unbounded read of a
/lib/firmware file into kernel memory, and emits a FLASH_UPDATE /
FLASH_UPDATE_END pair to devlink monitors for an operation that can never
succeed?
For reference, path traversal is not a concern here:
_request_firmware() rejects names containing ".." via
name_contains_dotdot(), and the operation requires CAP_NET_ADMIN.
Would it be worth mentioning the firmware read and the notifications for
switches without the callback in the commit message, in addition to the
errno change?
.sb_pool_get = dsa_devlink_sb_pool_get, .sb_pool_set = dsa_devlink_sb_pool_set, .sb_port_pool_get = dsa_devlink_sb_port_pool_get,
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789477568.git.daniel%40makrotopia.org