[PATCH nft v2 0/2] Improve handling of errors from mnl* functions"

[PATCH nft v2 0/2] Improve handling of errors from mnl* functions"

[Eugene Crosser]

   Version 2 of the patchset

Libnftables does not handle errors that libmnl functions may return
properly:

1. Retriable errors indicated by errno=EINTR are not retried, but
   rather treted as fatal.
2. Instead of reporting the error to the caller, functions call
   exit() on error, which terminates the caller process.

This patch set partly addresses the second point, by calling
abort() instead of exit() on ABI error, that will at least give
more information to the sysadmin than quiet termination of a
process.

It attempts to properly address the first point, by introducing
retry logic when mnl_socket_recvfrom() or mnl_cb_run() return
-1 with errno=EINTR.

It would be desirable to fully address the second point at some
future time, though it requires some redesign of the code structure.

  [PATCH nft v2 1/2] Use abort() in case of netlink_abi_error
  [PATCH nft v2 2/2] Handle retriable errors from mnl functions

[PATCH nft v2 1/2] Use abort() in case of netlink_abi_error

[Eugene Crosser]

Library functions should not use exit(), application that uses the
library may contain error handling path, that cannot be executed if
library functions calls exit(). For truly fatal errors, using abort() is
more acceptable than exit().

Signed-off-by: Eugene Crosser <crosser@average.org>
---
 src/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/netlink.c b/src/netlink.c
