[net:master 10/15] net/ipv6/addrconf.c:1251:14-30: WARNING: Unsigned expression compared with zero: tmp_prefered_lft < 0

I haven't checked the entire context, but it could be useful to look at
line 1251.

julia

---------- Forwarded message ----------
Date: Sun, 16 Oct 2016 01:34:18 +0800
From: kbuild test robot <redacted>
To: kbuild@01.org
Cc: Julia Lawall <redacted>
Subject: [net:master 10/15] net/ipv6/addrconf.c:1251:14-30: WARNING: Unsigned
    expression compared with zero: tmp_prefered_lft < 0

CC: kbuild-all@01.org
CC: netdev@vger.kernel.org
TO: Jiri Bohac <redacted>

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
head:   9e55d0f95460a067def5400fa5eee5dabb0fc5a5
commit: 76506a986dc31394fd1f2741db037d29c7e57843 [10/15] IPv6: fix DESYNC_FACTOR
:::::: branch date: 21 hours ago
:::::: commit date: 27 hours ago

quoted

net/ipv6/addrconf.c:1251:14-30: WARNING: Unsigned expression compared with zero: tmp_prefered_lft < 0

git remote add net https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git
git remote update net
git checkout 76506a986dc31394fd1f2741db037d29c7e57843
vim +1251 net/ipv6/addrconf.c

76506a986 Jiri Bohac     2016-10-13  1235  	if (unlikely(idev->desync_factor > max_desync_factor)) {
76506a986 Jiri Bohac     2016-10-13  1236  		if (max_desync_factor > 0) {
76506a986 Jiri Bohac     2016-10-13  1237  			get_random_bytes(&idev->desync_factor,
76506a986 Jiri Bohac     2016-10-13  1238  					 sizeof(idev->desync_factor));
76506a986 Jiri Bohac     2016-10-13  1239  			idev->desync_factor %= max_desync_factor;
76506a986 Jiri Bohac     2016-10-13  1240  		} else {
76506a986 Jiri Bohac     2016-10-13  1241  			idev->desync_factor = 0;
76506a986 Jiri Bohac     2016-10-13  1242  		}
76506a986 Jiri Bohac     2016-10-13  1243  	}
76506a986 Jiri Bohac     2016-10-13  1244
^1da177e4 Linus Torvalds 2005-04-16  1245  	tmp_valid_lft = min_t(__u32,
^1da177e4 Linus Torvalds 2005-04-16  1246  			      ifp->valid_lft,
7a876b0ef Glenn Wurster  2010-09-27  1247  			      idev->cnf.temp_valid_lft + age);
76506a986 Jiri Bohac     2016-10-13  1248  	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
76506a986 Jiri Bohac     2016-10-13  1249  			    idev->desync_factor;
76506a986 Jiri Bohac     2016-10-13  1250  	/* guard against underflow in case of concurrent updates to cnf */
76506a986 Jiri Bohac     2016-10-13 @1251  	if (unlikely(tmp_prefered_lft < 0))
76506a986 Jiri Bohac     2016-10-13  1252  		tmp_prefered_lft = 0;
76506a986 Jiri Bohac     2016-10-13  1253  	tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
^1da177e4 Linus Torvalds 2005-04-16  1254  	tmp_plen = ifp->prefix_len;
^1da177e4 Linus Torvalds 2005-04-16  1255  	tmp_tstamp = ifp->tstamp;
^1da177e4 Linus Torvalds 2005-04-16  1256  	spin_unlock_bh(&ifp->lock);
^1da177e4 Linus Torvalds 2005-04-16  1257
53bd67491 Jiri Pirko     2013-12-06  1258  	write_unlock_bh(&idev->lock);
95c385b4d Neil Horman    2007-04-25  1259

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Commit 76506a986dc31394fd1f2741db037d29c7e57843 (IPv6: fix
DESYNC_FACTOR) introduced a buggy check for underflow of
tmp_prefered_lft. tmp_prefered_lft is unsigned, so the condition
is always false.

Signed-off-by: Jiri Bohac <redacted>
Reported-by: Julia Lawall <redacted>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..7a043a4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -1248,7 +1248,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
 	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
 			    idev->desync_factor;
 	/* guard against underflow in case of concurrent updates to cnf */
-	if (unlikely(tmp_prefered_lft < 0))
+	if (unlikely((long)tmp_prefered_lft < 0))
 		tmp_prefered_lft = 0;
 	tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
 	tmp_plen = ifp->prefix_len;

From: Jiri Bohac <redacted>
Date: Tue, 18 Oct 2016 17:01:54 +0200

Commit 76506a986dc31394fd1f2741db037d29c7e57843 (IPv6: fix
DESYNC_FACTOR) introduced a buggy check for underflow of
tmp_prefered_lft. tmp_prefered_lft is unsigned, so the condition
is always false.

