The 'ret' should need to be initialized to 0, in case
return a uninitialized value.
Signed-off-by: Li Qiong <[email protected]>
---
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
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...
> 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.
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.
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 <[email protected]>
> ---
> 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).
在 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 <[email protected]>
>> ---
>> 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.
Add a 'default' case in case return a uninitialized value of ret.
Signed-off-by: Li Qiong <[email protected]>
---
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
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.