Re: [PATCH nf 7/7] netfilter: x_tables: use pr ratelimiting in all remaining spots

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > --- a/net/bridge/netfilter/ebt_among.c
> > +++ b/net/bridge/netfilter/ebt_among.c
> > @@ -187,17 +187,17 @@ static int ebt_among_mt_check(const struct xt_mtchk_param *par)
> >  	expected_length += ebt_mac_wormhash_size(wh_src);
> >  
> >  	if (em->match_size != EBT_ALIGN(expected_length)) {
> > -		pr_info("wrong size: %d against expected %d, rounded to %zd\n",
> > -			em->match_size, expected_length,
> > -			EBT_ALIGN(expected_length));
> > +		pr_info_ratelimited("wrong size: %d against expected %d, rounded to %zd\n",
> 
> Shouldn't all these be pr_err_ratelimited instead?

Don't know.

This could even be pr_debug actually since this message is
useless unless you're doing ebtables development work.

> Probably this is a good chance to homogeneize all error reporting in
> xtables.

Yes.

> >  	if (wh_dst && (err = ebt_mac_wormhash_check_integrity(wh_dst))) {
> > -		pr_info("dst integrity fail: %x\n", -err);
> > +		pr_info_ratelimited("dst integrity fail: %x\n", -err);
> >  		return -EINVAL;
> >  	}
> >  	if (wh_src && (err = ebt_mac_wormhash_check_integrity(wh_src))) {
> > -		pr_info("src integrity fail: %x\n", -err);
> > +		pr_info_ratelimited("src integrity fail: %x\n", -err);
> >  		return -EINVAL;

Same for these two, I'll convert all to pr_debug instead.

> >  	if (info->queues_total == 0) {
> > -		pr_err("NFQUEUE: number of total queues is 0\n");
>                         ^^^^^^^^
> 
> We can probably add this all over the place in the same go?
 
>         #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Yes.

> >  	if (index == IPSET_INVALID_ID) {
> > -		pr_warn("Cannot find set identified by id %u to match\n",
> > -			info->match_set.index);
> > +		pr_warn_ratelimited("Cannot find set identified by id %u to match\n",
> > +				    info->match_set.index);
> 
> Use pr_err_ratelimited instead?

I think we should settle on a single pr_foo, i suggest
pr_info(_ratelimited).

This is not an error condition, we only have these
printks because we can't return a proper error to userspace.

If this was netlink, it would be converted to extack instead...