netdev - Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

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: <ZRa1cu4TlCuj51gD@nanopsycho>
Date: Fri, 29 Sep 2023 13:30:58 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: David Ahern <dsahern@...il.com>
Cc: netdev@...r.kernel.org, stephen@...workplumber.org,
	daniel.machon@...rochip.com
Subject: Re: [patch iproute2-next v2 3/5] devlink: introduce support for
 netns id for nested handle

Wed, Sep 20, 2023 at 09:30:01AM CEST, jiri@...nulli.us wrote:
>Tue, Sep 19, 2023 at 08:48:29PM CEST, dsahern@...il.com wrote:
>>On 9/19/23 11:19 AM, Jiri Pirko wrote:
>>>>
>>>>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>>  {
>>>>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>>>>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>>>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>>>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>>>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>>>>> +
>>>>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>>>>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>>>>> +
>>>>> +		if (id >= 0) {
>>>>> +			char *name = netns_name_by_id(id);
>>>>> +
>>>>> +			if (name) {
>>>>> +				print_string(PRINT_ANY,
>>>>> +					     "nested_devlink_netns",
>>>>> +					     " nested_devlink_netns %s", name);
>>>>> +				free(name);
>>>>> +			} else {
>>>>> +				print_int(PRINT_ANY,
>>>>> +					  "nested_devlink_netnsid",
>>>>> +					  " nested_devlink_netnsid %d", id);
>>>>> +			}
>>>>> +		} else {
>>>>> +			print_string(PRINT_FP, NULL,
>>>>> +				     " nested_devlink_netnsid %s", "unknown");
>>>>> +			print_int(PRINT_JSON,
>>>>> +				  "nested_devlink_netnsid", NULL, id);
>>>>> +		}
>>>> Also, devlink in the name here provides no addititional value (devlink
>>>> is the command name) and why add 'nested'? The attribute is just
>>>> NETNS_ID, so why not just 'netnsid' here.
>>> Well, it is a netnsid of the nested devlink instance, not the object
>>> (e.g. port) itself. Omitting that would be misleading. Any idea how to
>>> do this differently?
>>> 
>>> 
>>
>>The attribute is a namespace id, and the value is a namespace id. Given
>>that, the name here should be netnsid (or nsid - we did a horrible job
>>with consistency across iproute2 commands). I have not followed the
>>kernel patches to understand what you mean by nested devlink instance.
>
>Please do that. Again, the netnsid is related to the nested instance.
>Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>as you suggest is wrong. Another possibility would be do nest this into
>object, but:
>1) I didn't find nice way to do that
>2) We would break linecards as they expose nested_devlink already

Did you have a chance to check this? I have v3 ready for submission with
the other changes you requested.

Thanks!


>
>IDK :/