Skip to contentSkip to conversation

netfilter: nfnetlink_log: consolidate check for instance in nfulnl_recv_config()

PATCH In review Pablo Neira Ayuso 2 participants 3 messages Oct 13, 2015 – Oct 15, 2015

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 1/2 netfilter: nfnetlink_log: consolidate check for instance in nfulnl_recv_config()

This patch consolidates the check for valid logger instance once we have passed the command handling:

The config message that we receive may contain the following info:

1) Command only: We always get a valid instance pointer if we just created it. In case that the instance is being destroyed or the command is unknown, we jump to exit path of nfulnl_recv_config(). This patch doesn't modify this handling.

2) Config only: In this case, the instance must always exist since the user is asking for configuration updates. If the instance doesn't exist this returns -ENODEV.

3) No command and no configs are specified: This case is rare. The user is sending us a config message with neither commands nor config options. In this case, we have to check if the instance exists and bail out otherwise. Before this patch, it was possible to send a config message with no command and no config updates for an unexisting instance without triggering an error. So this is the only case that changes.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
@Ken-ichirou: Could you give a test to this patches? Thanks.

 net/netfilter/nfnetlink_log.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)
net/netfilter/nfnetlink_log.c +5 -23
index f8d9bd8..2002d57 100644
@@ -874,16 +874,15 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -ENOTSUPP;
 			break;
 		}
