arm64 fixes for 5.11-rc6

arm64 fixes:

- Fix the virt_addr_valid() returning true for < PAGE_OFFSET addresses.

Quoted 4 lines · Catalin Marinas

arm64 fixes:

- Fix the virt_addr_valid() returning true for < PAGE_OFFSET addresses.

That's a really odd fix.

It went from an incorrect bitwise operation (masking) to an _odd_ bitwise operation (xor).

Yes, PAGE_OFFSET has the bit pattern of all upper bits set, so "(addr ^ PAGE_OFFSET)" by definition reverses the upper bits - and for a valid case turns them to zero.

But isn't the *logical* thing to do to use a subtract instead? For the valid cases, the two do the same thing (clear the upper bits), but just conceptually, isn't the operation that you actually want to do "(addr - PAGE_OFFSET)"?

IOW, why is it using that odd xor pattern that doesn't make much sense? I believe it _works_, but it looks very strange to me.

Also, shouldn't _lm_to_phys() do the same? It does that "mask upper bits" too that was problematic in __is_lm_address(). Again, shouldn't that logically be a subtract op?

Quoted 16 lines · Linus Torvalds

Quoted 4 lines · Catalin Marinas

arm64 fixes:

- Fix the virt_addr_valid() returning true for < PAGE_OFFSET addresses.

That's a really odd fix.

It went from an incorrect bitwise operation (masking) to an _odd_ bitwise operation (xor).

Yes, PAGE_OFFSET has the bit pattern of all upper bits set, so "(addr ^ PAGE_OFFSET)" by definition reverses the upper bits - and for a valid case turns them to zero.

Quoted 7 lines

But isn't the *logical* thing to do to use a subtract instead? For the valid cases, the two do the same thing (clear the upper bits), but just conceptually, isn't the operation that you actually want to do "(addr - PAGE_OFFSET)"?

IOW, why is it using that odd xor pattern that doesn't make much sense? I believe it _works_, but it looks very strange to me.

This macro used to test a single bit and it evolved into a bitmask. So, yes, basically what we need is:

#define __is_lm_address(addr)   ((u64)(addr) >= PAGE_OFFSET && \
                                 (u64)(addr) < PAGE_END)

I wasn't sure whether the code generation with two comparisons is similar to the xor variant but the compiler should probably be smart enough to use CMP and CCMP. In the grand scheme, it probably doesn't even matter.

Unless I miss something, I don't see any overflow issues even if we do (((u64)addr - PAGE_OFFSET) < (PAGE_END - PAGE_OFFSET)).

We can backport the fix already upstream and clean-up the code in mainline going forward (after some sanity check on the code generation). It would be easier to parse in the future.

Quoted 3 lines

Also, shouldn't _lm_to_phys() do the same? It does that "mask upper bits" too that was problematic in __is_lm_address(). Again, shouldn't that logically be a subtract op?

Yes, that's similar and a subtract should do.

Quoted 43 lines · Catalin Marinas

Quoted 16 lines · Linus Torvalds

Quoted 4 lines · Catalin Marinas

arm64 fixes:

- Fix the virt_addr_valid() returning true for < PAGE_OFFSET addresses.

That's a really odd fix.

It went from an incorrect bitwise operation (masking) to an _odd_ bitwise operation (xor).

Yes, PAGE_OFFSET has the bit pattern of all upper bits set, so "(addr ^ PAGE_OFFSET)" by definition reverses the upper bits - and for a valid case turns them to zero.

Quoted 7 lines

But isn't the *logical* thing to do to use a subtract instead? For the valid cases, the two do the same thing (clear the upper bits), but just conceptually, isn't the operation that you actually want to do "(addr - PAGE_OFFSET)"?

IOW, why is it using that odd xor pattern that doesn't make much sense? I believe it _works_, but it looks very strange to me.

This macro used to test a single bit and it evolved into a bitmask. So, yes, basically what we need is:

#define __is_lm_address(addr)   ((u64)(addr) >= PAGE_OFFSET && \
                                 (u64)(addr) < PAGE_END)

I wasn't sure whether the code generation with two comparisons is similar to the xor variant but the compiler should probably be smart enough to use CMP and CCMP. In the grand scheme, it probably doesn't even matter.

Unless I miss something, I don't see any overflow issues even if we do (((u64)addr - PAGE_OFFSET) < (PAGE_END - PAGE_OFFSET)).

We can backport the fix already upstream and clean-up the code in mainline going forward (after some sanity check on the code generation). It would be easier to parse in the future.

Quoted 5 lines

Quoted 3 lines

Also, shouldn't _lm_to_phys() do the same? It does that "mask upper bits" too that was problematic in __is_lm_address(). Again, shouldn't that logically be a subtract op?

Yes, that's similar and a subtract should do.

The original bit test was written like that because it removes the need to reason about a potential tag in the upper bits. I tried to preserve that behavior when removing the guaranteed 1:1 split between the vmalloc and linear regions, by masking with PAGE_OFFSET and comparing with PAGE_END - PAGE_OFFSET, but unfortunately, both approaches suffer from the issue fixed by this patch, i.e., that virt_addr_valid(0x0) erroneously returns true.

I think both proposed fixes are appropriate, but they both reintroduce the need to consider the tag. I don't know whether or where this could pose a problem, but it needs to be taken into account.