[nft PATCH 0/4] Fix ENOBUFS error in large transactions with --echo

[nft PATCH 0/4] Fix ENOBUFS error in large transactions with --echo

[Phil Sutter]

When committing a larger transaction (e.g. adding 300 rules) with echo
output turned on, mnl_batch_talk() would report ENOBUFS after the first
call to mnl_socket_recvfrom(). (ENOBUFS indicates congestion in netlink
socket.)

The problem in mnl_batch_talk() was a combination of unmodified socket
recv buffer, use of select() and unhandled ENOBUFS condition (abort
instead of retry).

This series solves the issue, admittedly a bit in sledge hammer method:
Maximize nf_sock receive buffer size for all users, make
mnl_batch_talk() fetch more messages at once and retry upon ENOBUFS
instead of just giving up.

There was also a problem with select() use which motivated the loop
rewrite in Patch 3. Actually, replacing the whole loop by a simple call
to nft_mnl_recv() worked and was even sufficient in avoiding ENOBUFS
condition, but I am not sure if that has other side-effects.

Phil Sutter (4):
  mnl: Maximize socket receive buffer by default
  mnl: Increase receive buffer in mnl_batch_talk()
  mnl: Fix and simplify mnl_batch_talk()
  tests/shell: Test large transaction with echo output

 src/mnl.c                                     | 82 ++++++++++---------
 tests/shell/testcases/transactions/0049huge_0 | 14 ++++
 2 files changed, 58 insertions(+), 38 deletions(-)
 create mode 100755 tests/shell/testcases/transactions/0049huge_0

-- 
2.21.0

[nft PATCH 1/4] mnl: Maximize socket receive buffer by default

[Phil Sutter]

With --echo option, regular commands may receive large replies just like
'nft monitor' does. Avoid buffer overruns and message loss by maximizing
the global nf_sock's receive buffer size upon creating, not just when
calling mnl_nft_event_listener.

Error reporting is tricky in nft_mnl_socket_open(), also being warned
about failures during receive buffer increase adds little value to the
user. So just fail silently instead.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 9bb712adfa3b5..2c5a26a5e3465 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -33,6 +33,26 @@
 #include <utils.h>
 #include <nftables.h>
 
+#define NFTABLES_NLEVENT_BUFSIZ	(1 << 24)
+
+static void maximize_recv_buffer(struct mnl_socket *nf_sock)
+{
+	unsigned int bufsiz = NFTABLES_NLEVENT_BUFSIZ;
+	int fd = mnl_socket_get_fd(nf_sock);
+
+	/* Set netlink socket buffer size to 16 Mbytes to reduce chances of
+	 * message loss due to ENOBUFS.
+	 */
+	if (setsockopt(fd, SOL_SOCKET, SO_RCVBUFFORCE,
+		       &bufsiz, sizeof(socklen_t)) < 0) {
+		/* If this doesn't work, try to reach the system wide maximum
+		 * (or whatever the user requested).
+		 */
+		setsockopt(fd, SOL_SOCKET, SO_RCVBUF,
+			   &bufsiz, sizeof(socklen_t));
+	}
+}
+
 struct mnl_socket *nft_mnl_socket_open(void)
 {
 	struct mnl_socket *nf_sock;
@@ -44,6 +64,7 @@ struct mnl_socket *nft_mnl_socket_open(void)
 	if (fcntl(mnl_socket_get_fd(nf_sock), F_SETFL, O_NONBLOCK))
 		netlink_init_error();
 
+	maximize_recv_buffer(nf_sock);
 	return nf_sock;
 }
 
@@ -1379,37 +1400,17 @@ int mnl_nft_flowtable_del(struct netlink_ctx *ctx, const struct cmd *cmd)
 /*
  * events
  */
