[PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

[PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

[Nathan Chancellor]

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(-)

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);

base-commit: 270d47dc1fc4756a0158778084a236bc83c156d2
-- 
2.32.0.rc3

Re: [PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

[patchwork-bot+netdevbpf]

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

Re: [PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

[subashab]

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.

Re: [PATCH net-next] net: ethernet: rmnet: Restructure if checks to avoid uninitialized warning

[Nathan Chancellor]

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