[dpdk-dev] [PATCH 0/5] small fixes for telemetry rework.

[dpdk-dev] [PATCH 0/5] small fixes for telemetry rework.

[Ciara Power]

Ciara Power (5):
  telemetry: keep telemetry threads separate from data plane
  telemetry: fix error checking for strchr function
  telemetry: fix closing socket fd on error
  telemetry: fix checking error return for socket creation
  telemetry: fix buffer overrun if max bytes read

 lib/librte_eal/freebsd/eal.c            |  3 ++-
 lib/librte_eal/linux/eal.c              |  3 ++-
 lib/librte_telemetry/rte_telemetry.h    |  3 ++-
 lib/librte_telemetry/telemetry.c        | 13 ++++++++-----
 lib/librte_telemetry/telemetry_legacy.c | 19 +++++++++++++++++--
 5 files changed, 31 insertions(+), 10 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/5] telemetry: keep telemetry threads separate from data plane

[Ciara Power]

The threads for listening on the telemetry sockets are control threads
and should be separated from those on the data plane. Since telemetry
cannot use the rte_ctrl_thread_create() API, as it does not depend on
EAL, we pass the ctrl thread cpu_set to telemetry init and use it
directly to ensure that telemetry cannot interfere with the data plane
threads.

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_eal/freebsd/eal.c         |  3 ++-
 lib/librte_eal/linux/eal.c           |  3 ++-
 lib/librte_telemetry/rte_telemetry.h |  3 ++-
 lib/librte_telemetry/telemetry.c     | 13 ++++++++-----
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c
index f681bc7a22..14b52168e2 100644
--- a/lib/librte_eal/freebsd/eal.c
+++ b/lib/librte_eal/freebsd/eal.c
@@ -958,7 +958,8 @@ rte_eal_init(int argc, char **argv)
 	if (!internal_config.no_telemetry) {
 		const char *error_str;
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
-				&error_str) != 0) {
+				&internal_config.ctrl_cpuset, &error_str)
+				!= 0) {
 			rte_eal_init_alert(error_str);
 			return -1;
 		}
diff --git a/lib/librte_eal/linux/eal.c b/lib/librte_eal/linux/eal.c
index 6123bb7c46..9620d25444 100644
--- a/lib/librte_eal/linux/eal.c
+++ b/lib/librte_eal/linux/eal.c
@@ -1296,7 +1296,8 @@ rte_eal_init(int argc, char **argv)
 	if (!internal_config.no_telemetry) {
 		const char *error_str;
 		if (rte_telemetry_init(rte_eal_get_runtime_dir(),
-				&error_str) != 0) {
+				&internal_config.ctrl_cpuset, &error_str)
+				!= 0) {
 			rte_eal_init_alert(error_str);
 			return -1;
 		}
diff --git a/lib/librte_telemetry/rte_telemetry.h b/lib/librte_telemetry/rte_telemetry.h
index 1965affba3..2c3c96cf73 100644
--- a/lib/librte_telemetry/rte_telemetry.h
+++ b/lib/librte_telemetry/rte_telemetry.h
@@ -250,6 +250,7 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
  */
 __rte_experimental
 int
-rte_telemetry_init(const char *runtime_dir, const char **err_str);
+rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
+		const char **err_str);
 
 #endif
diff --git a/lib/librte_telemetry/telemetry.c b/lib/librte_telemetry/telemetry.c
index 56a2fed3f5..7b6f8a79e4 100644
--- a/lib/librte_telemetry/telemetry.c
+++ b/lib/librte_telemetry/telemetry.c
@@ -341,7 +341,7 @@ create_socket(char *path)
 }
 
 static int
-telemetry_legacy_init(const char *runtime_dir)
+telemetry_legacy_init(const char *runtime_dir, rte_cpuset_t *cpuset)
 {
 	pthread_t t_old;
 
@@ -363,12 +363,13 @@ telemetry_legacy_init(const char *runtime_dir)
 	if (v1_socket.sock < 0)
 		return -1;
 	pthread_create(&t_old, NULL, socket_listener, &v1_socket);
+	pthread_setaffinity_np(t_old, sizeof(*cpuset), cpuset);
 
 	return 0;
 }
 
 static int
-telemetry_v2_init(const char *runtime_dir)
+telemetry_v2_init(const char *runtime_dir, rte_cpuset_t *cpuset)
 {
 	pthread_t t_new;
 
@@ -390,20 +391,22 @@ telemetry_v2_init(const char *runtime_dir)
 	if (v2_socket.sock < 0)
 		return -1;
 	pthread_create(&t_new, NULL, socket_listener, &v2_socket);
+	pthread_setaffinity_np(t_new, sizeof(*cpuset), cpuset);
 	atexit(unlink_sockets);
 
 	return 0;
 }
 
 int32_t