+	} else if (!inst) {
+		ret = -ENODEV;
+		goto out;
 	}
 
 	if (nfula[NFULA_CFG_MODE]) {
-		struct nfulnl_msg_config_mode *params;
-		params = nla_data(nfula[NFULA_CFG_MODE]);
+		struct nfulnl_msg_config_mode *params =
+			nla_data(nfula[NFULA_CFG_MODE]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_mode(inst, params->copy_mode,
 				ntohl(params->copy_range));
 	}

@@ -891,41 +890,24 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 	if (nfula[NFULA_CFG_TIMEOUT]) {
 		__be32 timeout = nla_get_be32(nfula[NFULA_CFG_TIMEOUT]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_timeout(inst, ntohl(timeout));
 	}
 
 	if (nfula[NFULA_CFG_NLBUFSIZ]) {
 		__be32 nlbufsiz = nla_get_be32(nfula[NFULA_CFG_NLBUFSIZ]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_nlbufsiz(inst, ntohl(nlbufsiz));
 	}
 
 	if (nfula[NFULA_CFG_QTHRESH]) {
 		__be32 qthresh = nla_get_be32(nfula[NFULA_CFG_QTHRESH]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_qthresh(inst, ntohl(qthresh));
 	}
 
 	if (nfula[NFULA_CFG_FLAGS]) {
 		u16 flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
-
 		if (flags & NFULNL_CFG_F_CONNTRACK &&
 		    !rcu_access_pointer(nfnl_ct_hook)) {
 #ifdef CONFIG_MODULES
Signature · 1 line
2.1.4
author maintainer
#1 lore raw

PATCH 2/2 netfilter: nfnetlink_log: validate dependencies to avoid breaking atomicity

Check that dependencies are fulfilled before updating the logger instance, otherwise we can leave things in intermediate state on errors in nfulnl_recv_config().

Signed-off-by Pablo Neira Ayuso <[email protected]>
---
 net/netfilter/nfnetlink_log.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)
net/netfilter/nfnetlink_log.c +24 -19
index 2002d57..a5b9680 100644
@@ -825,6 +825,7 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 	struct net *net = sock_net(ctnl);
 	struct nfnl_log_net *log = nfnl_log_pernet(net);
 	int ret = 0;
+	u16 flags;
 
 	if (nfula[NFULA_CFG_CMD]) {
 		u_int8_t pf = nfmsg->nfgen_family;

@@ -846,6 +847,28 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		goto out_put;
 	}
 
+	/* Check if we support these flags in first place, dependencies should
+	 * be there too not to break atomicity.
+	 */
+	if (nfula[NFULA_CFG_FLAGS]) {
+		flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
+
+		if ((flags & NFULNL_CFG_F_CONNTRACK) &&
+		    !rcu_access_pointer(nfnl_ct_hook)) {
+#ifdef CONFIG_MODULES
+			nfnl_unlock(NFNL_SUBSYS_ULOG);
+			request_module("ip_conntrack_netlink");
+			nfnl_lock(NFNL_SUBSYS_ULOG);
+			if (rcu_access_pointer(nfnl_ct_hook)) {
+				ret = -EAGAIN;
+				goto out_put;
+			}
+#endif
+			ret = -EOPNOTSUPP;
+			goto out_put;
+		}
+	}
+
 	if (cmd != NULL) {
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_BIND:

@@ -905,26 +928,8 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		nfulnl_set_qthresh(inst, ntohl(qthresh));
 	}
 
-	if (nfula[NFULA_CFG_FLAGS]) {
-		u16 flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
-
-		if (flags & NFULNL_CFG_F_CONNTRACK &&
-		    !rcu_access_pointer(nfnl_ct_hook)) {
-#ifdef CONFIG_MODULES
-			nfnl_unlock(NFNL_SUBSYS_ULOG);
-			request_module("ip_conntrack_netlink");
-			nfnl_lock(NFNL_SUBSYS_ULOG);
-			if (rcu_access_pointer(nfnl_ct_hook)) {
-				ret = -EAGAIN;
-				goto out;
-			}
-#endif
-			ret = -EOPNOTSUPP;
-			goto out;
-		}
-
+	if (nfula[NFULA_CFG_FLAGS])
 		nfulnl_set_flags(inst, flags);
-	}
 
 out_put:
 	instance_put(inst);
Signature · 1 line
2.1.4
#2 lore raw

Re: [PATCH nf-next 2/2] netfilter: nfnetlink_log: validate dependencies to avoid breaking atomicity

Thank you for your usual support.

Quoted 1 line · Pablo Neira Ayuso

@Ken-ichirou: Could you give a test to this patches? Thanks.

I've tested these with appended and is fine, of course. Before applying patches:

    # ./a.out 123
    a.out: empty_cfg.c:56: main: Assertion `mnl_cb_run(buf, ret, 0, portid, ((void *)0), ((void *)0)) == -1' failed.
    Aborted

net/netfilter/nfnetlink_log.c nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 868

Quoted 11 lines · Pablo Neira Ayuso
net/netfilter/nfnetlink_log.c +11 -0
+#ifdef CONFIG_MODULES
+			nfnl_unlock(NFNL_SUBSYS_ULOG);
+			request_module("ip_conntrack_netlink");
+			nfnl_lock(NFNL_SUBSYS_ULOG);
+			if (rcu_access_pointer(nfnl_ct_hook)) {
+				ret = -EAGAIN;
+				goto out_put;
+			}
+#endif
+			ret = -EOPNOTSUPP;
+			goto out_put;

It's off the subject, but this fixes module unload. It was not put instance at my first patch, Thanks!

/*
* This example is placed in the public domain.
* cc empty_cfg.c -lmnl -lnetfilter_log
*/

#include <stdio.h>
#include <stdlib.h>
#include <arpa/inet.h>
#include <errno.h>
#include <assert.h>

#include <linux/netfilter/nfnetlink_log.h>

#include <libmnl/libmnl.h>
#include <libnetfilter_log/libnetfilter_log.h>

int main(int argc, char *argv[])

{
	struct mnl_socket *nl;
	char buf[MNL_SOCKET_BUFFER_SIZE];
	struct nlmsghdr *nlh;
	int ret;
	unsigned int portid, qnum;

	if (argc != 2) {
		printf("Usage: %s [queue_num]\n", argv[0]);
		exit(EXIT_FAILURE);
	}
	qnum = atoi(argv[1]);

	nl = mnl_socket_open(NETLINK_NETFILTER);
	if (nl == NULL) {
		perror("mnl_socket_open");
		exit(EXIT_FAILURE);
	}

	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
		perror("mnl_socket_bind");
		exit(EXIT_FAILURE);
	}
	portid = mnl_socket_get_portid(nl);

	nlh = nflog_nlmsg_put_header(buf, NFULNL_MSG_CONFIG, AF_UNSPEC, qnum);
	nlh->nlmsg_flags |= NLM_F_ACK;
	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
		perror("mnl_socket_sendto");
		exit(EXIT_FAILURE);
	}

	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
	if (ret == -1) {
		perror("mnl_socket_recvfrom");
		exit(EXIT_FAILURE);
	}

	assert(mnl_cb_run(buf, ret, 0, portid, NULL, NULL) == MNL_CB_ERROR);
	assert(errno == ENODEV);

	mnl_socket_close(nl);

	return 0;
}