| Message ID | 20210603173410.310362-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 118de6106735cfeb04daf9de1d5a9f953ac034ba |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 30 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Thu, 3 Jun 2021 10:34:10 -0700 you wrote: > Clang warns that proto in rmnet_map_v5_checksum_uplink_packet() might be > used uninitialized: > > drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:14: warning: > variable 'proto' is used uninitialized whenever 'if' condition is false > [-Wsometimes-uninitialized] > } else if (skb->protocol == htons(ETH_P_IPV6)) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:295:36: note: > uninitialized use occurs here > check = rmnet_map_get_csum_field(proto, trans); > ^~~~~ > drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:10: note: > remove the 'if' if its condition is always true > } else if (skb->protocol == htons(ETH_P_IPV6)) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:270:11: note: > initialize the variable 'proto' to silence this warning > u8 proto; > ^ > = '\0' > 1 warning generated. > > [...] Here is the summary with links: - [net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning https://git.kernel.org/netdev/net-next/c/118de6106735 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 2021-06-03 16:40, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This patch was applied to netdev/net-next.git (refs/heads/master): > > On Thu, 3 Jun 2021 10:34:10 -0700 you wrote: >> Clang warns that proto in rmnet_map_v5_checksum_uplink_packet() might >> be >> used uninitialized: >> >> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:14: warning: >> variable 'proto' is used uninitialized whenever 'if' condition is >> false >> [-Wsometimes-uninitialized] >> } else if (skb->protocol == htons(ETH_P_IPV6)) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:295:36: note: >> uninitialized use occurs here >> check = rmnet_map_get_csum_field(proto, trans); >> ^~~~~ >> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:10: note: >> remove the 'if' if its condition is always true >> } else if (skb->protocol == htons(ETH_P_IPV6)) { >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:270:11: note: >> initialize the variable 'proto' to silence this warning >> u8 proto; >> ^ >> = '\0' >> 1 warning generated. >> >> [...] > > Here is the summary with links: > - [net-next] net: ethernet: rmnet: Restructure if checks to avoid > uninitialized warning > https://git.kernel.org/netdev/net-next/c/118de6106735 > > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html Hi Nathan Can you tell why CLANG detected this error. Does it require a bug fix.
Hi Subash, On 6/3/2021 10:15 PM, subashab@codeaurora.org wrote: > On 2021-06-03 16:40, patchwork-bot+netdevbpf@kernel.org wrote: >> Hello: >> >> This patch was applied to netdev/net-next.git (refs/heads/master): >> >> On Thu, 3 Jun 2021 10:34:10 -0700 you wrote: >>> Clang warns that proto in rmnet_map_v5_checksum_uplink_packet() might be >>> used uninitialized: >>> >>> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:14: warning: >>> variable 'proto' is used uninitialized whenever 'if' condition is false >>> [-Wsometimes-uninitialized] >>> } else if (skb->protocol == htons(ETH_P_IPV6)) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:295:36: note: >>> uninitialized use occurs here >>> check = rmnet_map_get_csum_field(proto, trans); >>> ^~~~~ >>> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:10: note: >>> remove the 'if' if its condition is always true >>> } else if (skb->protocol == htons(ETH_P_IPV6)) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:270:11: note: >>> initialize the variable 'proto' to silence this warning >>> u8 proto; >>> ^ >>> = '\0' >>> 1 warning generated. >>> >>> [...] >> >> Here is the summary with links: >> - [net-next] net: ethernet: rmnet: Restructure if checks to avoid >> uninitialized warning >> https://git.kernel.org/netdev/net-next/c/118de6106735 >> >> You are awesome, thank you! >> -- >> Deet-doot-dot, I am a bot. >> https://korg.docs.kernel.org/patchwork/pwbot.html > > Hi Nathan > > Can you tell why CLANG detected this error. > Does it require a bug fix. As far as I understand it, clang does not remember the conditions of previous if statements when generating this warning. Basically: void bar(int x) { } int foo(int a, int b) { int x; if (!a && !b) goto out; if (a) x = 1; else if (b) x = 2; bar(x); out: return 0; } clang will warn that x is uninitialized when neither of the second if statement's conditions are true, even though we as humans know that is not possible due to the first if statement. I am guessing this has something to do with how clang generates its control flow graphs. While this is a false positive, I do not personally see this as a bug in the compiler. The code is more clear to both the compiler and humans if it is written as: if (a) x = 1; else if (b) x = 2; else goto out; Cheers, Nathan
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c index 6492ec5bdec4..cecf72be5102 100644 --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c @@ -269,27 +269,20 @@ static void rmnet_map_v5_checksum_uplink_packet(struct sk_buff *skb, void *trans; u8 proto; - if (skb->protocol != htons(ETH_P_IP) && - skb->protocol != htons(ETH_P_IPV6)) { - priv->stats.csum_err_invalid_ip_version++; - goto sw_csum; - } - if (skb->protocol == htons(ETH_P_IP)) { u16 ip_len = ((struct iphdr *)iph)->ihl * 4; proto = ((struct iphdr *)iph)->protocol; trans = iph + ip_len; - } else if (skb->protocol == htons(ETH_P_IPV6)) { -#if IS_ENABLED(CONFIG_IPV6) + } else if (IS_ENABLED(CONFIG_IPV6) && + skb->protocol == htons(ETH_P_IPV6)) { u16 ip_len = sizeof(struct ipv6hdr); proto = ((struct ipv6hdr *)iph)->nexthdr; trans = iph + ip_len; -#else + } else { priv->stats.csum_err_invalid_ip_version++; goto sw_csum; -#endif /* CONFIG_IPV6 */ } check = rmnet_map_get_csum_field(proto, trans);
Clang warns that proto in rmnet_map_v5_checksum_uplink_packet() might be used uninitialized: drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:14: warning: variable 'proto' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] } else if (skb->protocol == htons(ETH_P_IPV6)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:295:36: note: uninitialized use occurs here check = rmnet_map_get_csum_field(proto, trans); ^~~~~ drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:283:10: note: remove the 'if' if its condition is always true } else if (skb->protocol == htons(ETH_P_IPV6)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:270:11: note: initialize the variable 'proto' to silence this warning u8 proto; ^ = '\0' 1 warning generated. This is technically a false positive because there is an if statement above this one that checks skb->protocol for not being either ETH_P_IP{,V6}. However, it is more obvious to sink that into the if statement as an else branch, which makes the code clearer and fixes the warning. At the same time, move the "IS_ENABLED(CONFIG_IPV6)" into the else if condition so that the else branch of the preprocessor conditional can be shared, since there is no build failure with CONFIG_IPV6 disabled. Fixes: b6e5d27e32ef ("net: ethernet: rmnet: Add support for MAPv5 egress packets") Link: https://github.com/ClangBuiltLinux/linux/issues/1390 Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) base-commit: 270d47dc1fc4756a0158778084a236bc83c156d2