[PATCH net-next] devlink: prevent function configuration when port is active

[PATCH net-next] devlink: prevent function configuration when port is active

[Tariq Toukan]

From: Oren Sidi <osidi@nvidia.com>

Reject port function parameter changes when the port is active.
Enforcing this insures configuration integrity and consistency.

Signed-off-by: Oren Sidi <osidi@nvidia.com>
Reviewed-by: Aya Levin <ayal@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/devlink/port.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/devlink/port.c b/net/devlink/port.c
index 1528f2d148df..6a9857bf79da 100644
--- a/net/devlink/port.c
+++ b/net/devlink/port.c
@@ -708,7 +708,10 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
 					  struct netlink_ext_ack *extack)
 {
 	const struct devlink_port_ops *ops = devlink_port->ops;
+	enum devlink_port_fn_opstate opstate;
+	enum devlink_port_fn_state state;
 	struct nlattr *attr;
+	int err;
 
 	if (tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] &&
 	    !ops->port_fn_hw_addr_set) {
@@ -721,6 +724,20 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
 				    "Function does not support state setting");
 		return -EOPNOTSUPP;
 	}
+	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
+	    ops->port_fn_state_get && !tb[DEVLINK_PORT_FN_ATTR_STATE]) {
+		err = ops->port_fn_state_get(devlink_port, &state,
+					     &opstate, extack);
+		if (err)
+			return err;
+
+		if (state == DEVLINK_PORT_FN_STATE_ACTIVE) {
+			NL_SET_ERR_MSG(extack,
+				       "port function parameters can't be configured when port is up");
+			return -EINVAL;
+		}
+	}
+
 	attr = tb[DEVLINK_PORT_FN_ATTR_CAPS];
 	if (attr) {
 		struct nla_bitfield32 caps;

base-commit: 001b5d347d8ba39b2dccaefcc57967b18caec8fe
-- 
2.44.0

Re: [PATCH net-next] devlink: prevent function configuration when port is active

[Przemek Kitszel]

On 8/10/26 12:32, Tariq Toukan wrote:
> From: Oren Sidi <osidi@nvidia.com>
> 
> Reject port function parameter changes when the port is active.
> Enforcing this insures configuration integrity and consistency.
> 
> Signed-off-by: Oren Sidi <osidi@nvidia.com>
> Reviewed-by: Aya Levin <ayal@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>   net/devlink/port.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/net/devlink/port.c b/net/devlink/port.c
> index 1528f2d148df..6a9857bf79da 100644
> --- a/net/devlink/port.c
> +++ b/net/devlink/port.c
> @@ -708,7 +708,10 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
>   					  struct netlink_ext_ack *extack)
>   {
>   	const struct devlink_port_ops *ops = devlink_port->ops;
> +	enum devlink_port_fn_opstate opstate;
> +	enum devlink_port_fn_state state;
>   	struct nlattr *attr;
> +	int err;
>   
>   	if (tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] &&
>   	    !ops->port_fn_hw_addr_set) {
> @@ -721,6 +724,20 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
>   				    "Function does not support state setting");
>   		return -EOPNOTSUPP;
>   	}
> +	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&

nit: this is the only place that marks the logic only for SUBFUNCTION,
would be good to amend commit message/subject line:
s/function/subfunction/ # for the subject