index 359d801c..c25a7e79 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -59,7 +59,7 @@ void __noreturn __netlink_abi_error(const char *file, int line,
 {
 	fprintf(stderr, "E: Contact urgently your Linux kernel vendor. "
 		"Netlink ABI is broken: %s:%d %s\n", file, line, reason);
-	exit(NFT_EXIT_FAILURE);
+	abort();
 }
 
 int netlink_io_error(struct netlink_ctx *ctx, const struct location *loc,
-- 
2.32.0

[PATCH nft v2 2/2] Handle retriable errors from mnl functions

[Eugene Crosser]

rc == -1 and errno == EINTR mean:

mnl_socket_recvfrom() - blindly rerun the function
mnl_cb_run()          - restart dump request from scratch

This commit introduces handling of both these conditions

Signed-off-by: Eugene Crosser <crosser@average.org>
---
 src/iface.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/src/iface.c b/src/iface.c
index d0e1834c..5ecc087d 100644
--- a/src/iface.c
+++ b/src/iface.c
@@ -59,14 +59,14 @@ static int data_cb(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
-void iface_cache_update(void)
+static int __iface_cache_update(void)
 {
 	char buf[MNL_SOCKET_BUFFER_SIZE];
 	struct mnl_socket *nl;
 	struct nlmsghdr *nlh;
 	struct rtgenmsg *rt;
 	uint32_t seq, portid;
-	int ret;
+	int ret = -1;
 
 	nlh = mnl_nlmsg_put_header(buf);
 	nlh->nlmsg_type	= RTM_GETLINK;
@@ -77,28 +77,39 @@ void iface_cache_update(void)
 
 	nl = mnl_socket_open(NETLINK_ROUTE);
 	if (nl == NULL)
-		netlink_init_error();
+		return -1;
 
 	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0)
-		netlink_init_error();
+		goto close_and_return;
 
 	portid = mnl_socket_get_portid(nl);
 
 	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0)
-		netlink_init_error();
+		goto close_and_return;
 
-	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
-	while (ret > 0) {
-		ret = mnl_cb_run(buf, ret, seq, portid, data_cb, NULL);
-		if (ret <= MNL_CB_STOP)
+	do {
+		do ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
+		while (ret == -1 && errno == EINTR);
+		if (ret == -1)
 			break;
-		ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
-	}
-	if (ret == -1)
-		netlink_init_error();
+		ret = mnl_cb_run(buf, ret, seq, portid, data_cb, NULL);
+	} while (ret > MNL_CB_STOP);
 
+close_and_return:
 	mnl_socket_close(nl);
 
+	return ret;
+}
+
+void iface_cache_update(void)
+{
+	int ret;
+
+	do ret = __iface_cache_update();
+	while (ret == -1 && errno == EINTR);
+	if (ret == -1)
+		netlink_init_error();
+
 	iface_cache_init = true;
 }
 
-- 
2.32.0

Re: [PATCH nft v2 1/2] Use abort() in case of netlink_abi_error

[Pablo Neira Ayuso]

On Thu, Dec 09, 2021 at 07:26:06PM +0100, Eugene Crosser wrote:
> Library functions should not use exit(), application that uses the
> library may contain error handling path, that cannot be executed if
> library functions calls exit(). For truly fatal errors, using abort() is
> more acceptable than exit().

Applied, thanks.

Re: [PATCH nft v2 2/2] Handle retriable errors from mnl functions

[Pablo Neira Ayuso]

On Thu, Dec 09, 2021 at 07:26:07PM +0100, Eugene Crosser wrote:
> rc == -1 and errno == EINTR mean:
> 
> mnl_socket_recvfrom() - blindly rerun the function
> mnl_cb_run()          - restart dump request from scratch
> 
> This commit introduces handling of both these conditions

Sorry it took me a while to come back to this.

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220127181835.571673-1-pablo@netfilter.org/

This follows the same approach as src/mnl.c, no need to close the
reopen the socket to drop the existing messages.

Re: [PATCH nft v2 2/2] Handle retriable errors from mnl functions

[Eugene Crosser]

[-- Attachment #1.1: Type: text/plain, Size: 1098 bytes --]

On 27/01/2022 19:20, Pablo Neira Ayuso wrote:
> On Thu, Dec 09, 2021 at 07:26:07PM +0100, Eugene Crosser wrote:
>> rc == -1 and errno == EINTR mean:
>>
>> mnl_socket_recvfrom() - blindly rerun the function
>> mnl_cb_run()          - restart dump request from scratch
>>
>> This commit introduces handling of both these conditions
> 
> Sorry it took me a while to come back to this.
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220127181835.571673-1-pablo@netfilter.org/
> 
> This follows the same approach as src/mnl.c, no need to close the
> reopen the socket to drop the existing messages.

Thanks for getting back to it and producing the fix!

I think it is slightly less clean, because if some (not EINTR) error happens
while it is draining the queue (the second call to `mnl_socket_recvfrom()` with
`eintr == true`), `errno` will be overwritten and the error misrepresented. This
should not be a _practical_ problem because presumably the same error will be
raised upon retry, and this time it will be reported correctly.

Thanks again,

Eugene

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH nft v2 2/2] Handle retriable errors from mnl functions

[Pablo Neira Ayuso]

On Thu, Jan 27, 2022 at 07:39:56PM +0100, Eugene Crosser wrote:
> On 27/01/2022 19:20, Pablo Neira Ayuso wrote:
> > On Thu, Dec 09, 2021 at 07:26:07PM +0100, Eugene Crosser wrote:
> >> rc == -1 and errno == EINTR mean:
> >>
> >> mnl_socket_recvfrom() - blindly rerun the function
> >> mnl_cb_run()          - restart dump request from scratch
> >>
> >> This commit introduces handling of both these conditions
> > 
> > Sorry it took me a while to come back to this.
> > 
> > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20220127181835.571673-1-pablo@netfilter.org/
> > 
> > This follows the same approach as src/mnl.c, no need to close the
> > reopen the socket to drop the existing messages.
> 
> Thanks for getting back to it and producing the fix!
> 
> I think it is slightly less clean, because if some (not EINTR) error happens
> while it is draining the queue (the second call to `mnl_socket_recvfrom()` with
> `eintr == true`), `errno` will be overwritten and the error misrepresented. This
> should not be a _practical_ problem because presumably the same error will be
> raised upon retry, and this time it will be reported correctly.

Good catch, sending v2.