Re: [PATCH net 05/10] netfilter: x_tables: remove pr_debug

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: x_tables: remove pr_debug

This drops the pr_debug() calls from a set of xtables matches and
targets, and converts the ones in the .check paths into
pr_info_ratelimited() so that legacy iptables users get a hint in dmesg
when a rule is rejected.

A few observations below.

> diff --git a/net/ipv6/netfilter/ip6t_hbh.c b/net/ipv6/netfilter/ip6t_hbh.c
> index 6d1a5d2026a67..37c207d3152a4 100644
> --- a/net/ipv6/netfilter/ip6t_hbh.c
> +++ b/net/ipv6/netfilter/ip6t_hbh.c

[ ... ]

> @@ -166,16 +144,16 @@ static int hbh_mt6_check(const struct xt_mtchk_param *par)
>  	const struct ip6t_opts *optsinfo = par->matchinfo;
>  
>  	if (optsinfo->invflags & ~IP6T_OPTS_INV_MASK) {
> -		pr_debug("unknown flags %X\n", optsinfo->invflags);
> +		pr_info_ratelimited("unknown flags %X\n", optsinfo->invflags);
>  		return -EINVAL;
>  	}
>  	if (optsinfo->optsnr > IP6T_OPTS_OPTSNR) {
> -		pr_debug("too many supported opts specified\n");
> +		pr_info_ratelimited("too many supported opts specified\n");
>  		return -EINVAL;
>  	}
>  
>  	if (optsinfo->flags & IP6T_OPTS_NSTRICT) {
> -		pr_debug("Not strict - not implemented");
> +		pr_info_ratelimited("Not strict - not implemented");
>  		return -EINVAL;
>  	}

[Severity: Low]
Should this string get a trailing newline like its two siblings in
hbh_mt6_check()?

The missing '\n' was harmless while this was pr_debug() (compiled out or
dynamic-debug gated), but as an unconditional pr_info_ratelimited() the
record is left open for continuation:

kernel/printk/printk.c:printk_sprint() {
	/* Mark and strip a trailing newline. */
	if (text_len && text[text_len - 1] == '\n') {
		text_len--;
		*flags |= LOG_NEWLINE;
	}
}

kernel/printk/printk.c:vprintk_store() {
	/* A message without a trailing newline can be continued. */
	if (!(flags & LOG_NEWLINE))
		prb_commit(&e);
	else
		prb_final_commit(&e);
}

So an iptables-legacy rule with IP6T_OPTS_NSTRICT set produces a dmesg
line that gets glued to whatever is printed next, which works against the
"hint via dmesg" goal in the changelog.

The same missing '\n' appears in three more of the converted strings, noted
below.

> diff --git a/net/ipv6/netfilter/ip6t_rt.c b/net/ipv6/netfilter/ip6t_rt.c
> index 278b52752f364..0c01ac2746d1f 100644
> --- a/net/ipv6/netfilter/ip6t_rt.c
> +++ b/net/ipv6/netfilter/ip6t_rt.c
> @@ -155,18 +155,18 @@ static int rt_mt6_check(const struct xt_mtchk_param *par)
>  	const struct ip6t_rt *rtinfo = par->matchinfo;
>  
>  	if (rtinfo->invflags & ~IP6T_RT_INV_MASK) {
> -		pr_debug("unknown flags %X\n", rtinfo->invflags);
> +		pr_info_ratelimited("unknown flags %X\n", rtinfo->invflags);
>  		return -EINVAL;
>  	}
>  	if (rtinfo->addrnr > IP6T_RT_HOPS) {
> -		pr_debug("too many addresses specified\n");
> +		pr_info_ratelimited("too many addresses specified\n");
>  		return -EINVAL;
>  	}
>  	if ((rtinfo->flags & (IP6T_RT_RES | IP6T_RT_FST_MASK)) &&
>  	    (!(rtinfo->flags & IP6T_RT_TYP) ||
>  	     (rtinfo->rt_type != 0) ||
>  	     (rtinfo->invflags & IP6T_RT_INV_TYP))) {
> -		pr_debug("`--rt-type 0' required before `--rt-0-*'");
> +		pr_info_ratelimited("`--rt-type 0' required before `--rt-0-*'");
>  		return -EINVAL;
>  	}

[Severity: Low]
Same question here in rt_mt6_check(): the two messages above it end in
'\n' but this one does not, so a rule with IP6T_RT_RES or IP6T_RT_FST_MASK
and rt_type != 0 leaves the record un-terminated.

> diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
> index bfcf2d44e93db..15b698ea98aa6 100644
> --- a/net/netfilter/xt_IDLETIMER.c
> +++ b/net/netfilter/xt_IDLETIMER.c

[ ... ]

> @@ -171,7 +168,7 @@ static int idletimer_tg_create(struct idletimer_tg_info *info)
>  
>  	ret = sysfs_create_file(idletimer_tg_kobj, &info->timer->attr.attr);
>  	if (ret < 0) {
> -		pr_debug("couldn't add file to sysfs");
> +		pr_info_ratelimited("couldn't add file to sysfs");
>  		goto out_free_attr;
>  	}

