[PATCH] common/mlx5: fix the sysfs port name translation

[PATCH] common/mlx5: fix the sysfs port name translation

[Bing Zhao]

With some OFED or upstream kernel of mlx5, the port name fetched from
"/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the
EOL. The sscanf() will return the scanned items number with this EOL.

In such case, the "equal to" condition is considered as false and
the function mlx5_translate_port_name() will recognize the port type
wrongly with UNKNOWN result.

By changing the condition from "equal to" to "more than or equal
to", the port type can be recognized successfully.

Fixes: 654810b56828 ("common/mlx5: share Netlink commands")
Fixes: 420bbdae89f2 ("net/mlx5: fix host physical function representor naming")

Signed-off-by: Bing Zhao <bingz@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 drivers/common/mlx5/linux/mlx5_common_os.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/common/mlx5/linux/mlx5_common_os.c b/drivers/common/mlx5/linux/mlx5_common_os.c
index aafff60eeb..655347a7c8 100644
--- a/drivers/common/mlx5/linux/mlx5_common_os.c
+++ b/drivers/common/mlx5/linux/mlx5_common_os.c
@@ -108,7 +108,7 @@ mlx5_translate_port_name(const char *port_name_in,
 	sc_items = sscanf(port_name_in, "%c%c%d%c%c%d%c",
 			  &pf_c1, &pf_c2, &port_info_out->pf_num,
 			  &vf_c1, &vf_c2, &port_info_out->port_name, &eol);
-	if (sc_items == 6 && pf_c1 == 'p' && pf_c2 == 'f') {
+	if (sc_items >= 6 && pf_c1 == 'p' && pf_c2 == 'f') {
 		if (vf_c1 == 'v' && vf_c2 == 'f') {
 			/* Kernel ver >= 5.0 or OFED ver >= 4.6 */
 			port_info_out->name_type =
@@ -128,7 +128,7 @@ mlx5_translate_port_name(const char *port_name_in,
 	 */
 	sc_items = sscanf(port_name_in, "%c%d%c",
 			  &pf_c1, &port_info_out->port_name, &eol);
-	if (sc_items == 2 && pf_c1 == 'p') {
+	if (sc_items >= 2 && pf_c1 == 'p') {
 		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_UPLINK;
 		return;
 	}
@@ -138,7 +138,7 @@ mlx5_translate_port_name(const char *port_name_in,
 	 */
 	sc_items = sscanf(port_name_in, "%c%c%d%c",
 			  &pf_c1, &pf_c2, &port_info_out->pf_num, &eol);
-	if (sc_items == 3 && pf_c1 == 'p' && pf_c2 == 'f') {
+	if (sc_items >= 3 && pf_c1 == 'p' && pf_c2 == 'f') {
 		port_info_out->port_name = -1;
 		port_info_out->name_type = MLX5_PHYS_PORT_NAME_TYPE_PFHPF;
 		return;
-- 
2.21.0

Re: [PATCH] common/mlx5: fix the sysfs port name translation

[Stephen Hemminger]

On Thu, 10 Nov 2022 17:57:26 +0200
Bing Zhao <bingz@nvidia.com> wrote:

> With some OFED or upstream kernel of mlx5, the port name fetched from
> "/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the
> EOL. The sscanf() will return the scanned items number with this EOL.

Why not fix the DPDK driver to strip off the new line when the port
name is read? The code in mlx5_os.c should do it there.

RE: [PATCH] common/mlx5: fix the sysfs port name translation

[Bing Zhao]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, November 11, 2022 12:22 AM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; dev@dpdk.org; Raslan Darawsheh
> <rasland@nvidia.com>; stable@dpdk.org
> Subject: Re: [PATCH] common/mlx5: fix the sysfs port name
> translation
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 10 Nov 2022 17:57:26 +0200
> Bing Zhao <bingz@nvidia.com> wrote:
> 
> > With some OFED or upstream kernel of mlx5, the port name fetched
> from
> > "/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as
> the
> > EOL. The sscanf() will return the scanned items number with this
> EOL.
> 
> Why not fix the DPDK driver to strip off the new line when the port
> name is read? The code in mlx5_os.c should do it there.

Thanks for your comments. Yes, IF_NAMESIZE is large enough to read all the characters including the newline into the buffer.
After fgets(), the strlen() or strchr() can be used to remove the tailing newline character.


BR. Bing

Re: [PATCH] common/mlx5: fix the sysfs port name translation

[Stephen Hemminger]

On Fri, 11 Nov 2022 05:41:05 +0000
Bing Zhao <bingz@nvidia.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, November 11, 2022 12:22 AM
> > To: Bing Zhao <bingz@nvidia.com>
> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> > <matan@nvidia.com>; dev@dpdk.org; Raslan Darawsheh
> > <rasland@nvidia.com>; stable@dpdk.org
> > Subject: Re: [PATCH] common/mlx5: fix the sysfs port name
> > translation
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, 10 Nov 2022 17:57:26 +0200
> > Bing Zhao <bingz@nvidia.com> wrote:
> >   
> > > With some OFED or upstream kernel of mlx5, the port name fetched  
> > from  
> > > "/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as  
> > the  
> > > EOL. The sscanf() will return the scanned items number with this  
> > EOL.
> > 
> > Why not fix the DPDK driver to strip off the new line when the port
> > name is read? The code in mlx5_os.c should do it there.  
> 
> Thanks for your comments. Yes, IF_NAMESIZE is large enough to read all the characters including the newline into the buffer.
> After fgets(), the strlen() or strchr() can be used to remove the tailing newline character.
> 
> 
> BR. Bing

Another alternative would be to use getline() which doesn't have annoying length restrictions
to deal with.

[PATCH v2] common/mlx5: fix the sysfs port name translation

[Bing Zhao]

With some OFED or upstream kernel of mlx5, the port name fetched from
"/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the
EOL. The sscanf() will return the scanned items number with this EOL.

In such case, the "equal to" condition is considered as false and
the function mlx5_translate_port_name() will recognize the port type
wrongly with UNKNOWN result.

The tailing carriage return character should be removed before
calling the mlx5_translate_port_name(), this was already done in the
NL message handling. In the meanwhile, the possible incorrect line
feed character is also taken into consideration.

Fixes: 654810b56828 ("common/mlx5: share Netlink commands")
Fixes: 420bbdae89f2 ("net/mlx5: fix host physical function representor naming")
Cc: stable@dpdk.org

Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index f1ff6f49f9..2955008592 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -1035,7 +1035,8 @@ int
 mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 {
 	char ifname[IF_NAMESIZE];
-	char port_name[IF_NAMESIZE];
+	char *port_name;
+	size_t port_name_size = 0;
 	FILE *file;
 	struct mlx5_switch_info data = {
 		.master = 0,
@@ -1048,6 +1049,7 @@ mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
+	ssize_t line_size;
 
 	if (!if_indextoname(ifindex, ifname)) {
 		rte_errno = errno;
@@ -1063,8 +1065,26 @@ mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 
 	file = fopen(phys_port_name, "rb");
 	if (file != NULL) {
-		if (fgets(port_name, IF_NAMESIZE, file) != NULL)
+		char *tail_nl;
+
+		port_name = (char *)malloc(IF_NAMESIZE);
+		if (port_name == NULL) {
+			fclose(file);
+			rte_errno = ENOMEM;
+			return -rte_errno;
+		}
+		line_size = getline(&port_name, &port_name_size, file);
+		if (line_size > 0) {
+			/* Remove tailing newline character. */
+			tail_nl = strchr(port_name, '\n');
+			if (tail_nl)
+				*tail_nl = '\0';
+			tail_nl = strchr(port_name, '\r');
+			if (tail_nl)
+				*tail_nl = '\0';
 			mlx5_translate_port_name(port_name, &data);
+		}
+		free(port_name);
 		fclose(file);
 	}
 	file = fopen(phys_switch_id, "rb");
-- 
2.31.1

Re: [PATCH v2] common/mlx5: fix the sysfs port name translation

[Stephen Hemminger]

On Wed, 22 Mar 2023 13:34:12 +0200
Bing Zhao <bingz@nvidia.com> wrote:

> +
> +		port_name = (char *)malloc(IF_NAMESIZE);
> +		if (port_name == NULL) {
> +			fclose(file);
> +			rte_errno = ENOMEM;
> +			return -rte_errno;
> +		}
> +		line_size = getline(&port_name, &port_name_size, file);

This code is doing unnecessary extra work here.
From getline() man page:

     If *lineptr is set to NULL before the call, then getline() will
       allocate a buffer for storing the line.  This buffer should be
       freed by the user program even if getline() failed.

Re: [PATCH v2] common/mlx5: fix the sysfs port name translation

[Stephen Hemminger]

On Wed, 22 Mar 2023 13:34:12 +0200
Bing Zhao <bingz@nvidia.com> wrote:

> +			tail_nl = strchr(port_name, '\r');
> +			if (tail_nl)
> +				*tail_nl = '\0';

This is a file provided by Linux kernel, there is no way
it could have a CR character.

RE: [PATCH v2] common/mlx5: fix the sysfs port name translation

[Bing Zhao]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, March 22, 2023 10:59 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; dev@dpdk.org; Raslan Darawsheh
> <rasland@nvidia.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] common/mlx5: fix the sysfs port name translation
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 22 Mar 2023 13:34:12 +0200
> Bing Zhao <bingz@nvidia.com> wrote:
> 
> > +
> > +             port_name = (char *)malloc(IF_NAMESIZE);
> > +             if (port_name == NULL) {
> > +                     fclose(file);
> > +                     rte_errno = ENOMEM;
> > +                     return -rte_errno;
> > +             }
> > +             line_size = getline(&port_name, &port_name_size, file);
> 
> This code is doing unnecessary extra work here.
> From getline() man page:

Yes, I also checked the description of this interface before. The original purpose was to allocate / free the memory explicitly.
This will be removed.

> 
>      If *lineptr is set to NULL before the call, then getline() will
>        allocate a buffer for storing the line.  This buffer should be
>        freed by the user program even if getline() failed.

RE: [PATCH v2] common/mlx5: fix the sysfs port name translation

[Bing Zhao]

Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, March 22, 2023 11:00 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; dev@dpdk.org; Raslan Darawsheh
> <rasland@nvidia.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] common/mlx5: fix the sysfs port name translation
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 22 Mar 2023 13:34:12 +0200
> Bing Zhao <bingz@nvidia.com> wrote:
> 
> > +                     tail_nl = strchr(port_name, '\r');
> > +                     if (tail_nl)
> > +                             *tail_nl = '\0';
> 
> This is a file provided by Linux kernel, there is no way it could have a CR
> character.

Correct, thanks for pointing this. I was overanxious. I will remove this as well.

[PATCH v3] net/mlx5: fix the sysfs port name translation

[Bing Zhao]

With some OFED or upstream kernel of mlx5, the port name fetched from
"/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the
EOL. The sscanf() will return the scanned items number with this EOL.

In such case, the "equal to" condition is considered as false and
the function mlx5_translate_port_name() will recognize the port type
wrongly with UNKNOWN result.

The tailing carriage return character should be removed before
calling the mlx5_translate_port_name(), this was already done in the
NL message handling. In the meanwhile, the possible incorrect line
feed character is also taken into consideration.

Fixes: 654810b56828 ("common/mlx5: share Netlink commands")
Fixes: 420bbdae89f2 ("net/mlx5: fix host physical function representor naming")
Cc: stable@dpdk.org

Signed-off-by: Bing Zhao <bingz@nvidia.com>
---
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index f1ff6f49f9..55801534d1 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -1035,7 +1035,8 @@ int
 mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 {
 	char ifname[IF_NAMESIZE];
-	char port_name[IF_NAMESIZE];
+	char *port_name = NULL;
+	size_t port_name_size = 0;
 	FILE *file;
 	struct mlx5_switch_info data = {
 		.master = 0,
@@ -1048,6 +1049,7 @@ mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 	bool port_switch_id_set = false;
 	bool device_dir = false;
 	char c;
+	ssize_t line_size;
 
 	if (!if_indextoname(ifindex, ifname)) {
 		rte_errno = errno;
@@ -1063,8 +1065,21 @@ mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info *info)
 
 	file = fopen(phys_port_name, "rb");
 	if (file != NULL) {
-		if (fgets(port_name, IF_NAMESIZE, file) != NULL)
+		char *tail_nl;
+
+		line_size = getline(&port_name, &port_name_size, file);
+		if (line_size < 0) {
+			fclose(file);
+			rte_errno = errno;
+			return -rte_errno;
+		} else if (line_size > 0) {
+			/* Remove tailing newline character. */
+			tail_nl = strchr(port_name, '\n');
+			if (tail_nl)
+				*tail_nl = '\0';
 			mlx5_translate_port_name(port_name, &data);
+		}
+		free(port_name);
 		fclose(file);
 	}
 	file = fopen(phys_switch_id, "rb");
-- 
2.31.1

RE: [PATCH v3] net/mlx5: fix the sysfs port name translation

[Matan Azrad]

From: Bing Zhao 
> With some OFED or upstream kernel of mlx5, the port name fetched from
> "/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the EOL.
> The sscanf() will return the scanned items number with this EOL.
> 
> In such case, the "equal to" condition is considered as false and the function
> mlx5_translate_port_name() will recognize the port type wrongly with
> UNKNOWN result.
> 
> The tailing carriage return character should be removed before calling the
> mlx5_translate_port_name(), this was already done in the NL message
> handling. In the meanwhile, the possible incorrect line feed character is also
> taken into consideration.
> 
> Fixes: 654810b56828 ("common/mlx5: share Netlink commands")
> Fixes: 420bbdae89f2 ("net/mlx5: fix host physical function representor
> naming")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>

RE: [PATCH v3] net/mlx5: fix the sysfs port name translation

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Bing Zhao <bingz@nvidia.com>
> Sent: Thursday, March 23, 2023 12:54 PM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> <matan@nvidia.com>; stephen@networkplumber.org
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>;
> stable@dpdk.org
> Subject: [PATCH v3] net/mlx5: fix the sysfs port name translation
> 
> With some OFED or upstream kernel of mlx5, the port name fetched from
> "/sys/class/net/[DEV]/phys_port_name" may have a tailing "\n" as the
> EOL. The sscanf() will return the scanned items number with this EOL.
> 
> In such case, the "equal to" condition is considered as false and
> the function mlx5_translate_port_name() will recognize the port type
> wrongly with UNKNOWN result.
> 
> The tailing carriage return character should be removed before
> calling the mlx5_translate_port_name(), this was already done in the
> NL message handling. In the meanwhile, the possible incorrect line
> feed character is also taken into consideration.
> 
> Fixes: 654810b56828 ("common/mlx5: share Netlink commands")
> Fixes: 420bbdae89f2 ("net/mlx5: fix host physical function representor
> naming")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>

Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh