* [PATCH nft,v2 2/7] mnl: mnl_set_rcvbuffer() skips buffer size update if it is too small
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 3/7] mnl: call mnl_set_sndbuffer() from mnl_batch_talk() Pablo Neira Ayuso
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
Check for existing buffer size, if this is larger than the requested new
buffer size, skip the buffer size update.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/mnl.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/mnl.c b/src/mnl.c
index 288a887df097..a84a6a609333 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -235,8 +235,15 @@ static void mnl_set_sndbuffer(const struct mnl_socket *nl,
static int mnl_set_rcvbuffer(const struct mnl_socket *nl, size_t bufsiz)
{
+ size_t cur_bufsiz;
+ socklen_t len;
int ret;
+ ret = getsockopt(mnl_socket_get_fd(nl), SOL_SOCKET, SO_RCVBUF,
+ &cur_bufsiz, &len);
+ if (cur_bufsiz > bufsiz)
+ return 0;
+
ret = setsockopt(mnl_socket_get_fd(nl), SOL_SOCKET, SO_RCVBUFFORCE,
&bufsiz, sizeof(socklen_t));
if (ret < 0) {
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH nft,v2 3/7] mnl: call mnl_set_sndbuffer() from mnl_batch_talk()
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 2/7] mnl: mnl_set_rcvbuffer() skips buffer size update if it is too small Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 4/7] mnl: add mnl_nft_batch_to_msg() Pablo Neira Ayuso
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
Instead of mnl_nft_socket_sendmsg(), just a cleanup.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/mnl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/mnl.c b/src/mnl.c
index a84a6a609333..b3999d5f1d9f 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -272,7 +272,6 @@ static ssize_t mnl_nft_socket_sendmsg(const struct netlink_ctx *ctx)
};
uint32_t i;
- mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
nftnl_batch_iovec(ctx->batch, iov, iov_len);
for (i = 0; i < iov_len; i++) {
@@ -298,6 +297,8 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
};
int err = 0;
+ mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
+
ret = mnl_nft_socket_sendmsg(ctx);
if (ret == -1)
return -1;
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH nft,v2 4/7] mnl: add mnl_nft_batch_to_msg()
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 2/7] mnl: mnl_set_rcvbuffer() skips buffer size update if it is too small Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 3/7] mnl: call mnl_set_sndbuffer() from mnl_batch_talk() Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 5/7] mnl: estimate receiver buffer size Pablo Neira Ayuso
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
This function transforms the batch into a msghdr object.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/mnl.c | 54 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 36 insertions(+), 18 deletions(-)
diff --git a/src/mnl.c b/src/mnl.c
index b3999d5f1d9f..6c85b1855c86 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -257,49 +257,67 @@ static int mnl_set_rcvbuffer(const struct mnl_socket *nl, size_t bufsiz)
return ret;
}
-static ssize_t mnl_nft_socket_sendmsg(const struct netlink_ctx *ctx)
+static size_t mnl_nft_batch_to_msg(struct netlink_ctx *ctx, struct msghdr *msg,
+ const struct sockaddr_nl *snl,
+ struct iovec *iov, unsigned int iov_len)
{
- static const struct sockaddr_nl snl = {
- .nl_family = AF_NETLINK
- };
- uint32_t iov_len = nftnl_batch_iovec_len(ctx->batch);
- struct iovec iov[iov_len];
- struct msghdr msg = {
- .msg_name = (struct sockaddr *) &snl,
- .msg_namelen = sizeof(snl),
- .msg_iov = iov,
- .msg_iovlen = iov_len,
- };
- uint32_t i;
+ unsigned int i;
+ size_t len = 0;
+
+ msg->msg_name = (struct sockaddr_nl *)snl;
+ msg->msg_namelen = sizeof(*snl);
+ msg->msg_iov = iov;
+ msg->msg_iovlen = iov_len;
nftnl_batch_iovec(ctx->batch, iov, iov_len);
- for (i = 0; i < iov_len; i++) {
- if (ctx->nft->debug_mask & NFT_DEBUG_MNL) {
+ for (i = 0; i < iov_len; i++)
+ len += msg->msg_iov[i].iov_len;
+
+ return len;
+}
+
+static ssize_t mnl_nft_socket_sendmsg(struct netlink_ctx *ctx,
+ const struct msghdr *msg)
+{
+ uint32_t iov_len = msg->msg_iovlen;
+ struct iovec *iov = msg->msg_iov;
+ unsigned int i;
+
+ if (ctx->nft->debug_mask & NFT_DEBUG_MNL) {
+ for (i = 0; i < iov_len; i++) {
mnl_nlmsg_fprintf(ctx->nft->output.output_fp,
iov[i].iov_base, iov[i].iov_len,
sizeof(struct nfgenmsg));
}
}
- return sendmsg(mnl_socket_get_fd(ctx->nft->nf_sock), &msg, 0);
+ return sendmsg(mnl_socket_get_fd(ctx->nft->nf_sock), msg, 0);
}
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);
+ uint32_t iov_len = nftnl_batch_iovec_len(ctx->batch);
char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
- fd_set readfds;
+ const struct sockaddr_nl snl = {
+ .nl_family = AF_NETLINK
+ };
struct timeval tv = {
.tv_sec = 0,
.tv_usec = 0
};
+ fd_set readfds;
+ struct iovec iov[iov_len];
+ struct msghdr msg = {};
int err = 0;
mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
- ret = mnl_nft_socket_sendmsg(ctx);
+ mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
+
+ ret = mnl_nft_socket_sendmsg(ctx, &msg);
if (ret == -1)
return -1;
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH nft,v2 5/7] mnl: estimate receiver buffer size
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
` (2 preceding siblings ...)
2019-05-30 10:55 ` [PATCH nft,v2 4/7] mnl: add mnl_nft_batch_to_msg() Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
2019-05-31 18:11 ` Eric Garver
2019-05-30 10:55 ` [PATCH nft,v2 6/7] mnl: mnl_batch_talk() returns -1 on internal netlink errors Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 7/7] erec: remove double \n on error when internal_netlink is used Pablo Neira Ayuso
5 siblings, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
Set a receiver buffer size based on the number of commands and the
average message size, this is useful for the --echo option in order to
avoid ENOBUFS errors.
Double the estimated size is used to ensure enough receiver buffer
space.
Skip buffer receiver logic if estimation is smaller than current buffer.
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/mnl.h | 3 ++-
src/libnftables.c | 5 +++--
src/mnl.c | 11 ++++++++---
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/mnl.h b/include/mnl.h
index c63a7e7fd73a..9f50c3da0f3a 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -25,7 +25,8 @@ bool mnl_batch_ready(struct nftnl_batch *batch);
void mnl_batch_reset(struct nftnl_batch *batch);
uint32_t mnl_batch_begin(struct nftnl_batch *batch, uint32_t seqnum);
void mnl_batch_end(struct nftnl_batch *batch, uint32_t seqnum);
-int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list);
+int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
+ uint32_t num_cmds);
int mnl_nft_rule_add(struct netlink_ctx *ctx, const struct cmd *cmd,
unsigned int flags);
diff --git a/src/libnftables.c b/src/libnftables.c
index 199dbc97b801..a58b8ca9dcf6 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -21,7 +21,7 @@ static int nft_netlink(struct nft_ctx *nft,
struct list_head *cmds, struct list_head *msgs,
struct mnl_socket *nf_sock)
{
- uint32_t batch_seqnum, seqnum = 0;
+ uint32_t batch_seqnum, seqnum = 0, num_cmds = 0;
struct nftnl_batch *batch;
struct netlink_ctx ctx;
struct cmd *cmd;
@@ -49,6 +49,7 @@ static int nft_netlink(struct nft_ctx *nft,
strerror(errno));
goto out;
}
+ num_cmds++;
}
if (!nft->check)
mnl_batch_end(batch, mnl_seqnum_alloc(&seqnum));
@@ -56,7 +57,7 @@ static int nft_netlink(struct nft_ctx *nft,
if (!mnl_batch_ready(batch))
goto out;
- ret = mnl_batch_talk(&ctx, &err_list);
+ ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
list_for_each_entry_safe(err, tmp, &err_list, head) {
list_for_each_entry(cmd, cmds, list) {
diff --git a/src/mnl.c b/src/mnl.c
index 6c85b1855c86..96984f03e1be 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -295,12 +295,14 @@ static ssize_t mnl_nft_socket_sendmsg(struct netlink_ctx *ctx,
return sendmsg(mnl_socket_get_fd(ctx->nft->nf_sock), msg, 0);
}
-int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
+int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
+ uint32_t num_cmds)
{
struct mnl_socket *nl = ctx->nft->nf_sock;
int ret, fd = mnl_socket_get_fd(nl), portid = mnl_socket_get_portid(nl);
uint32_t iov_len = nftnl_batch_iovec_len(ctx->batch);
char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
+ size_t avg_msg_size, batch_size;
const struct sockaddr_nl snl = {
.nl_family = AF_NETLINK
};
@@ -308,14 +310,17 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
.tv_sec = 0,
.tv_usec = 0
};
- fd_set readfds;
struct iovec iov[iov_len];
struct msghdr msg = {};
+ fd_set readfds;
int err = 0;
mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
- mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
+ batch_size = mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
+ avg_msg_size = div_round_up(batch_size, num_cmds);
+
+ mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * 2);
ret = mnl_nft_socket_sendmsg(ctx, &msg);
if (ret == -1)
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH nft,v2 5/7] mnl: estimate receiver buffer size
2019-05-30 10:55 ` [PATCH nft,v2 5/7] mnl: estimate receiver buffer size Pablo Neira Ayuso
@ 2019-05-31 18:11 ` Eric Garver
2019-05-31 18:40 ` Pablo Neira Ayuso
0 siblings, 1 reply; 9+ messages in thread
From: Eric Garver @ 2019-05-31 18:11 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, phil
On Thu, May 30, 2019 at 12:55:27PM +0200, Pablo Neira Ayuso wrote:
> Set a receiver buffer size based on the number of commands and the
> average message size, this is useful for the --echo option in order to
> avoid ENOBUFS errors.
>
> Double the estimated size is used to ensure enough receiver buffer
> space.
>
> Skip buffer receiver logic if estimation is smaller than current buffer.
>
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
[..]
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 199dbc97b801..a58b8ca9dcf6 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
[..]
> @@ -308,14 +310,17 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
> .tv_sec = 0,
> .tv_usec = 0
> };
> - fd_set readfds;
> struct iovec iov[iov_len];
> struct msghdr msg = {};
> + fd_set readfds;
> int err = 0;
>
> mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
>
> - mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
> + batch_size = mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
> + avg_msg_size = div_round_up(batch_size, num_cmds);
> +
> + mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * 2);
I think this calculation is incorrect. I'm still getting ENOBUFS with
Phil's testcase and firewalld's testsuite (large json blob). I changed
the multiplier from 2 to 6 and it worked.
-->8--
# ./run-tests.sh ./testcases/transactions/0049huge_0
I: using nft binary ./../../src/nft
W: [FAILED] ./testcases/transactions/0049huge_0: got 1
netlink: Error: Could not process rule: No buffer space available
I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH nft,v2 5/7] mnl: estimate receiver buffer size
2019-05-31 18:11 ` Eric Garver
@ 2019-05-31 18:40 ` Pablo Neira Ayuso
0 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-31 18:40 UTC (permalink / raw)
To: Eric Garver, netfilter-devel, phil
On Fri, May 31, 2019 at 02:11:41PM -0400, Eric Garver wrote:
> On Thu, May 30, 2019 at 12:55:27PM +0200, Pablo Neira Ayuso wrote:
> > Set a receiver buffer size based on the number of commands and the
> > average message size, this is useful for the --echo option in order to
> > avoid ENOBUFS errors.
> >
> > Double the estimated size is used to ensure enough receiver buffer
> > space.
> >
> > Skip buffer receiver logic if estimation is smaller than current buffer.
> >
> > Reported-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> [..]
> > diff --git a/src/libnftables.c b/src/libnftables.c
> > index 199dbc97b801..a58b8ca9dcf6 100644
> > --- a/src/libnftables.c
> > +++ b/src/libnftables.c
> [..]
> > @@ -308,14 +310,17 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list)
> > .tv_sec = 0,
> > .tv_usec = 0
> > };
> > - fd_set readfds;
> > struct iovec iov[iov_len];
> > struct msghdr msg = {};
> > + fd_set readfds;
> > int err = 0;
> >
> > mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
> >
> > - mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
> > + batch_size = mnl_nft_batch_to_msg(ctx, &msg, &snl, iov, iov_len);
> > + avg_msg_size = div_round_up(batch_size, num_cmds);
> > +
> > + mnl_set_rcvbuffer(ctx->nft->nf_sock, num_cmds * avg_msg_size * 2);
>
> I think this calculation is incorrect.
Yes, see v4 of this patch:
https://patchwork.ozlabs.org/patch/1107737/
> I'm still getting ENOBUFS with Phil's testcase and firewalld's
> testsuite (large json blob). I changed the multiplier from 2 to 6
> and it worked.
I just pushed out the patchset, the last version is using a multiplier
of 4, I modified Phil's testcase to 100000 and it works fine. Please
try the version upstream and let me know.
We can enhance this code by checking for ENOBUFS in _sendmsg(), extend
the buffer size and retry. Then, we also need to update kernel code to
abort the transaction in case NLM_F_ECHO flag is set on and we hit
ENOBUFS.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH nft,v2 6/7] mnl: mnl_batch_talk() returns -1 on internal netlink errors
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
` (3 preceding siblings ...)
2019-05-30 10:55 ` [PATCH nft,v2 5/7] mnl: estimate receiver buffer size Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
2019-05-30 10:55 ` [PATCH nft,v2 7/7] erec: remove double \n on error when internal_netlink is used Pablo Neira Ayuso
5 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
Display an error in case internal netlink plumbing hits problems.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/libnftables.c | 8 ++++++++
src/mnl.c | 7 ++-----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/libnftables.c b/src/libnftables.c
index a58b8ca9dcf6..d8de89ca509c 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -58,6 +58,14 @@ static int nft_netlink(struct nft_ctx *nft,
goto out;
ret = mnl_batch_talk(&ctx, &err_list, num_cmds);
+ if (ret < 0) {
+ netlink_io_error(&ctx, NULL,
+ "Could not process rule: %s", strerror(errno));
+ goto out;
+ }
+
+ if (!list_empty(&err_list))
+ ret = -1;
list_for_each_entry_safe(err, tmp, &err_list, head) {
list_for_each_entry(cmd, cmds, list) {
diff --git a/src/mnl.c b/src/mnl.c
index 96984f03e1be..4c15387000e9 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -313,7 +313,6 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
struct iovec iov[iov_len];
struct msghdr msg = {};
fd_set readfds;
- int err = 0;
mnl_set_sndbuffer(ctx->nft->nf_sock, ctx->batch);
@@ -343,10 +342,8 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
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) {
+ if (ret == -1)
mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
- err = -1;
- }
ret = select(fd+1, &readfds, NULL, NULL, &tv);
if (ret == -1)
@@ -355,7 +352,7 @@ int mnl_batch_talk(struct netlink_ctx *ctx, struct list_head *err_list,
FD_ZERO(&readfds);
FD_SET(fd, &readfds);
}
- return err;
+ return 0;
}
int mnl_nft_rule_add(struct netlink_ctx *ctx, const struct cmd *cmd,
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH nft,v2 7/7] erec: remove double \n on error when internal_netlink is used
2019-05-30 10:55 [PATCH nft,v2 1/7] mnl: add mnl_set_rcvbuffer() and use it Pablo Neira Ayuso
` (4 preceding siblings ...)
2019-05-30 10:55 ` [PATCH nft,v2 6/7] mnl: mnl_batch_talk() returns -1 on internal netlink errors Pablo Neira Ayuso
@ 2019-05-30 10:55 ` Pablo Neira Ayuso
5 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2019-05-30 10:55 UTC (permalink / raw)
To: netfilter-devel; +Cc: phil
Remove double empty line linebreak when printing internal errors.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
src/erec.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/erec.c b/src/erec.c
index 617c04ade178..cf543a980bc0 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -161,7 +161,6 @@ void erec_print(struct output_ctx *octx, const struct error_record *erec,
loc = &erec->locations[l];
netlink_dump_expr(loc->nle, f, debug_mask);
}
- fprintf(f, "\n\n");
return;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread