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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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
>