netdev - Re: [PATCH net-next 1/3] devlink: Introduce devlink eswitch state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPEsjG-mFIx-IqtV@x130>
Date: Thu, 16 Oct 2025 10:34:04 -0700
From: Saeed Mahameed <saeed@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
	Tariq Toukan <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>, mbloch@...dia.com,
	Parav Pandit <parav@...dia.com>,
	Adithya Jayachandran <ajayachandra@...dia.com>
Subject: Re: [PATCH net-next 1/3] devlink: Introduce devlink eswitch state

On 16 Oct 11:16, Jiri Pirko wrote:
>Thu, Oct 16, 2025 at 03:36:16AM +0200, saeed@...nel.org wrote:
>>From: Parav Pandit <parav@...dia.com>
>
>[...]
>
>
>>@@ -722,6 +734,24 @@ int devlink_nl_eswitch_set_doit(struct sk_buff *skb, struct genl_info *info)
>> 			return err;
>> 	}
>>
>>+	state = DEVLINK_ESWITCH_STATE_ACTIVE;
>>+	if (info->attrs[DEVLINK_ATTR_ESWITCH_STATE]) {
>>+		if (!ops->eswitch_state_set)
>>+			return -EOPNOTSUPP;
>>+		state = nla_get_u8(info->attrs[DEVLINK_ATTR_ESWITCH_STATE]);
>>+	}
>>+	/* If user did not supply the state attribute, the default is
>>+	 * active state. If the state was not explicitly set, set the default
>>+	 * state for drivers that support eswitch state.
>>+	 * Keep this after mode-set as state handling can be dependent on
>>+	 * the eswitch mode.
>>+	 */
>>+	if (ops->eswitch_state_set) {
>>+		err = ops->eswitch_state_set(devlink, state, info->extack);
>
>Calling state_set() upon every DEVLINK_CMD_ESWITCH_SET call,
>even if STATE attr is not present, is plain wrong. Don't do it.
>I don't really understand why you do so.
>

I don't get the "plain wrong" part? Please explain.

Here's is what we are trying to solve and why I think this way is the best
way to solve it, unless you have a better idea.

We want to preserve backwards compatibility, think of:
  - old devlink iproute2 (doesn't provide STATE attr).
  - new kernel (expects new STATE attr).

Upon your request we split mode and state handling into separate callbacks,
meaning, you set mode first and then state in DEVLINK_CMD_ESWITCH_SET.

ops->mode_set(); doesn't have information on state, so a drivers that
implement state_set() will expect state_set() to be called after
mode_set(), otherwise, state will remain inactive for that driver.

If state attr is not provided (e.g. old devlink userspace) but the user
expects state to be active, then if we do what you ask for, we don't
call state_set() and after mode_set() we will be in an inactive state,
while user expects active (default behavior) for backward compatibility.

To solve this we always default state = ACTIVE (if state attr wasn't
provided) and call state_set();

Let me know if you have better ideas, on how to solve this problem.
Otherwise, this patch's way of preserving backward compatibility is 
not "plain wrong".

We can optimize to call set_state() only if (mode || state) attr was
provided. Let me know if that works for you.

- Saeed.