[PATCH nf-next] netfilter: xt_cpu: no need to check the validity of invert flag

[PATCH nf-next] netfilter: xt_cpu: no need to check the validity of invert flag

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Instead, we can convert invert flag and ensure it is 1 or 0.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/xt_cpu.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/net/netfilter/xt_cpu.c b/net/netfilter/xt_cpu.c
index c7a2e54..ca1eaaf 100644
--- a/net/netfilter/xt_cpu.c
+++ b/net/netfilter/xt_cpu.c
@@ -25,27 +25,17 @@ MODULE_DESCRIPTION("Xtables: CPU match");
 MODULE_ALIAS("ipt_cpu");
 MODULE_ALIAS("ip6t_cpu");
 
-static int cpu_mt_check(const struct xt_mtchk_param *par)
-{
-	const struct xt_cpu_info *info = par->matchinfo;
-
-	if (info->invert & ~1)
-		return -EINVAL;
-	return 0;
-}
-
 static bool cpu_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct xt_cpu_info *info = par->matchinfo;
 
-	return (info->cpu == smp_processor_id()) ^ info->invert;
+	return (info->cpu == smp_processor_id()) ^ !!info->invert;
 }
 
 static struct xt_match cpu_mt_reg __read_mostly = {
 	.name       = "cpu",
 	.revision   = 0,
 	.family     = NFPROTO_UNSPEC,
-	.checkentry = cpu_mt_check,
 	.match      = cpu_mt,
 	.matchsize  = sizeof(struct xt_cpu_info),
 	.me         = THIS_MODULE,
-- 
2.5.5

Re: [PATCH nf-next] netfilter: xt_cpu: no need to check the validity of invert flag

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 08:23:27PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Instead, we can convert invert flag and ensure it is 1 or 0.
> 
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
> ---
>  net/netfilter/xt_cpu.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/net/netfilter/xt_cpu.c b/net/netfilter/xt_cpu.c
> index c7a2e54..ca1eaaf 100644
> --- a/net/netfilter/xt_cpu.c
> +++ b/net/netfilter/xt_cpu.c
> @@ -25,27 +25,17 @@ MODULE_DESCRIPTION("Xtables: CPU match");
>  MODULE_ALIAS("ipt_cpu");
>  MODULE_ALIAS("ip6t_cpu");
>  
> -static int cpu_mt_check(const struct xt_mtchk_param *par)
> -{
> -	const struct xt_cpu_info *info = par->matchinfo;
> -
> -	if (info->invert & ~1)
> -		return -EINVAL;
> -	return 0;
> -}

This trick is there so we can convert info->invert to info->flags in
the future without a new revision (given the binary interface did not
change). I'm not convinced there is much of benefit from getting rid
of this little extra _check() code that runs from the control plane
path.

>  static bool cpu_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  {
>  	const struct xt_cpu_info *info = par->matchinfo;
>  
> -	return (info->cpu == smp_processor_id()) ^ info->invert;
> +	return (info->cpu == smp_processor_id()) ^ !!info->invert;
>  }
>  
>  static struct xt_match cpu_mt_reg __read_mostly = {
>  	.name       = "cpu",
>  	.revision   = 0,
>  	.family     = NFPROTO_UNSPEC,
> -	.checkentry = cpu_mt_check,
>  	.match      = cpu_mt,
>  	.matchsize  = sizeof(struct xt_cpu_info),
>  	.me         = THIS_MODULE,
> -- 
> 2.5.5
> 
> 

Re: [PATCH nf-next] netfilter: xt_cpu: no need to check the validity of invert flag

[Liping Zhang]

Hi Pablo,

2016-06-23 19:11 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
>> -static int cpu_mt_check(const struct xt_mtchk_param *par)
>> -{
>> -     const struct xt_cpu_info *info = par->matchinfo;
>> -
>> -     if (info->invert & ~1)
>> -             return -EINVAL;
>> -     return 0;
>> -}
>
> This trick is there so we can convert info->invert to info->flags in
> the future without a new revision (given the binary interface did not
> change). I'm not convinced there is much of benefit from getting rid
> of this little extra _check() code that runs from the control plane
> path.
>

Thanks for pointing this out. At my first glace, I think this _check
is tricky and a little ugly,
so I try to remove it and send this patch.

As you said, if we add new flags in the future, for example, we
support a new flag like this
"iptables -A INPUT -m cpu --cpu 0 --flagXXX". When the user use the
new iptables utility
but the kernel is old, currently kernel will reject this request,
because we don't recognize the
"flagXXX".

But apply my patch, kernel will just ignore this unknown flag, this
will confuse the user.
And change a new revision seems unworthy.

So I'd rather not apply this pacth.

Thanks.