netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

From: Lance Yang <[email protected]>

When nf_log_syslog is not loaded, nf_conntrack_log_invalid fails to log invalid packets, leaving users unaware of actual invalid traffic. Improve this by loading nf_log_syslog, similar to how 'iptables -I FORWARD 1 -m conntrack --ctstate INVALID -j LOG' triggers it.

Signed-off-by Zi Li <[email protected]>

Signed-off-by Lance Yang <[email protected]>

---
 net/netfilter/nf_conntrack_standalone.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

net/netfilter/nf_conntrack_standalone.c +19 -1

index 2f666751c7e7..b4acff01088f 100644
@@ -543,6 +543,24 @@ nf_conntrack_hash_sysctl(const struct ctl_table *table, int write,
 	return ret;
 }
 
+static int
+nf_conntrack_log_invalid_sysctl(const struct ctl_table *table, int write,
+				void *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret;
+
+	ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
+	if (ret < 0 || !write)
+		return ret;
+
+	if (*(u8 *)table->data == 0)
+		return ret;
+
+	request_module("%s", "nf_log_syslog");
+
+	return ret;
+}
+
 static struct ctl_table_header *nf_ct_netfilter_header;
 
 enum nf_ct_sysctl_index {

@@ -649,7 +667,7 @@ static struct ctl_table nf_ct_sysctl_table[] = {
 		.data		= &init_net.ct.sysctl_log_invalid,
 		.maxlen		= sizeof(u8),
 		.mode		= 0644,
-		.proc_handler	= proc_dou8vec_minmax,
+		.proc_handler	= nf_conntrack_log_invalid_sysctl,
 	},
 	[NF_SYSCTL_CT_EXPECT_MAX] = {
 		.procname	= "nf_conntrack_expect_max",

Signature · 1 line

2.49.0

I have been beaten by this usability issue in the past, it happens since conntrack is loaded on demand.

Maybe add an inconditionally soft dependency? This is a oneliner patch.

        MODULE_SOFTDEP("pre: nf_log_syslog");

Florian, do you prefer this patch (on-demand) or a oneliner to load this module when conntrack gets loaded too?

It is a bit more memory to make it inconditional, but better to expose to users this soft dependency via lsmod.

Thanks.

Quoted 10 lines · Pablo Neira Ayuso

I have been beaten by this usability issue in the past, it happens since conntrack is loaded on demand.

Maybe add an inconditionally soft dependency? This is a oneliner patch.

         MODULE_SOFTDEP("pre: nf_log_syslog");

Florian, do you prefer this patch (on-demand) or a oneliner to load this module when conntrack gets loaded too?

Quoted 4 lines

It is a bit more memory to make it inconditional, but better to expose to users this soft dependency via lsmod.

Thanks.

I don't like this patch or the above because we do have two log backends, syslog + nflog.

There is no need for 'syslog' to be active for 'log_invalid' to be useful as long as the system in question has e.g. ulogd running and listening to nflog messages.

If anything, the modprobe should be done only when no logger is registered.

Quoted 7 lines · Lance Yang

Quoted 6 lines

There is no need for 'syslog' to be active for 'log_invalid' to be useful as long as the system in question has e.g. ulogd running and listening to nflog messages.

If anything, the modprobe should be done only when no logger is registered.

net/netfilter/nf_conntrack_standalone.c nf_conntrack_hash_sysctl(const struct ctl_table *table, int write, · line 559

Quoted 5 lines

Yes, could we load the module only when no logger exists? Something like:

+ if (nf_logger_find_get(NFPROTO_IPV4, NF_LOG_TYPE_LOG) != 0)

net/netfilter/nf_conntrack_standalone.c +1 -0

+ 	request_module("%s", "nf_log_syslog");

This function bumps the module refcount, so if the logger exists you would need to call nf_logger_put() too.

I'd add a new, simpler helper, that only returns if any logger is active.

bool nf_log_is_registered(int pf);

or something like that.

Nice, thanks for jumping in! I'll hold until the helper lands, then rebase and send the v2.

Quoted 2 lines · Lance Yang

Nice, thanks for jumping in! I'll hold until the helper lands, then rebase and send the v2.

Please just add this new helpre yourself in v2.

Quoted 5 lines · Florian Westphal

Quoted 2 lines · Lance Yang

Nice, thanks for jumping in! I'll hold until the helper lands, then rebase and send the v2.

Please just add this new helpre yourself in v2.

Ah, got it. I'll do that.

Thanks,
Lance

Does this helper look correct?

/**
* nf_log_is_registered - Check if NF_LOG is registered for a protocol
* family
*
* @pf: protocol family (e.g., NFPROTO_IPV4)
*
* Returns true if NF_LOG is registered, false otherwise. */
bool nf_log_is_registered(int pf)

{
         struct nf_logger *logger;

         logger = nf_logger_find_get(pf, NF_LOG_TYPE_LOG);
         if (logger) {
                 nf_logger_put(pf, NF_LOG_TYPE_LOG);
                 return true;
         }

         logger = nf_logger_find_get(pf, NF_LOG_TYPE_ULOG);
         if (logger) {
                 nf_logger_put(pf, NF_LOG_TYPE_ULOG);
                 return true;
         }

         return false;
}

Quoted 1 line · Lance Yang

Does this helper look correct?

Yes, but ...

Quoted 18 lines

/**
* nf_log_is_registered - Check if NF_LOG is registered for a protocol
* family
*
* @pf: protocol family (e.g., NFPROTO_IPV4)
*
* Returns true if NF_LOG is registered, false otherwise. */
bool nf_log_is_registered(int pf)

{
          struct nf_logger *logger;

          logger = nf_logger_find_get(pf, NF_LOG_TYPE_LOG);
          if (logger) {
                  nf_logger_put(pf, NF_LOG_TYPE_LOG);
                  return true;
          }

Quoted 8 lines

          logger = nf_logger_find_get(pf, NF_LOG_TYPE_ULOG);
          if (logger) {
                  nf_logger_put(pf, NF_LOG_TYPE_ULOG);
                  return true;
          }

          return false;
}

Why not simply do:

bool nf_log_is_registered(int pf)

{
	int i;

	for (i = 0; i < NF_LOG_TYPE_MAX; i++) {
		if (rcu_access_pointer(loggers[pf][i]))
			return true;
	}

	return false;
}