diff mbox series

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

Message ID 20250514053751.2271-1-lance.yang@linux.dev
State Changes Requested
Headers show
Series [RESEND,1/1] netfilter: load nf_log_syslog on enabling nf_conntrack_log_invalid | expand

Commit Message

Lance Yang May 14, 2025, 5:37 a.m. UTC 
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(-)

Comments

Pablo Neira Ayuso May 21, 2025, 10:07 a.m. UTC | #1 
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
>
Florian Westphal May 21, 2025, 11:21 a.m. UTC | #2 
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.
Lance Yang May 21, 2025, 3:21 p.m. UTC | #3 
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
Florian Westphal May 21, 2025, 6:23 p.m. UTC | #4 
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.
Lance Yang May 22, 2025, 2:05 a.m. UTC | #5 
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
Florian Westphal May 22, 2025, 6:34 a.m. UTC | #6 
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.
Lance Yang May 22, 2025, 6:53 a.m. UTC | #7 
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
Lance Yang May 22, 2025, 1:14 p.m. UTC | #8 
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
>
Florian Westphal May 22, 2025, 1:19 p.m. UTC | #9 
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;
}

?
Lance Yang May 22, 2025, 1:58 p.m. UTC | #10 
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!
diff mbox series

Patch

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",