[dpdk-dev] [PATCH 0/3] fix VXLAN related rules cleanup and management

[dpdk-dev] [PATCH 0/3] fix VXLAN related rules cleanup and management

[Slava Ovsiienko]

The VXLAN related rule cleanup routine queries and gathers all
existing local IP and neigh rules into buffer list. One buffer
may contain multiple rule deletetion commands and is prepared
to send into Netlink as single message. But, if error occurs
for some deletion commands in the buffer, the multiple ACK
message with errors can be send back by the kernel. It breaks
the Netlink communication sequence numbers, because we expect
only one ACK message and it smashes out futher Netlik
communication.

The workaround of this problem is to send rule deletion commands
from buffer in one-by-one fashion and get ACK message for every
command sent. We do not expect too may rules preexist, so there
should not be critical performance degradation at VXLAN outer
interface initialization.

Viacheslav Ovsiienko (3):
  net/mlx5: fix buffer allocation check in rule cleanup
  net/mlx5: add Netlink message size check in rule cleanup
  net/mlx5: fix rule cleanup Netlink command sending

 drivers/net/mlx5/mlx5_flow_tcf.c | 93 +++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 48 deletions(-)

-- 
1.8.3.1

[dpdk-dev] [PATCH 1/3] net/mlx5: fix buffer allocation check in rule cleanup

[Slava Ovsiienko]

The Netlink message buffer is allocated and there is the typo,
the other pointer is checked instead of returned one. If no
memory is allocated and NULL is returned by allocation routine
the bug causes segmentation fault. The patch fixes typo,
returned pointer is validated.

Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow_tcf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
index fb817b2..ba0674a 100644
--- a/drivers/net/mlx5/mlx5_flow_tcf.c
+++ b/drivers/net/mlx5/mlx5_flow_tcf.c
@@ -4226,7 +4226,7 @@ struct tcf_nlcb_context {
 	vxindex = ifm->ifi_index;
 	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
 					MNL_ALIGN(sizeof(struct ifinfomsg)));
-	if (!nlh) {
+	if (!cmd) {
 		rte_errno = ENOMEM;
 		return -rte_errno;
 	}
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 1/3] net/mlx5: fix buffer allocation check in rule cleanup

[Yongseok Koh]

> On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> The Netlink message buffer is allocated and there is the typo,
> the other pointer is checked instead of returned one. If no
> memory is allocated and NULL is returned by allocation routine
> the bug causes segmentation fault. The patch fixes typo,
> returned pointer is validated.
> 
> Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---

Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

> drivers/net/mlx5/mlx5_flow_tcf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
> index fb817b2..ba0674a 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -4226,7 +4226,7 @@ struct tcf_nlcb_context {
> 	vxindex = ifm->ifi_index;
> 	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
> 					MNL_ALIGN(sizeof(struct ifinfomsg)));
> -	if (!nlh) {
> +	if (!cmd) {
> 		rte_errno = ENOMEM;
> 		return -rte_errno;
> 	}
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 2/3] net/mlx5: add Netlink message size check in rule cleanup

[Slava Ovsiienko]