with or without the above:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> +	    ops->port_fn_state_get && !tb[DEVLINK_PORT_FN_ATTR_STATE]) {
> +		err = ops->port_fn_state_get(devlink_port, &state,
> +					     &opstate, extack);
> +		if (err)
> +			return err;
> +
> +		if (state == DEVLINK_PORT_FN_STATE_ACTIVE) {
> +			NL_SET_ERR_MSG(extack,
> +				       "port function parameters can't be configured when port is up");
> +			return -EINVAL;
> +		}
> +	}
> +
>   	attr = tb[DEVLINK_PORT_FN_ATTR_CAPS];
>   	if (attr) {
>   		struct nla_bitfield32 caps;
> 
> base-commit: 001b5d347d8ba39b2dccaefcc57967b18caec8fe

Re: [PATCH net-next] devlink: prevent function configuration when port is active

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
devlink: prevent function configuration when port is active

This adds a check in devlink_port_function_validate() that queries the
port function state for PCI_SF flavour ports and rejects the request with
-EINVAL when the function is active.  A few questions about the scope and
the enforcement of the new rule below.

> diff --git a/net/devlink/port.c b/net/devlink/port.c
> index 1528f2d148dfc..6a9857bf79da1 100644
> --- a/net/devlink/port.c
> +++ b/net/devlink/port.c
> @@ -708,7 +708,10 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
>  					  struct netlink_ext_ack *extack)
>  {
>  	const struct devlink_port_ops *ops = devlink_port->ops;
> +	enum devlink_port_fn_opstate opstate;
> +	enum devlink_port_fn_state state;
>  	struct nlattr *attr;
> +	int err;
>  
>  	if (tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] &&
>  	    !ops->port_fn_hw_addr_set) {
> @@ -721,6 +724,20 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
>  				    "Function does not support state setting");
>  		return -EOPNOTSUPP;
>  	}
> +	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
> +	    ops->port_fn_state_get && !tb[DEVLINK_PORT_FN_ATTR_STATE]) {

[Severity: Medium]
Can the new restriction be bypassed by including a state attribute in the
same request?  The condition skips the state query whenever
tb[DEVLINK_PORT_FN_ATTR_STATE] is present, regardless of the value being
requested or of the port's current state.

devlink_port_function_set() then applies the parameters before the state,
by design:

	attr = tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR];
	if (attr) {
		err = devlink_port_function_hw_addr_set(port, attr, extack);
	...
	/* Keep this as the last function attribute set, so that when
	 * multiple port function attributes are set along with state,
	 * Those can be applied first before activating the state.
	 */
	attr = tb[DEVLINK_PORT_FN_ATTR_STATE];

So for an already active mlx5 SF, a request such as

	devlink port function set <sf> hw_addr XX:.. state active

would run mlx5_devlink_port_fn_hw_addr_set() ->
mlx5_eswitch_set_vport_mac() against the live function, and the state set
afterwards is a no-op:

drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c:mlx5_sf_state_set() {
	...
	if (state == mlx5_sf_to_devlink_state(sf->hw_state))
		goto out;
	...
}

The same applies to caps and max_io_eqs combined with state active, and to
state inactive where the parameter is programmed while the function is
still active.  Is the intent to also cover the "parameters plus state" case,
for instance by checking the current state whenever any settable parameter
attribute is present?

[Severity: Low]
Does this also change the result of a request that sets nothing at all?
devlink_port_function_set() calls nla_parse_nested() and then
devlink_port_function_validate() before it looks at whether any of
tb[HW_ADDR], tb[CAPS], tb[MAX_IO_EQS] or tb[STATE] is present.

For a zero-length DEVLINK_ATTR_PORT_FUNCTION nest every tb[] entry is NULL,
so the new condition is true, ops->port_fn_state_get() is called (taking
mlx5's sf_state_lock), and an active SF now returns -EINVAL.  Previously
the function fell through every "if (attr)" block, sent the
DEVLINK_CMD_PORT_NEW notification and returned 0.  Would keying the check
on the presence of a settable attribute avoid that?

[Severity: Low]
The subject and the body say "prevent function configuration when port is
active" and "Reject port function parameter changes when the port is
active", but the check only applies to DEVLINK_PORT_FLAVOUR_PCI_SF.

VF and PF function ports also expose a function admin state, for example
mlx5_esw_pf_vf_dl_port_ops in
drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c:

	.port_fn_state_get = mlx5_devlink_pf_port_fn_state_get,
	.port_fn_state_set = mlx5_devlink_pf_port_fn_state_set,

and those keep the old behaviour.  Could the commit message (or a comment
here) say that the rule is limited to subfunctions and why?

> +		err = ops->port_fn_state_get(devlink_port, &state,
> +					     &opstate, extack);
> +		if (err)
> +			return err;
> +
> +		if (state == DEVLINK_PORT_FN_STATE_ACTIVE) {

[Severity: Medium]
This turns requests that used to succeed into -EINVAL for every PCI_SF
port.  Before this patch, hw_addr, roce and max_io_eqs could be set on an
active SF: mlx5_devlink_port_fn_hw_addr_set() calls
mlx5_eswitch_set_vport_mac() and mlx5_devlink_port_fn_max_io_eqs_set()
has no active-state rejection of its own.

Documentation/networking/devlink/devlink-port.rst still only recommends the
ordering:

	For subfunctions, this means user should configure port function
	attribute before activating the port function.

and documents max_io_eqs as settable for subfunctions without qualification.
Should the documentation be updated to say the ordering is now mandatory and
enforced with -EINVAL, and should the behaviour change be called out in the
commit message?

Since the policy lives in devlink core rather than in the driver, it also
becomes the rule for every current and future driver registering
DEVLINK_PORT_FLAVOUR_PCI_SF ports, including hardware that can change a
parameter while the function is active.  Was that intended?

> +			NL_SET_ERR_MSG(extack,
> +				       "port function parameters can't be configured when port is up");
> +			return -EINVAL;

[Severity: Low]
This isn't a bug, but the wording says "port is up" while the condition
tests the function state DEVLINK_PORT_FN_STATE_ACTIVE, which user space
and the documentation call active/inactive ("devlink port show" prints
"state active").  Would "function state is active" map better onto what
users can observe?

Also, the sibling rejections in the same function are capitalised and use
NL_SET_ERR_MSG_ATTR to point at the offending attribute:

	NL_SET_ERR_MSG_ATTR(extack, tb[DEVLINK_PORT_FN_ATTR_STATE],
			    "Function does not support state setting");

The offending attribute (hw_addr, caps or max_io_eqs) is known here too,
so could NL_SET_ERR_MSG_ATTR be used for consistency?

> +		}
> +	}
> +
>  	attr = tb[DEVLINK_PORT_FN_ATTR_CAPS];
>  	if (attr) {
>  		struct nla_bitfield32 caps;

[ ... ]
-- 
pw-bot: cr