net/netfilter/nf_flow_table_ip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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.0On 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).
在 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.
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 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.
> 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.
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© 2016 - 2025 Red Hat, Inc.