This patch is preparation for the following fix, we are going to send
Netlink message from buffer in one-by-one fashion. It is highly
desirable to check multimessage buffer consistency for debug purposes.

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow_tcf.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
index ba0674a..bba8aed 100644
--- a/drivers/net/mlx5/mlx5_flow_tcf.c
+++ b/drivers/net/mlx5/mlx5_flow_tcf.c
@@ -3935,6 +3935,7 @@ struct tcf_nlcb_context {
 	struct nlattr *na_local = NULL;
 	struct nlattr *na_peer = NULL;
 	unsigned char family;
+	uint32_t size;
 
 	if (nlh->nlmsg_type != RTM_NEWADDR) {
 		rte_errno = EINVAL;
@@ -3962,11 +3963,11 @@ struct tcf_nlcb_context {
 	if (!na_local || !na_peer)
 		return 1;
 	/* Local rule found with scope link, permanent and assigned peer. */
-	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
-					MNL_ALIGN(sizeof(struct ifaddrmsg)) +
-					(family == AF_INET6
-					? 2 * SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
-					: 2 * SZ_NLATTR_TYPE_OF(uint32_t)));
+	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
+	       MNL_ALIGN(sizeof(struct ifaddrmsg)) +
+	       (family == AF_INET6 ? 2 * SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
+				   : 2 * SZ_NLATTR_TYPE_OF(uint32_t));
+	cmd = flow_tcf_alloc_nlcmd(ctx, size);
 	if (!cmd) {
 		rte_errno = ENOMEM;
 		return -rte_errno;
@@ -3991,6 +3992,7 @@ struct tcf_nlcb_context {
 		mnl_attr_put(cmd, IFA_ADDRESS, IPV6_ADDR_LEN,
 			mnl_attr_get_payload(na_peer));
 	}
+	assert(size == cmd->nlmsg_len);
 	return 1;
 }
 
@@ -4059,6 +4061,7 @@ struct tcf_nlcb_context {
 	struct nlattr *na_ip = NULL;
 	struct nlattr *na_mac = NULL;
 	unsigned char family;
+	uint32_t size;
 
 	if (nlh->nlmsg_type != RTM_NEWNEIGH) {
 		rte_errno = EINVAL;
@@ -4085,12 +4088,12 @@ struct tcf_nlcb_context {
 	if (!na_mac || !na_ip)
 		return 1;
 	/* Neigh rule with permenent attribute found. */
-	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
-					MNL_ALIGN(sizeof(struct ndmsg)) +
-					SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) +
-					(family == AF_INET6
-					? SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
-					: SZ_NLATTR_TYPE_OF(uint32_t)));
+	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
+	       MNL_ALIGN(sizeof(struct ndmsg)) +
+	       SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) +
+	       (family == AF_INET6 ? SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
+				   : SZ_NLATTR_TYPE_OF(uint32_t));
+	cmd = flow_tcf_alloc_nlcmd(ctx, size);
 	if (!cmd) {
 		rte_errno = ENOMEM;
 		return -rte_errno;
@@ -4113,6 +4116,7 @@ struct tcf_nlcb_context {
 	}
 	mnl_attr_put(cmd, NDA_LLADDR, ETHER_ADDR_LEN,
 		     mnl_attr_get_payload(na_mac));
+	assert(size == cmd->nlmsg_len);
 	return 1;
 }
 
@@ -4179,6 +4183,7 @@ struct tcf_nlcb_context {
 	struct nlattr *na_vxlan = NULL;
 	bool found = false;
 	unsigned int vxindex;
+	uint32_t size;
 
 	if (nlh->nlmsg_type != RTM_NEWLINK) {
 		rte_errno = EINVAL;
@@ -4224,8 +4229,9 @@ struct tcf_nlcb_context {
 		return 1;
 	/* Attached VXLAN device found, store the command to delete. */
 	vxindex = ifm->ifi_index;
-	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
-					MNL_ALIGN(sizeof(struct ifinfomsg)));
+	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
+	       MNL_ALIGN(sizeof(struct ifinfomsg));
+	cmd = flow_tcf_alloc_nlcmd(ctx, size);
 	if (!cmd) {
 		rte_errno = ENOMEM;
 		return -rte_errno;
@@ -4236,6 +4242,7 @@ struct tcf_nlcb_context {
 	ifm = mnl_nlmsg_put_extra_header(cmd, sizeof(*ifm));
 	ifm->ifi_family = AF_UNSPEC;
 	ifm->ifi_index = vxindex;
+	assert(size == cmd->nlmsg_len);
 	return 1;
 }
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 2/3] net/mlx5: add Netlink message size check in rule cleanup

[Yongseok Koh]

> On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> This patch is preparation for the following fix, we are going to send
> Netlink message from buffer in one-by-one fashion. It is highly
> desirable to check multimessage buffer consistency for debug purposes.
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---

Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

> drivers/net/mlx5/mlx5_flow_tcf.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
> index ba0674a..bba8aed 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -3935,6 +3935,7 @@ struct tcf_nlcb_context {
> 	struct nlattr *na_local = NULL;
> 	struct nlattr *na_peer = NULL;
> 	unsigned char family;
> +	uint32_t size;
> 
> 	if (nlh->nlmsg_type != RTM_NEWADDR) {
> 		rte_errno = EINVAL;
> @@ -3962,11 +3963,11 @@ struct tcf_nlcb_context {
> 	if (!na_local || !na_peer)
> 		return 1;
> 	/* Local rule found with scope link, permanent and assigned peer. */
> -	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
> -					MNL_ALIGN(sizeof(struct ifaddrmsg)) +
> -					(family == AF_INET6
> -					? 2 * SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
> -					: 2 * SZ_NLATTR_TYPE_OF(uint32_t)));
> +	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
> +	       MNL_ALIGN(sizeof(struct ifaddrmsg)) +
> +	       (family == AF_INET6 ? 2 * SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
> +				   : 2 * SZ_NLATTR_TYPE_OF(uint32_t));
> +	cmd = flow_tcf_alloc_nlcmd(ctx, size);
> 	if (!cmd) {
> 		rte_errno = ENOMEM;
> 		return -rte_errno;
> @@ -3991,6 +3992,7 @@ struct tcf_nlcb_context {
> 		mnl_attr_put(cmd, IFA_ADDRESS, IPV6_ADDR_LEN,
> 			mnl_attr_get_payload(na_peer));
> 	}
> +	assert(size == cmd->nlmsg_len);
> 	return 1;
> }
> 
> @@ -4059,6 +4061,7 @@ struct tcf_nlcb_context {
> 	struct nlattr *na_ip = NULL;
> 	struct nlattr *na_mac = NULL;
> 	unsigned char family;
> +	uint32_t size;
> 
> 	if (nlh->nlmsg_type != RTM_NEWNEIGH) {
> 		rte_errno = EINVAL;
> @@ -4085,12 +4088,12 @@ struct tcf_nlcb_context {
> 	if (!na_mac || !na_ip)
> 		return 1;
> 	/* Neigh rule with permenent attribute found. */
> -	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
> -					MNL_ALIGN(sizeof(struct ndmsg)) +
> -					SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) +
> -					(family == AF_INET6
> -					? SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
> -					: SZ_NLATTR_TYPE_OF(uint32_t)));
> +	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
> +	       MNL_ALIGN(sizeof(struct ndmsg)) +
> +	       SZ_NLATTR_DATA_OF(ETHER_ADDR_LEN) +
> +	       (family == AF_INET6 ? SZ_NLATTR_DATA_OF(IPV6_ADDR_LEN)
> +				   : SZ_NLATTR_TYPE_OF(uint32_t));
> +	cmd = flow_tcf_alloc_nlcmd(ctx, size);
> 	if (!cmd) {
> 		rte_errno = ENOMEM;
> 		return -rte_errno;
> @@ -4113,6 +4116,7 @@ struct tcf_nlcb_context {
> 	}
> 	mnl_attr_put(cmd, NDA_LLADDR, ETHER_ADDR_LEN,
> 		     mnl_attr_get_payload(na_mac));
> +	assert(size == cmd->nlmsg_len);
> 	return 1;
> }
> 
> @@ -4179,6 +4183,7 @@ struct tcf_nlcb_context {
> 	struct nlattr *na_vxlan = NULL;
> 	bool found = false;
> 	unsigned int vxindex;
> +	uint32_t size;
> 
> 	if (nlh->nlmsg_type != RTM_NEWLINK) {
> 		rte_errno = EINVAL;
> @@ -4224,8 +4229,9 @@ struct tcf_nlcb_context {
> 		return 1;
> 	/* Attached VXLAN device found, store the command to delete. */
> 	vxindex = ifm->ifi_index;
> -	cmd = flow_tcf_alloc_nlcmd(ctx, MNL_ALIGN(sizeof(struct nlmsghdr)) +
> -					MNL_ALIGN(sizeof(struct ifinfomsg)));
> +	size = MNL_ALIGN(sizeof(struct nlmsghdr)) +
> +	       MNL_ALIGN(sizeof(struct ifinfomsg));
> +	cmd = flow_tcf_alloc_nlcmd(ctx, size);
> 	if (!cmd) {
> 		rte_errno = ENOMEM;
> 		return -rte_errno;
> @@ -4236,6 +4242,7 @@ struct tcf_nlcb_context {
> 	ifm = mnl_nlmsg_put_extra_header(cmd, sizeof(*ifm));
> 	ifm->ifi_family = AF_UNSPEC;
> 	ifm->ifi_index = vxindex;
> +	assert(size == cmd->nlmsg_len);
> 	return 1;
> }
> 
> -- 
> 1.8.3.1
> 

[dpdk-dev] [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending

[Slava Ovsiienko]

The VXLAN related rule cleanup routine queries and gathers all
existing local IP and neigh rules into buffer list. One buffer
may contain multiple rule deletetion commands and is prepared
to send into Netlink as single message. But, if error occurs
for some deletion commands in the buffer, the multiple ACK
message with errors can be send back by the kernel. It breaks
the Netlink communication sequence numbers, because we expect
only one ACK message and it smashes out futher Netlik
communication.

The workaround of this problem is to send rule deletion commands
from buffer in one-by-one fashion and get ACK message for every
command sent. We do not expect too may rules preexist, so there
should not be critical performance degradation at VXLAN outer
interface initialization.

Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow_tcf.c | 58 +++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
index bba8aed..21eb99e 100644
--- a/drivers/net/mlx5/mlx5_flow_tcf.c
+++ b/drivers/net/mlx5/mlx5_flow_tcf.c
@@ -3847,30 +3847,6 @@ struct tcf_nlcb_context {
 }
 
 /**
- * Set NLM_F_ACK flags in the last netlink command in buffer.
- * Only last command in the buffer will be acked by system.
- *
- * @param[in, out] buf
- *   Pointer to buffer with netlink commands.
- */
-static void
-flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf)
-{
-	struct nlmsghdr *nlh;
-	uint32_t size = 0;
-
-	assert(buf->size);
-	do {
-		nlh = (struct nlmsghdr *)&buf->msg[size];
-		size += NLMSG_ALIGN(nlh->nlmsg_len);
-		if (size >= buf->size) {
-			nlh->nlmsg_flags |= NLM_F_ACK;
-			break;
-		}
-	} while (true);
-}
-
-/**
  * Send the buffers with prepared netlink commands. Scans the list and
  * sends all found buffers. Buffers are sent and freed anyway in order
  * to prevent memory leakage if some every message in received packet.
@@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
 flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
 		    struct tcf_nlcb_context *ctx)
 {
-	struct tcf_nlcb_buf *bc, *bn;
-	struct nlmsghdr *nlh;
+	struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
 	int ret = 0;
 
-	bc = LIST_FIRST(&ctx->nlbuf);
 	while (bc) {
+		struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
+		struct nlmsghdr *nlh;
+		uint32_t msg = 0;
 		int rc;
 
-		bn = LIST_NEXT(bc, next);
-		if (bc->size) {
-			flow_tcf_setack_nlcmd(bc);
-			nlh = (struct nlmsghdr *)&bc->msg;
-			rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
-			if (rc && !ret)
-				ret = rc;
+		while (msg < bc->size) {
+			/*
+			 * Send Netlink commands from buffer in one by one
+			 * fashion. If we send multiple rule deletion commands
+			 * in one Netlink message and some error occurs it may
+			 * cause multiple ACK error messages and break sequence
+			 * numbers of Netlink communication, because we expect
+			 * the only one ACK reply.
+			 */
+			assert((bc->size - msg) >= sizeof(struct nlmsghdr));
+			nlh = (struct nlmsghdr *)&bc->msg[msg];
+			assert((bc->size - msg) >= nlh->nlmsg_len);
+			msg += nlh->nlmsg_len;
+			rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
+			if (rc) {
+				DRV_LOG(WARNING,
+					"netlink: cleanup error %d", rc);
+				if (!ret)
+					ret = rc;
+			}
 		}
 		rte_free(bc);
 		bc = bn;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending

[Yongseok Koh]

> On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> The VXLAN related rule cleanup routine queries and gathers all
> existing local IP and neigh rules into buffer list. One buffer
> may contain multiple rule deletetion commands and is prepared
> to send into Netlink as single message. But, if error occurs
> for some deletion commands in the buffer, the multiple ACK
> message with errors can be send back by the kernel. It breaks
> the Netlink communication sequence numbers, because we expect
> only one ACK message and it smashes out futher Netlik
> communication.

Just curious.
Is parsing the multiple ack msgs more complex than sending commands
one by one?

> The workaround of this problem is to send rule deletion commands
> from buffer in one-by-one fashion and get ACK message for every
> command sent. We do not expect too may rules preexist, so there
> should not be critical performance degradation at VXLAN outer
> interface initialization.
> 
> Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup routines")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> ---

Acked-by: Yongseok Koh <yskoh@mellanox.com>
 
Thanks

> drivers/net/mlx5/mlx5_flow_tcf.c | 58 +++++++++++++++++-----------------------
> 1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c b/drivers/net/mlx5/mlx5_flow_tcf.c
> index bba8aed..21eb99e 100644
> --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> @@ -3847,30 +3847,6 @@ struct tcf_nlcb_context {
> }
> 
> /**
> - * Set NLM_F_ACK flags in the last netlink command in buffer.
> - * Only last command in the buffer will be acked by system.
> - *
> - * @param[in, out] buf
> - *   Pointer to buffer with netlink commands.
> - */
> -static void
> -flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf)
> -{
> -	struct nlmsghdr *nlh;
> -	uint32_t size = 0;
> -
> -	assert(buf->size);
> -	do {
> -		nlh = (struct nlmsghdr *)&buf->msg[size];
> -		size += NLMSG_ALIGN(nlh->nlmsg_len);
> -		if (size >= buf->size) {
> -			nlh->nlmsg_flags |= NLM_F_ACK;
> -			break;
> -		}
> -	} while (true);
> -}
> -
> -/**
>  * Send the buffers with prepared netlink commands. Scans the list and
>  * sends all found buffers. Buffers are sent and freed anyway in order
>  * to prevent memory leakage if some every message in received packet.
> @@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
> flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
> 		    struct tcf_nlcb_context *ctx)
> {
> -	struct tcf_nlcb_buf *bc, *bn;
> -	struct nlmsghdr *nlh;
> +	struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
> 	int ret = 0;
> 
> -	bc = LIST_FIRST(&ctx->nlbuf);
> 	while (bc) {
> +		struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
> +		struct nlmsghdr *nlh;
> +		uint32_t msg = 0;
> 		int rc;
> 
> -		bn = LIST_NEXT(bc, next);
> -		if (bc->size) {
> -			flow_tcf_setack_nlcmd(bc);
> -			nlh = (struct nlmsghdr *)&bc->msg;
> -			rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
> -			if (rc && !ret)
> -				ret = rc;
> +		while (msg < bc->size) {
> +			/*
> +			 * Send Netlink commands from buffer in one by one
> +			 * fashion. If we send multiple rule deletion commands
> +			 * in one Netlink message and some error occurs it may
> +			 * cause multiple ACK error messages and break sequence
> +			 * numbers of Netlink communication, because we expect
> +			 * the only one ACK reply.
> +			 */
> +			assert((bc->size - msg) >= sizeof(struct nlmsghdr));
> +			nlh = (struct nlmsghdr *)&bc->msg[msg];
> +			assert((bc->size - msg) >= nlh->nlmsg_len);
> +			msg += nlh->nlmsg_len;
> +			rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
> +			if (rc) {
> +				DRV_LOG(WARNING,
> +					"netlink: cleanup error %d", rc);
> +				if (!ret)
> +					ret = rc;
> +			}
> 		}
> 		rte_free(bc);
> 		bc = bn;
> -- 
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending

[Slava Ovsiienko]

> -----Original Message-----
> From: Yongseok Koh
> Sent: Sunday, November 11, 2018 13:42
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> Subject: Re: [PATCH 3/3] net/mlx5: fix rule cleanup Netlink command sending
> 
> 
> > On Nov 10, 2018, at 1:59 AM, Slava Ovsiienko
> <viacheslavo@mellanox.com> wrote:
> >
> > The VXLAN related rule cleanup routine queries and gathers all
> > existing local IP and neigh rules into buffer list. One buffer may
> > contain multiple rule deletetion commands and is prepared to send into
> > Netlink as single message. But, if error occurs for some deletion
> > commands in the buffer, the multiple ACK message with errors can be
> > send back by the kernel. It breaks the Netlink communication sequence
> > numbers, because we expect only one ACK message and it smashes out
> > futher Netlik communication.
> 
> Just curious.
> Is parsing the multiple ack msgs more complex than sending commands one
> by one?

We are in the midst of send query/get dump process. We can't send
another request and wait ack for it - we are receiving the dump. Possible,
it can be done via creation one  more Netlink socket, but I'm not sure the
requests are not queued by kernel. So - the simplest way - gather dump
and then send commands.

PS. Actually I have refactored gathering/sending, we need to gather
parameters only, not build entire commands in callbacks, but this patch
is not tested yet and too large as for simple fix. 

WBR,
Slava
> 
> > The workaround of this problem is to send rule deletion commands from
> > buffer in one-by-one fashion and get ACK message for every command
> > sent. We do not expect too may rules preexist, so there should not be
> > critical performance degradation at VXLAN outer interface
> > initialization.
> >
> > Fixes: f420f03d6772 ("net/mlx5: add E-switch VXLAN rule cleanup
> > routines")
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> 
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> 
> Thanks
> 
> > drivers/net/mlx5/mlx5_flow_tcf.c | 58
> > +++++++++++++++++-----------------------
> > 1 file changed, 24 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_flow_tcf.c
> > b/drivers/net/mlx5/mlx5_flow_tcf.c
> > index bba8aed..21eb99e 100644
> > --- a/drivers/net/mlx5/mlx5_flow_tcf.c
> > +++ b/drivers/net/mlx5/mlx5_flow_tcf.c
> > @@ -3847,30 +3847,6 @@ struct tcf_nlcb_context { }
> >
> > /**
> > - * Set NLM_F_ACK flags in the last netlink command in buffer.
> > - * Only last command in the buffer will be acked by system.
> > - *
> > - * @param[in, out] buf
> > - *   Pointer to buffer with netlink commands.
> > - */
> > -static void
> > -flow_tcf_setack_nlcmd(struct tcf_nlcb_buf *buf) -{
> > -	struct nlmsghdr *nlh;
> > -	uint32_t size = 0;
> > -
> > -	assert(buf->size);
> > -	do {
> > -		nlh = (struct nlmsghdr *)&buf->msg[size];
> > -		size += NLMSG_ALIGN(nlh->nlmsg_len);
> > -		if (size >= buf->size) {
> > -			nlh->nlmsg_flags |= NLM_F_ACK;
> > -			break;
> > -		}
> > -	} while (true);
> > -}
> > -
> > -/**
> >  * Send the buffers with prepared netlink commands. Scans the list and
> >  * sends all found buffers. Buffers are sent and freed anyway in order
> >  * to prevent memory leakage if some every message in received packet.
> > @@ -3888,21 +3864,35 @@ struct tcf_nlcb_context {
> > flow_tcf_send_nlcmd(struct mlx5_flow_tcf_context *tcf,
> > 		    struct tcf_nlcb_context *ctx)
> > {
> > -	struct tcf_nlcb_buf *bc, *bn;
> > -	struct nlmsghdr *nlh;
> > +	struct tcf_nlcb_buf *bc = LIST_FIRST(&ctx->nlbuf);
> > 	int ret = 0;
> >
> > -	bc = LIST_FIRST(&ctx->nlbuf);
> > 	while (bc) {
> > +		struct tcf_nlcb_buf *bn = LIST_NEXT(bc, next);
> > +		struct nlmsghdr *nlh;
> > +		uint32_t msg = 0;
> > 		int rc;
> >
> > -		bn = LIST_NEXT(bc, next);
> > -		if (bc->size) {
> > -			flow_tcf_setack_nlcmd(bc);
> > -			nlh = (struct nlmsghdr *)&bc->msg;
> > -			rc = flow_tcf_nl_ack(tcf, nlh, bc->size, NULL, NULL);
> > -			if (rc && !ret)
> > -				ret = rc;
> > +		while (msg < bc->size) {
> > +			/*
> > +			 * Send Netlink commands from buffer in one by one
> > +			 * fashion. If we send multiple rule deletion
> commands
> > +			 * in one Netlink message and some error occurs it
> may
> > +			 * cause multiple ACK error messages and break
> sequence
> > +			 * numbers of Netlink communication, because we
> expect
> > +			 * the only one ACK reply.
> > +			 */
> > +			assert((bc->size - msg) >= sizeof(struct nlmsghdr));
> > +			nlh = (struct nlmsghdr *)&bc->msg[msg];
> > +			assert((bc->size - msg) >= nlh->nlmsg_len);
> > +			msg += nlh->nlmsg_len;
> > +			rc = flow_tcf_nl_ack(tcf, nlh, 0, NULL, NULL);
> > +			if (rc) {
> > +				DRV_LOG(WARNING,
> > +					"netlink: cleanup error %d", rc);
> > +				if (!ret)
> > +					ret = rc;
> > +			}
> > 		}
> > 		rte_free(bc);
> > 		bc = bn;
> > --
> > 1.8.3.1
> >

Re: [dpdk-dev] [PATCH 0/3] fix VXLAN related rules cleanup and management

[Shahaf Shuler]

Saturday, November 10, 2018 11:59 AM, Slava Ovsiienko:
> Subject: [PATCH 0/3] fix VXLAN related rules cleanup and management
> 
> The VXLAN related rule cleanup routine queries and gathers all existing local
> IP and neigh rules into buffer list. One buffer may contain multiple rule
> deletetion commands and is prepared to send into Netlink as single message.
> But, if error occurs for some deletion commands in the buffer, the multiple
> ACK message with errors can be send back by the kernel. It breaks the
> Netlink communication sequence numbers, because we expect only one
> ACK message and it smashes out futher Netlik communication.
> 
> The workaround of this problem is to send rule deletion commands from
> buffer in one-by-one fashion and get ACK message for every command sent.
> We do not expect too may rules preexist, so there should not be critical
> performance degradation at VXLAN outer interface initialization.
> 
> Viacheslav Ovsiienko (3):
>   net/mlx5: fix buffer allocation check in rule cleanup
>   net/mlx5: add Netlink message size check in rule cleanup
>   net/mlx5: fix rule cleanup Netlink command sending

Series applied to next-net-mlx, thanks.