* [PATCH] netfilter: initialize 'ret' variable
@ 2022-12-02 7:03 Li Qiong
2022-12-02 7:13 ` Al Viro
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Li Qiong @ 2022-12-02 7:03 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, netfilter-devel, kernel-janitors, coreteam,
Yu Zhe, Li Qiong
The 'ret' should need to be initialized to 0, in case
return a uninitialized value.
Signed-off-by: Li Qiong <liqiong@nfschina.com>
---
net/netfilter/nf_flow_table_ip.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index b350fe9d00b0..225ff865d609 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -351,7 +351,7 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
struct rtable *rt;
struct iphdr *iph;
__be32 nexthop;
- int ret;
+ int ret = 0;
if (skb->protocol != htons(ETH_P_IP) &&
!nf_flow_skb_encap_protocol(skb, htons(ETH_P_IP), &offset))
@@ -613,7 +613,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
u32 hdrsize, offset = 0;
struct ipv6hdr *ip6h;
struct rt6_info *rt;
- int ret;
+ int ret = 0;
if (skb->protocol != htons(ETH_P_IPV6) &&
!nf_flow_skb_encap_protocol(skb, htons(ETH_P_IPV6), &offset))
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: initialize 'ret' variable
2022-12-02 7:03 [PATCH] netfilter: initialize 'ret' variable Li Qiong
@ 2022-12-02 7:13 ` Al Viro
2022-12-02 8:06 ` liqiong
2022-12-02 9:18 ` Jan Engelhardt
2022-12-05 14:26 ` Pablo Neira Ayuso
2022-12-06 7:44 ` [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)' Li Qiong
2 siblings, 2 replies; 8+ messages in thread
From: Al Viro @ 2022-12-02 7:13 UTC (permalink / raw)
To: Li Qiong
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, netfilter-devel, kernel-janitors, coreteam,
Yu Zhe
On Fri, Dec 02, 2022 at 03:03:31PM +0800, Li Qiong wrote:
> The 'ret' should need to be initialized to 0, in case
> return a uninitialized value.
Why is 0 the right value? And which case would it be?
We clearly need to know that to figure out which return
value would be correct for it...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: initialize 'ret' variable
2022-12-02 7:13 ` Al Viro
@ 2022-12-02 8:06 ` liqiong
2022-12-02 9:18 ` Jan Engelhardt
1 sibling, 0 replies; 8+ messages in thread
From: liqiong @ 2022-12-02 8:06 UTC (permalink / raw)
To: Al Viro
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, netfilter-devel, kernel-janitors, coreteam,
Yu Zhe
> On Fri, Dec 02, 2022 at 03:03:31PM +0800, Li Qiong wrote:
>> The 'ret' should need to be initialized to 0, in case
>> return a uninitialized value.
> Why is 0 the right value? And which case would it be?
> We clearly need to know that to figure out which return
> value would be correct for it...
Hi,
here is a case:
for (i = 0; i < e->num_hook_entries; i++) {
ret = e->hooks[i].hook(e->hooks[i].priv, skb, state);
if (ret != NF_ACCEPT)
return ret;
....
}
I am not sure if 0 (NF_DROP) is the best value, but It's better to initialize a value.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: initialize 'ret' variable
2022-12-02 7:13 ` Al Viro
2022-12-02 8:06 ` liqiong
@ 2022-12-02 9:18 ` Jan Engelhardt
1 sibling, 0 replies; 8+ messages in thread
From: Jan Engelhardt @ 2022-12-02 9:18 UTC (permalink / raw)
To: Al Viro
Cc: Li Qiong, Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, netfilter-devel, kernel-janitors, coreteam,
Yu Zhe
On Friday 2022-12-02 08:13, Al Viro wrote:
>On Fri, Dec 02, 2022 at 03:03:31PM +0800, Li Qiong wrote:
>> The 'ret' should need to be initialized to 0, in case
>> return a uninitialized value.
>
>Why is 0 the right value? And which case would it be?
>We clearly need to know that to figure out which return
>value would be correct for it...
Judging from the error-handling branches,
it should be ret = NF_ACCEPT.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: initialize 'ret' variable
2022-12-02 7:03 [PATCH] netfilter: initialize 'ret' variable Li Qiong
2022-12-02 7:13 ` Al Viro
@ 2022-12-05 14:26 ` Pablo Neira Ayuso
2022-12-06 0:29 ` liqiong
2022-12-06 7:44 ` [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)' Li Qiong
2 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2022-12-05 14:26 UTC (permalink / raw)
To: Li Qiong
Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
netfilter-devel, kernel-janitors, coreteam, Yu Zhe
On Fri, Dec 02, 2022 at 03:03:31PM +0800, Li Qiong wrote:
> The 'ret' should need to be initialized to 0, in case
> return a uninitialized value.
>
> Signed-off-by: Li Qiong <liqiong@nfschina.com>
> ---
> net/netfilter/nf_flow_table_ip.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index b350fe9d00b0..225ff865d609 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -351,7 +351,7 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
> struct rtable *rt;
> struct iphdr *iph;
> __be32 nexthop;
> - int ret;
> + int ret = 0;
>
> if (skb->protocol != htons(ETH_P_IP) &&
> !nf_flow_skb_encap_protocol(skb, htons(ETH_P_IP), &offset))
> @@ -613,7 +613,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
> u32 hdrsize, offset = 0;
> struct ipv6hdr *ip6h;
> struct rt6_info *rt;
> - int ret;
> + int ret = 0;
>
> if (skb->protocol != htons(ETH_P_IPV6) &&
> !nf_flow_skb_encap_protocol(skb, htons(ETH_P_IPV6), &offset))
This can only happen with tuplehash->tuple.xmit_type:
- FLOW_OFFLOAD_XMIT_UNSPEC
- FLOW_OFFLOAD_XMIT_TC
but this should not ever happen in that path.
Instead, I'd suggest to add a 'default' case to the switch, set ret to
NF_DROP and WARN_ON_ONCE(1).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: initialize 'ret' variable
2022-12-05 14:26 ` Pablo Neira Ayuso
@ 2022-12-06 0:29 ` liqiong
0 siblings, 0 replies; 8+ messages in thread
From: liqiong @ 2022-12-06 0:29 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
netfilter-devel, kernel-janitors, coreteam, Yu Zhe
在 2022年12月05日 22:26, Pablo Neira Ayuso 写道:
> On Fri, Dec 02, 2022 at 03:03:31PM +0800, Li Qiong wrote:
>> The 'ret' should need to be initialized to 0, in case
>> return a uninitialized value.
>>
>> Signed-off-by: Li Qiong <liqiong@nfschina.com>
>> ---
>> net/netfilter/nf_flow_table_ip.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
>> index b350fe9d00b0..225ff865d609 100644
>> --- a/net/netfilter/nf_flow_table_ip.c
>> +++ b/net/netfilter/nf_flow_table_ip.c
>> @@ -351,7 +351,7 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
>> struct rtable *rt;
>> struct iphdr *iph;
>> __be32 nexthop;
>> - int ret;
>> + int ret = 0;
>>
>> if (skb->protocol != htons(ETH_P_IP) &&
>> !nf_flow_skb_encap_protocol(skb, htons(ETH_P_IP), &offset))
>> @@ -613,7 +613,7 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>> u32 hdrsize, offset = 0;
>> struct ipv6hdr *ip6h;
>> struct rt6_info *rt;
>> - int ret;
>> + int ret = 0;
>>
>> if (skb->protocol != htons(ETH_P_IPV6) &&
>> !nf_flow_skb_encap_protocol(skb, htons(ETH_P_IPV6), &offset))
> This can only happen with tuplehash->tuple.xmit_type:
>
> - FLOW_OFFLOAD_XMIT_UNSPEC
> - FLOW_OFFLOAD_XMIT_TC
>
> but this should not ever happen in that path.
>
> Instead, I'd suggest to add a 'default' case to the switch, set ret to
> NF_DROP and WARN_ON_ONCE(1).
Thanks, I will send a v2 patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)'
2022-12-02 7:03 [PATCH] netfilter: initialize 'ret' variable Li Qiong
2022-12-02 7:13 ` Al Viro
2022-12-05 14:26 ` Pablo Neira Ayuso
@ 2022-12-06 7:44 ` Li Qiong
2022-12-08 21:12 ` Pablo Neira Ayuso
2 siblings, 1 reply; 8+ messages in thread
From: Li Qiong @ 2022-12-06 7:44 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: netdev, linux-kernel, netfilter-devel, kernel-janitors, coreteam,
Yu Zhe, Li Qiong
Add a 'default' case in case return a uninitialized value of ret.
Signed-off-by: Li Qiong <liqiong@nfschina.com>
---
v2: Add 'default' case instead of initializing 'ret'.
---
net/netfilter/nf_flow_table_ip.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index b350fe9d00b0..19efba1e51ef 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -421,6 +421,10 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
if (ret == NF_DROP)
flow_offload_teardown(flow);
break;
+ default:
+ WARN_ON_ONCE(1);
+ ret = NF_DROP;
+ break;
}
return ret;
@@ -682,6 +686,10 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
if (ret == NF_DROP)
flow_offload_teardown(flow);
break;
+ default:
+ WARN_ON_ONCE(1);
+ ret = NF_DROP;
+ break;
}
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)'
2022-12-06 7:44 ` [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)' Li Qiong
@ 2022-12-08 21:12 ` Pablo Neira Ayuso
0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2022-12-08 21:12 UTC (permalink / raw)
To: Li Qiong
Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel,
netfilter-devel, kernel-janitors, coreteam, Yu Zhe
On Tue, Dec 06, 2022 at 03:44:14PM +0800, Li Qiong wrote:
> Add a 'default' case in case return a uninitialized value of ret.
Applied, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-08 21:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-02 7:03 [PATCH] netfilter: initialize 'ret' variable Li Qiong
2022-12-02 7:13 ` Al Viro
2022-12-02 8:06 ` liqiong
2022-12-02 9:18 ` Jan Engelhardt
2022-12-05 14:26 ` Pablo Neira Ayuso
2022-12-06 0:29 ` liqiong
2022-12-06 7:44 ` [PATCH v2] netfilter: add a 'default' case to 'switch (tuplehash->tuple.xmit_type)' Li Qiong
2022-12-08 21:12 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).