-rte_telemetry_init(const char *runtime_dir, const char **err_str)
+rte_telemetry_init(const char *runtime_dir, rte_cpuset_t *cpuset,
+		const char **err_str)
 {
-	if (telemetry_v2_init(runtime_dir) != 0) {
+	if (telemetry_v2_init(runtime_dir, cpuset) != 0) {
 		*err_str = telemetry_log_error;
 		printf("Error initialising telemetry - %s\n", *err_str);
 		return -1;
 	}
-	if (telemetry_legacy_init(runtime_dir) != 0) {
+	if (telemetry_legacy_init(runtime_dir, cpuset) != 0) {
 		*err_str = telemetry_log_error;
 		printf("No telemetry legacy support - %s\n", *err_str);
 	}
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/5] telemetry: keep telemetry threads separate from data plane

[Laatz, Kevin]

On 12/05/2020 16:28, Ciara Power wrote:
> The threads for listening on the telemetry sockets are control threads
> and should be separated from those on the data plane. Since telemetry
> cannot use the rte_ctrl_thread_create() API, as it does not depend on
> EAL, we pass the ctrl thread cpu_set to telemetry init and use it
> directly to ensure that telemetry cannot interfere with the data plane
> threads.
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>   lib/librte_eal/freebsd/eal.c         |  3 ++-
>   lib/librte_eal/linux/eal.c           |  3 ++-
>   lib/librte_telemetry/rte_telemetry.h |  3 ++-
>   lib/librte_telemetry/telemetry.c     | 13 ++++++++-----
>   4 files changed, 14 insertions(+), 8 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[dpdk-dev] [PATCH 2/5] telemetry: fix error checking for strchr function

[Ciara Power]

The strchr function return was not being checked which could lead to
NULL deferencing later in the function.

Coverity issue: 358438
Coverity issue: 358445
Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
Cc: ciara.power@intel.com

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c
index 8e24eb4cb9..10b575adfd 100644
--- a/lib/librte_telemetry/telemetry_legacy.c
+++ b/lib/librte_telemetry/telemetry_legacy.c
@@ -82,8 +82,16 @@ register_client(const char *cmd __rte_unused, const char *params,
 	int fd;
 	struct sockaddr_un addrs;
 
+	if (!strchr(params, ':')) {
+		fprintf(stderr, "Invalid data\n");
+		return -1;
+	}
 	strlcpy(data, strchr(params, ':'), sizeof(data));
 	memcpy(data, &data[strlen(":\"")], strlen(data));
+	if (!strchr(data, '\"')) {
+		fprintf(stderr, "Invalid client data\n");
+		return -1;
+	}
 	*strchr(data, '\"') = 0;
 
 	fd = socket(AF_UNIX, SOCK_SEQPACKET, 0);
@@ -178,6 +186,8 @@ parse_client_request(char *buffer, int buf_len, int s)
 		if (!strchr(data_ptr, '{'))
 			data_sep = data_ptr[strlen(callbacks[i].data)];
 		else {
+			if (!strchr(data_ptr, '}'))
+				return -EINVAL;
 			char *data_end = strchr(data_ptr, '}');
 			data = data_ptr + strlen(DATA_REQ_LABEL);
 			data_sep = data_end[1];
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/5] telemetry: fix error checking for strchr function

[Laatz, Kevin]

On 12/05/2020 16:28, Ciara Power wrote:
> The strchr function return was not being checked which could lead to
> NULL deferencing later in the function.
>
> Coverity issue: 358438
> Coverity issue: 358445
> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
> Cc: ciara.power@intel.com
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>   lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[dpdk-dev] [PATCH 3/5] telemetry: fix closing socket fd on error

[Ciara Power]

The socket fd is now being closed when the connection fails.

Coverity issue: 358444
Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
Cc: ciara.power@intel.com

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_telemetry/telemetry_legacy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c
index 10b575adfd..72471cbfbe 100644
--- a/lib/librte_telemetry/telemetry_legacy.c
+++ b/lib/librte_telemetry/telemetry_legacy.c
@@ -100,6 +100,7 @@ register_client(const char *cmd __rte_unused, const char *params,
 
 	if (connect(fd, (struct sockaddr *)&addrs, sizeof(addrs)) == -1) {
 		perror("\nClient connection error\n");
+		close(fd);
 		return -1;
 	}
 	pthread_create(&th, NULL, &legacy_client_handler,
-- 
2.17.1

Re: [dpdk-dev] [PATCH 3/5] telemetry: fix closing socket fd on error

[Laatz, Kevin]

On 12/05/2020 16:29, Ciara Power wrote:
> The socket fd is now being closed when the connection fails.
>
> Coverity issue: 358444
> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
> Cc: ciara.power@intel.com
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>   lib/librte_telemetry/telemetry_legacy.c | 1 +
>   1 file changed, 1 insertion(+)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[dpdk-dev] [PATCH 4/5] telemetry: fix checking error return for socket creation

[Ciara Power]

The return value from the socket function is now checked, as it can
return a negative value on error.

Coverity issue: 358443
Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
Cc: ciara.power@intel.com

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_telemetry/telemetry_legacy.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c
index 72471cbfbe..2de9021349 100644
--- a/lib/librte_telemetry/telemetry_legacy.c
+++ b/lib/librte_telemetry/telemetry_legacy.c
@@ -95,6 +95,10 @@ register_client(const char *cmd __rte_unused, const char *params,
 	*strchr(data, '\"') = 0;
 
 	fd = socket(AF_UNIX, SOCK_SEQPACKET, 0);
+	if (fd < 0) {
+		perror("Failed to open socket");
+		return -1;
+	}
 	addrs.sun_family = AF_UNIX;
 	strlcpy(addrs.sun_path, data, sizeof(addrs.sun_path));
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH 4/5] telemetry: fix checking error return for socket creation

[Laatz, Kevin]

On 12/05/2020 16:29, Ciara Power wrote:
> The return value from the socket function is now checked, as it can
> return a negative value on error.
>
> Coverity issue: 358443
> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
> Cc: ciara.power@intel.com
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>   lib/librte_telemetry/telemetry_legacy.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

[dpdk-dev] [PATCH 5/5] telemetry: fix buffer overrun if max bytes read

[Ciara Power]

If 1024 bytes were received over the socket, this caused
buffer_recvf[bytes] to overrun the array. The size of the buffer - 1 is
now passed to the read function.

Coverity issue: 358442
Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
Cc: ciara.power@intel.com

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 lib/librte_telemetry/telemetry_legacy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_telemetry/telemetry_legacy.c b/lib/librte_telemetry/telemetry_legacy.c
index 2de9021349..a341fe4ebd 100644
--- a/lib/librte_telemetry/telemetry_legacy.c
+++ b/lib/librte_telemetry/telemetry_legacy.c
@@ -217,7 +217,7 @@ legacy_client_handler(void *sock_id)
 	int ret;
 	char buffer_recv[BUF_SIZE];
 	/* receive data is not null terminated */
-	int bytes = read(s, buffer_recv, sizeof(buffer_recv));
+	int bytes = read(s, buffer_recv, sizeof(buffer_recv) - 1);
 
 	while (bytes > 0) {
 		buffer_recv[bytes] = 0;
@@ -234,7 +234,7 @@ legacy_client_handler(void *sock_id)
 			if (ret < 0)
 				printf("\nCould not send error response\n");
 		}
-		bytes = read(s, buffer_recv, sizeof(buffer_recv));
+		bytes = read(s, buffer_recv, sizeof(buffer_recv) - 1);
 	}
 	close(s);
 	return NULL;
-- 
2.17.1

Re: [dpdk-dev] [PATCH 5/5] telemetry: fix buffer overrun if max bytes read

[Laatz, Kevin]

On 12/05/2020 16:29, Ciara Power wrote:
> If 1024 bytes were received over the socket, this caused
> buffer_recvf[bytes] to overrun the array. The size of the buffer - 1 is
> now passed to the read function.
>
> Coverity issue: 358442
> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility")
> Cc: ciara.power@intel.com
>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>   lib/librte_telemetry/telemetry_legacy.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Kevin Laatz <kevin.laatz@intel.com>

Re: [dpdk-dev] [PATCH 0/5] small fixes for telemetry rework.

[Thomas Monjalon]

12/05/2020 17:28, Ciara Power:
> 
> Ciara Power (5):
>   telemetry: keep telemetry threads separate from data plane
>   telemetry: fix error checking for strchr function
>   telemetry: fix closing socket fd on error
>   telemetry: fix checking error return for socket creation
>   telemetry: fix buffer overrun if max bytes read

Applied, thanks