[PATCH net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()

[PATCH net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()

[Eric Biggers]

The 'if (!key && hash_location)' check in tcp_inbound_md5_hash() implies
that hash_location might be null.  However, later code in the function
dereferences hash_location anyway, without checking for null first.
Fortunately, there is no real bug, since tcp_inbound_md5_hash() is
called only with non-null values of hash_location.

Therefore, remove the unnecessary and misleading null check of
hash_location.  This silences a Smatch static checker warning
(https://lore.kernel.org/netdev/aPi4b6aWBbBR52P1@stanley.mountain/)

Also fix the related comment at the beginning of the function.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 net/ipv4/tcp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index e15b38f6bd2d5..b79da6d393927 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4884,22 +4884,20 @@ static enum skb_drop_reason
 tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 		     const void *saddr, const void *daddr,
 		     int family, int l3index, const __u8 *hash_location)
 {
 	/* This gets called for each TCP segment that has TCP-MD5 option.
-	 * We have 3 drop cases:
-	 * o No MD5 hash and one expected.
-	 * o MD5 hash and we're not expecting one.
-	 * o MD5 hash and its wrong.
+	 * We have 2 drop cases:
+	 * o An MD5 signature is present, but we're not expecting one.
+	 * o The MD5 signature is wrong.
 	 */
 	const struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_md5sig_key *key;
 	u8 newhash[16];
 
 	key = tcp_md5_do_lookup(sk, l3index, saddr, family);
-
-	if (!key && hash_location) {
+	if (!key) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5UNEXPECTED);
 		trace_tcp_hash_md5_unexpected(sk, skb);
 		return SKB_DROP_REASON_TCP_MD5UNEXPECTED;
 	}
 

base-commit: 962ac5ca99a5c3e7469215bf47572440402dfd59
-- 
2.51.0

Re: [PATCH net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()

[Kuniyuki Iwashima]

On Wed, Oct 22, 2025 at 3:12 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> The 'if (!key && hash_location)' check in tcp_inbound_md5_hash() implies
> that hash_location might be null.  However, later code in the function
> dereferences hash_location anyway, without checking for null first.
> Fortunately, there is no real bug, since tcp_inbound_md5_hash() is
> called only with non-null values of hash_location.
>
> Therefore, remove the unnecessary and misleading null check of
> hash_location.  This silences a Smatch static checker warning
> (https://lore.kernel.org/netdev/aPi4b6aWBbBR52P1@stanley.mountain/)
>
> Also fix the related comment at the beginning of the function.
>
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>

Re: [PATCH net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()

[Dmitry Safonov]

On Wed, 22 Oct 2025 at 23:12, Eric Biggers <ebiggers@kernel.org> wrote:
>
> The 'if (!key && hash_location)' check in tcp_inbound_md5_hash() implies
> that hash_location might be null.  However, later code in the function
> dereferences hash_location anyway, without checking for null first.
> Fortunately, there is no real bug, since tcp_inbound_md5_hash() is
> called only with non-null values of hash_location.
>
> Therefore, remove the unnecessary and misleading null check of
> hash_location.  This silences a Smatch static checker warning
> (https://lore.kernel.org/netdev/aPi4b6aWBbBR52P1@stanley.mountain/)
>
> Also fix the related comment at the beginning of the function.
>
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

Yeah, I think it's a left-over from the time when there was only
TCP-MD5 support.

Thanks for cleaning this up,
             Dmitry

Re: [PATCH net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 22 Oct 2025 15:12:09 -0700 you wrote:
> The 'if (!key && hash_location)' check in tcp_inbound_md5_hash() implies
> that hash_location might be null.  However, later code in the function
> dereferences hash_location anyway, without checking for null first.
> Fortunately, there is no real bug, since tcp_inbound_md5_hash() is
> called only with non-null values of hash_location.
> 
> Therefore, remove the unnecessary and misleading null check of
> hash_location.  This silences a Smatch static checker warning
> (https://lore.kernel.org/netdev/aPi4b6aWBbBR52P1@stanley.mountain/)
> 
> [...]

Here is the summary with links:
  - [net-next] tcp: Remove unnecessary null check in tcp_inbound_md5_hash()
    https://git.kernel.org/netdev/net-next/c/05774d7e4201

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html