-#define NFTABLES_NLEVENT_BUFSIZ	(1 << 24)
 
 int mnl_nft_event_listener(struct mnl_socket *nf_sock, unsigned int debug_mask,
 			   struct output_ctx *octx,
 			   int (*cb)(const struct nlmsghdr *nlh, void *data),
 			   void *cb_data)
 {
-	/* Set netlink socket buffer size to 16 Mbytes to reduce chances of
- 	 * message loss due to ENOBUFS.
-	 */
-	unsigned int bufsiz = NFTABLES_NLEVENT_BUFSIZ;
 	int fd = mnl_socket_get_fd(nf_sock);
 	char buf[NFT_NLMSG_MAXSIZE];
 	fd_set readfds;
 	int ret;
 
-	ret = setsockopt(fd, SOL_SOCKET, SO_RCVBUFFORCE, &bufsiz,
-			 sizeof(socklen_t));
-	if (ret < 0) {
-		/* If this doesn't work, try to reach the system wide maximum
-		 * (or whatever the user requested).
-		 */
-		ret = setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &bufsiz,
-				 sizeof(socklen_t));
-		if (ret < 0)
-			nft_print(octx, "# Cannot increase netlink socket buffer size, expect message loss\n");
-		else
-			nft_print(octx, "# Cannot set up netlink socket buffer size to %u bytes, falling back to %u bytes\n",
-				  NFTABLES_NLEVENT_BUFSIZ, bufsiz);
-	}
-
 	while (1) {
 		FD_ZERO(&readfds);
 		FD_SET(fd, &readfds);
-- 
2.21.0

[nft PATCH 2/4] mnl: Increase receive buffer in mnl_batch_talk()

[Phil Sutter]

Be prepared to receive larger messages for the same reason as in
nft_mnl_recv() and mnl_nft_event_listener().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mnl.c b/src/mnl.c
index 2c5a26a5e3465..06280aa2cb50a 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -287,7 +287,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 {
 	struct mnl_socket *nl = ctx->nft->nf_sock;
 	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
-	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
+	char rcv_buf[NFT_NLMSG_MAXSIZE];
 	fd_set readfds;
 	struct timeval tv = {
 		.tv_sec		= 0,
-- 
2.21.0

[nft PATCH 3/4] mnl: Fix and simplify mnl_batch_talk()

[Phil Sutter]

Use of select() after the first call to mnl_socket_recvfrom() was
incorrect, FD_SET() was called after the call to select() returned. This
effectively turned the FD_ISSET() check into a noop (always true
condition).

Rewrite the receive loop using mnl_nft_event_listener() as an example:

* Combine the two calls to FD_ZERO(), FD_SET() and select() into one at
  loop start.
* Check ENOBUFS condition and warn the user, also upon other errors.
* Continue on ENOBUFS, it is not a permanent error.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/mnl.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/mnl.c b/src/mnl.c
index 06280aa2cb50a..4fbfd059c0228 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -299,34 +299,39 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
 	if (ret == -1)
 		return -1;
 
-	FD_ZERO(&readfds);
-	FD_SET(fd, &readfds);
+	while (true) {
+		FD_ZERO(&readfds);
+		FD_SET(fd, &readfds);
 
-	/* receive and digest all the acknowledgments from the kernel. */
-	ret = select(fd+1, &readfds, NULL, NULL, &tv);
-	if (ret == -1)
-		return -1;
+		/* receive and digest all the acknowledgments from the kernel. */
+		ret = select(fd + 1, &readfds, NULL, NULL, &tv);
+		if (ret < 0)
+			return -1;
 
-	while (ret > 0 && FD_ISSET(fd, &readfds)) {
-		struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
+		if (!FD_ISSET(fd, &readfds))
+			break;
 
 		ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
-		if (ret == -1)
-			return -1;
+		if (ret < 0) {
+			if (errno == ENOBUFS) {
+				nft_print(&ctx->nft->output,
+					  "# ERROR: We lost some netlink events!\n");
+				continue;
+			}
+			nft_print(&ctx->nft->output, "# ERROR: %s\n",
+				  strerror(errno));
+			err = ret;
+			break;
+		}
 
 		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
 		/* Continue on error, make sure we get all acknowledgments */
 		if (ret == -1) {
+			struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
+
 			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
 			err = -1;
 		}
-
-		ret = select(fd+1, &readfds, NULL, NULL, &tv);
-		if (ret == -1)
-			return -1;
-
-		FD_ZERO(&readfds);
-		FD_SET(fd, &readfds);
 	}
 	return err;
 }
-- 
2.21.0

[nft PATCH 4/4] tests/shell: Test large transaction with echo output

[Phil Sutter]

This reliably triggered ENOBUFS condition in mnl_batch_talk(). With the
past changes, it passes even after increasing the number of rules to
300k.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/shell/testcases/transactions/0049huge_0 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100755 tests/shell/testcases/transactions/0049huge_0

diff --git a/tests/shell/testcases/transactions/0049huge_0 b/tests/shell/testcases/transactions/0049huge_0
new file mode 100755
index 0000000000000..12338087c63e0
--- /dev/null
+++ b/tests/shell/testcases/transactions/0049huge_0
@@ -0,0 +1,14 @@
+#!/bin/bash
+
+# let's try to exceed transaction buffer space
+
+$NFT flush ruleset
+$NFT add table inet test
+$NFT add chain inet test c
+
+RULESET=$(
+for ((i = 0; i < 3000; i++)); do
+	echo "add rule inet test c accept comment rule$i"
+done
+)
+$NFT -e -f - <<< "$RULESET" >/dev/null
-- 
2.21.0

Re: [nft PATCH 3/4] mnl: Fix and simplify mnl_batch_talk()

[Pablo Neira Ayuso]

On Wed, May 29, 2019 at 03:13:45PM +0200, Phil Sutter wrote:
> Use of select() after the first call to mnl_socket_recvfrom() was
> incorrect, FD_SET() was called after the call to select() returned. This
> effectively turned the FD_ISSET() check into a noop (always true
> condition).

Good catch.

> Rewrite the receive loop using mnl_nft_event_listener() as an example:
> 
> * Combine the two calls to FD_ZERO(), FD_SET() and select() into one at
>   loop start.
> * Check ENOBUFS condition and warn the user, also upon other errors.
> * Continue on ENOBUFS, it is not a permanent error.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/mnl.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 06280aa2cb50a..4fbfd059c0228 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -299,34 +299,39 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
>  	if (ret == -1)
>  		return -1;
>  
> -	FD_ZERO(&readfds);
> -	FD_SET(fd, &readfds);
> +	while (true) {
> +		FD_ZERO(&readfds);
> +		FD_SET(fd, &readfds);
>  
> -	/* receive and digest all the acknowledgments from the kernel. */
> -	ret = select(fd+1, &readfds, NULL, NULL, &tv);
> -	if (ret == -1)
> -		return -1;
> +		/* receive and digest all the acknowledgments from the kernel. */
> +		ret = select(fd + 1, &readfds, NULL, NULL, &tv);
> +		if (ret < 0)
> +			return -1;
>  
> -	while (ret > 0 && FD_ISSET(fd, &readfds)) {
> -		struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
> +		if (!FD_ISSET(fd, &readfds))
> +			break;
>  
>  		ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
> -		if (ret == -1)
> -			return -1;
> +		if (ret < 0) {
> +			if (errno == ENOBUFS) {
> +				nft_print(&ctx->nft->output,
> +					  "# ERROR: We lost some netlink events!\n");

Probably better handling this from nft_netlink().

Could you just fix the problem you report above? Then, we make another
pass on this ENOBUFS error.

> +				continue;
> +			}
> +			nft_print(&ctx->nft->output, "# ERROR: %s\n",
> +				  strerror(errno));
> +			err = ret;
> +			break;
> +		}
>  
>  		ret = mnl_cb_run(rcv_buf, ret, 0, portid, &netlink_echo_callback, ctx);
>  		/* Continue on error, make sure we get all acknowledgments */
>  		if (ret == -1) {
> +			struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
> +
>  			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
>  			err = -1;
>  		}
> -
> -		ret = select(fd+1, &readfds, NULL, NULL, &tv);
> -		if (ret == -1)
> -			return -1;
> -
> -		FD_ZERO(&readfds);
> -		FD_SET(fd, &readfds);
>  	}
>  	return err;
>  }
> -- 
> 2.21.0
> 

Re: [nft PATCH 0/4] Fix ENOBUFS error in large transactions with --echo

[Pablo Neira Ayuso]

On Wed, May 29, 2019 at 03:13:42PM +0200, Phil Sutter wrote:
> When committing a larger transaction (e.g. adding 300 rules) with echo
> output turned on, mnl_batch_talk() would report ENOBUFS after the first
> call to mnl_socket_recvfrom(). (ENOBUFS indicates congestion in netlink
> socket.)

We can avoid this if we select the right buffer size for the --echo
case, to make this reliable. For events, that's a different case,
there is not much we can do in case this hits ENOBUFS, since we don't
know how much information the kernel will send to us, so we can just
report message losts to the users.

> The problem in mnl_batch_talk() was a combination of unmodified socket
> recv buffer, use of select() and unhandled ENOBUFS condition (abort
> instead of retry).
> 
> This series solves the issue, admittedly a bit in sledge hammer method:
> Maximize nf_sock receive buffer size for all users, make
> mnl_batch_talk() fetch more messages at once and retry upon ENOBUFS
> instead of just giving up.

Setting a fixed size works around the problem, yes. But still we will
hit ENOBUFS at some point. I sent you a patch to start estimating the
size of the receiver buffer size in a simple way.

> There was also a problem with select() use which motivated the loop
> rewrite in Patch 3.

Please, send a patch to fix this, thanks!

> Actually, replacing the whole loop by a simple call to
> nft_mnl_recv() worked and was even sufficient in avoiding ENOBUFS
> condition, but I am not sure if that has other side-effects.

Not sure what you mean.

>  tests/shell/testcases/transactions/0049huge_0 | 14 ++++

Thanks for this testcase.

Re: [nft PATCH 2/4] mnl: Increase receive buffer in mnl_batch_talk()

[Pablo Neira Ayuso]

On Wed, May 29, 2019 at 03:13:44PM +0200, Phil Sutter wrote:
> Be prepared to receive larger messages for the same reason as in
> nft_mnl_recv() and mnl_nft_event_listener().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/mnl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 2c5a26a5e3465..06280aa2cb50a 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -287,7 +287,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
>  {
>  	struct mnl_socket *nl = ctx->nft->nf_sock;
>  	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
> -	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
> +	char rcv_buf[NFT_NLMSG_MAXSIZE];

Right, this needs to be done.

I think I need to enhance my code to estimate the buffer size, through
number of commands and the average message size that is sent to the
kernel. This can be extracted from the batch.

>  	fd_set readfds;
>  	struct timeval tv = {
>  		.tv_sec		= 0,
> -- 
> 2.21.0
> 

Re: [nft PATCH 2/4] mnl: Increase receive buffer in mnl_batch_talk()

[Pablo Neira Ayuso]

On Wed, May 29, 2019 at 03:13:44PM +0200, Phil Sutter wrote:
> Be prepared to receive larger messages for the same reason as in
> nft_mnl_recv() and mnl_nft_event_listener().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/mnl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 2c5a26a5e3465..06280aa2cb50a 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -287,7 +287,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
>  {
>  	struct mnl_socket *nl = ctx->nft->nf_sock;
>  	int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
> -	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
> +	char rcv_buf[NFT_NLMSG_MAXSIZE];

Revisiting this:

The kernel uses NLMSG_GOODSIZE for events and also for the echo
message, so MNL_SOCKET_BUFFER_SIZE should be fine.

Re: [nft PATCH 4/4] tests/shell: Test large transaction with echo output

[Pablo Neira Ayuso]

On Wed, May 29, 2019 at 03:13:46PM +0200, Phil Sutter wrote:
> This reliably triggered ENOBUFS condition in mnl_batch_talk(). With the
> past changes, it passes even after increasing the number of rules to
> 300k.

Applied, thanks.