Signed-off-by: Jiri Bohac <redacted>
Reported-by: Julia Lawall <redacted>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

Does the check make any sense at all?  I'd say just remove it.

The check for an underflow of tmp_prefered_lft is always false
because tmp_prefered_lft is unsigned.

The intention of the check was to guard against racing with an
update of the temp_prefered_lft sysctl, potentially resulting in
an underflow and a very large preferred lifetime. However, the
result of the check in such a situation would be not creating the
temporary address at all, which might be an even worse outcome
than the bogus lifetime.

Drop the faulty check.

Signed-off-by: Jiri Bohac <redacted>
Reported-by: Julia Lawall <redacted>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..f7c7c2b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -1247,9 +1247,6 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
 			      idev->cnf.temp_valid_lft + age);
 	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
 			    idev->desync_factor;
-	/* guard against underflow in case of concurrent updates to cnf */
-	if (unlikely(tmp_prefered_lft < 0))
-		tmp_prefered_lft = 0;
 	tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
 	tmp_plen = ifp->prefix_len;
 	tmp_tstamp = ifp->tstamp;

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Hi,

On Tue, Oct 18, 2016 at 02:25:25PM -0400, David Miller wrote:

Does the check make any sense at all?  I'd say just remove it.

The purpose was to guard against the user updating the
temp_prefered_lft sysctl after this:

        max_desync_factor = min_t(__u32,
                                  idev->cnf.max_desync_factor,
                                  idev->cnf.temp_prefered_lft - regen_advance);

but before this:

	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
			    idev->desync_factor;


With enough bad luck, tmp_prefered_lft could underflow and the resulting
preferred lifetime could be almost infinity.

On the other hand, with this check, this situation will result
with the temporary address not being created at all, which might
be even worse. So if you prefer it, just drop the check.
Patch in a follow-up e-mail.

Thanks,

-- 
Jiri Bohac [off-list ref]
SUSE Labs, SUSE CZ

From: Jiri Bohac <redacted>
Date: Wed, 19 Oct 2016 15:16:36 +0200

The purpose was to guard against the user updating the
temp_prefered_lft sysctl after this:

        max_desync_factor = min_t(__u32,
                                  idev->cnf.max_desync_factor,
                                  idev->cnf.temp_prefered_lft - regen_advance);

but before this:

	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
			    idev->desync_factor;

That's a different problem.

Read the sysctl values of interest into local variables using
READ_ONCE() before the calculations, that way the situation your
describe is impossible.

The check for an underflow of tmp_prefered_lft is always false
because tmp_prefered_lft is unsigned. The intention of the check
was to guard against racing with an update of the
temp_prefered_lft sysctl, potentially resulting in an underflow.

As suggested by David Miller, the best way to prevent the race is
by reading the sysctl variable using READ_ONCE.

Signed-off-by: Jiri Bohac <redacted>
Reported-by: Julia Lawall <redacted>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..060dd99 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c

@@ -1185,6 +1185,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
 	u32 addr_flags;
 	unsigned long now = jiffies;
 	long max_desync_factor;
+	s32 cnf_temp_preferred_lft;
 
 	write_lock_bh(&idev->lock);
 	if (ift) {

@@ -1228,9 +1229,10 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
 	/* recalculate max_desync_factor each time and update
 	 * idev->desync_factor if it's larger
 	 */
+	cnf_temp_preferred_lft = READ_ONCE(idev->cnf.temp_prefered_lft);
 	max_desync_factor = min_t(__u32,
 				  idev->cnf.max_desync_factor,
-				  idev->cnf.temp_prefered_lft - regen_advance);
+				  cnf_temp_preferred_lft - regen_advance);
 
 	if (unlikely(idev->desync_factor > max_desync_factor)) {
 		if (max_desync_factor > 0) {

@@ -1245,11 +1247,8 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
 	tmp_valid_lft = min_t(__u32,
 			      ifp->valid_lft,
 			      idev->cnf.temp_valid_lft + age);
-	tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
+	tmp_prefered_lft = cnf_temp_preferred_lft + age -
 			    idev->desync_factor;
-	/* guard against underflow in case of concurrent updates to cnf */
-	if (unlikely(tmp_prefered_lft < 0))
-		tmp_prefered_lft = 0;
 	tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
 	tmp_plen = ifp->prefix_len;
 	tmp_tstamp = ifp->tstamp;

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

From: Jiri Bohac <redacted>
Date: Thu, 20 Oct 2016 12:29:26 +0200

The check for an underflow of tmp_prefered_lft is always false
because tmp_prefered_lft is unsigned. The intention of the check
was to guard against racing with an update of the
temp_prefered_lft sysctl, potentially resulting in an underflow.

As suggested by David Miller, the best way to prevent the race is
by reading the sysctl variable using READ_ONCE.

Signed-off-by: Jiri Bohac <redacted>
Reported-by: Julia Lawall <redacted>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")

Applied, thanks Jiri.