[Severity: Low]
Would it be worth adding the '\n' here in idletimer_tg_create() too?  This
one is reached from idletimer_tg_checkentry() when sysfs_create_file()
fails, and the other strings converted in this file
("timeout value is zero\n", "label is empty or not nul-terminated\n") are
newline terminated.

> @@ -220,7 +217,7 @@ static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
>  
>  	ret = sysfs_create_file(idletimer_tg_kobj, &info->timer->attr.attr);
>  	if (ret < 0) {
> -		pr_debug("couldn't add file to sysfs");
> +		pr_info_ratelimited("couldn't add file to sysfs");
>  		goto out_free_attr;
>  	}

[Severity: Low]
And the second copy of the same string, in idletimer_tg_create_v1(),
reached from idletimer_tg_checkentry_v1().

[ ... ]

> @@ -534,7 +490,7 @@ static int __init idletimer_tg_init(void)
>  	idletimer_tg_class = class_create("xt_idletimer");
>  	err = PTR_ERR(idletimer_tg_class);
>  	if (IS_ERR(idletimer_tg_class)) {
> -		pr_debug("couldn't register device class\n");
> +		pr_info_ratelimited("couldn't register device class\n");
>  		goto out;
>  	}
>  
> @@ -542,7 +498,7 @@ static int __init idletimer_tg_init(void)
>  					    MKDEV(0, 0), NULL, "timers");
>  	err = PTR_ERR(idletimer_tg_device);
>  	if (IS_ERR(idletimer_tg_device)) {
> -		pr_debug("couldn't register system device\n");
> +		pr_info_ratelimited("couldn't register system device\n");
>  		goto out_class;
>  	}
>  
> @@ -551,7 +507,7 @@ static int __init idletimer_tg_init(void)
>  	err = xt_register_targets(idletimer_tg, ARRAY_SIZE(idletimer_tg));
>  
>  	if (err < 0) {
> -		pr_debug("couldn't register xt target\n");
> +		pr_info_ratelimited("couldn't register xt target\n");
>  		goto out_dev;
>  	}

[Severity: Low]
The changelog says:

    Still, turn pr_debug() into pr_info_ratelimited() in the .check
    path since this helps provide a hint via dmesg in legacy iptables.

These three conversions are in idletimer_tg_init(), which is __init and not
an xtables checkentry callback, so they fall outside the ".check path"
described above.

Were these intended?  Ratelimiting can never trigger on a one-shot module
init failure, and pr_err() would be the usual level for class_create(),
device_create() and xt_register_targets() failing.

> diff --git a/net/netfilter/xt_NETMAP.c b/net/netfilter/xt_NETMAP.c
> index cb2ee80d84fa8..8c86d9a43373a 100644
> --- a/net/netfilter/xt_NETMAP.c
> +++ b/net/netfilter/xt_NETMAP.c
> @@ -106,11 +106,11 @@ static int netmap_tg4_check(const struct xt_tgchk_param *par)
>  	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
>  
>  	if (!(mr->range[0].flags & NF_NAT_RANGE_MAP_IPS)) {
> -		pr_debug("bad MAP_IPS.\n");
> +		pr_info_ratelimited("bad MAP_IPS.\n");
>  		return -EINVAL;
>  	}
>  	if (mr->rangesize != 1) {
> -		pr_debug("bad rangesize %u.\n", mr->rangesize);
> +		pr_info_ratelimited("bad rangesize %u.\n", mr->rangesize);
>  		return -EINVAL;
>  	}
>  	return nf_ct_netns_get(par->net, par->family);

[Severity: Low]
Do these new messages need a pr_fmt() in this file to be identifiable?

xt_NETMAP.c and xt_REDIRECT.c do not define pr_fmt(), so the fallback in
include/linux/printk.h applies:

	#define pr_fmt(fmt) fmt

while the peer site does define one:

net/netfilter/xt_MASQUERADE.c:
	#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

and prints the byte-identical strings "bad MAP_IPS." and
"bad rangesize %u".  The result is that MASQUERADE reports
"xt_MASQUERADE: bad MAP_IPS." while NETMAP and REDIRECT report a bare
"bad MAP_IPS.", so dmesg no longer says which target rejected the rule.
With pr_debug() the dynamic debug machinery supplied module, file and
function, so this did not matter before.

> diff --git a/net/netfilter/xt_REDIRECT.c b/net/netfilter/xt_REDIRECT.c
> index ff66b56a3f97d..27bc0545c4f74 100644
> --- a/net/netfilter/xt_REDIRECT.c
> +++ b/net/netfilter/xt_REDIRECT.c
> @@ -51,11 +51,11 @@ static int redirect_tg4_check(const struct xt_tgchk_param *par)
>  	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
>  
>  	if (mr->range[0].flags & NF_NAT_RANGE_MAP_IPS) {
> -		pr_debug("bad MAP_IPS.\n");
> +		pr_info_ratelimited("bad MAP_IPS.\n");
>  		return -EINVAL;
>  	}
>  	if (mr->rangesize != 1) {
> -		pr_debug("bad rangesize %u.\n", mr->rangesize);
> +		pr_info_ratelimited("bad rangesize %u.\n", mr->rangesize);
>  		return -EINVAL;
>  	}
>  	return nf_ct_netns_get(par->net, par->family);

[Severity: Low]
Same for redirect_tg4_check() in xt_REDIRECT.c, which also has no pr_fmt()
definition.