On Thu, Jun 4, 2015 at 5:36 PM, Al Viro <v...@zeniv.linux.org.uk> wrote: > > So basically that thing will trigger only on the last pass through > the outer loop. The only way for it to trigger a wraparound would > be to have sizeof(long)/sizeof(compat_long_t) greater than LONG_MAX, > which is, not too likely.
No. You missed that the "nr_compat_longs-- > 0" thing is an *unsigned* comparison. So the bug would trigger whenever the last pass through the outer loop would then have an inner loop that decremented it from 0 to ULONG_MAX. However, it turns out that since sizeof(long)/sizeof(compat_long_t) in practice is always just 2, the decrement itself can happen only twice, and since we only ever enter with nr_compat_longs being non-zero, only the second decrement can do that overflow thing. And since that's the last iteration of the inner loop (_and_ the outer loop), it doesn't matter that the value ends up having overflowed So the code _works_. But it works almost by accident, and it sure as hell does not need that "greater than LONG_MAX" thing you think. Anything bigger than 2 would cause an active bug. So the whole "nr_compat_longs-- > 0" is just broken. It happens to work, but it is ugly and wrong. It's also pointless, when we might as well just move the decrement into the conditional, and avoid the whole "it could overflow/underflow" discussion entirely, and make the whole signed-vs-unsigned a total non-issue. So the patch is a good cleanup. It doesn't fix any actual bugs, but it definitely fixes potential bugs (ie if somebody were to have a 32-bit compat mode on a 128-bit native kernel - something we don't actually have yet, but an example of a situation that would trigger the bug) Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/