[RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

From: Lance Yang <lance.yang@linux.dev>

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 <zi.li@linux.dev>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
---
 net/netfilter/nf_conntrack_standalone.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 2f666751c7e7..b4acff01088f 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -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",
-- 
2.49.0

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Pablo Neira Ayuso]

Hi,

Cc: Florian Westphal.

On Wed, May 14, 2025 at 01:37:51PM +0800, Lance Yang wrote:
> From: Lance Yang <lance.yang@linux.dev>
> 
> 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.

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.

> Signed-off-by: Zi Li <zi.li@linux.dev>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
>  net/netfilter/nf_conntrack_standalone.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 2f666751c7e7..b4acff01088f 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -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",
> -- 
> 2.49.0
> 

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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.

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.

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

Hi Pablo and Florian,

Thanks for taking the time to review!

On 2025/5/21 19:21, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> 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.
> 
> I don't like this patch or the above because we do have two log
> backends, syslog + nflog.

Ah, good to know! I wasn't aware of that :(

> 
> 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.

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)
+ 	request_module("%s", "nf_log_syslog");

Hmm... is nf_logger_find_get() the correct way to check if no
logger is registered, or are there preferred alternatives?

Thanks,
Lance

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Florian Westphal]

Lance Yang <lance.yang@linux.dev> wrote:
> > 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.
> 
> 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)
> + 	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.

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

On 2025/5/22 02:23, Florian Westphal wrote:
> Lance Yang <lance.yang@linux.dev> wrote:
>>> 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.
>>
>> 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)
>> + 	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.

Ah, understood ;)

> 
> 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.

Thanks,
Lance

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Florian Westphal]

Lance Yang <lance.yang@linux.dev> wrote:
> 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.

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

On 2025/5/22 14:34, Florian Westphal wrote:
> Lance Yang <lance.yang@linux.dev> wrote:
>> 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

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

On 2025/5/22 14:53, Lance Yang wrote:
> 
> 
> On 2025/5/22 14:34, Florian Westphal wrote:
>> Lance Yang <lance.yang@linux.dev> wrote:
>>> 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.

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;
}

Thanks,
Lance

> 
> Ah, got it. I'll do that.
> 
> Thanks,
> Lance
> 

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Florian Westphal]

Lance Yang <lance.yang@linux.dev> wrote:
> Does this helper look correct?

Yes, but ...
> /**
>   * 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;
> }

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;
}

?

Re: [RESEND PATCH 1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid

[Lance Yang]

On 2025/5/22 21:19, Florian Westphal wrote:
> Lance Yang <lance.yang@linux.dev> wrote:
>> Does this helper look correct?
> 
> Yes, but ...
>> /**
>>    * 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;
>> }
> 
> 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;
> }

Yeah, it's simpler and better. Thanks!