+cc Kirill for commit
On Tue, Jan 14, 2025 at 02:43:17PM -0500, Liam R. Howlett wrote:
> Hello,
>
> I noticed that mm/madivse.c:do_madvise() calls untagged_addr_remote()
> after validating start.
>
> Looking through git blame shows that this line was moved in
> 428e106ae1ad4 ("mm: Introduce untagged_addr_remote()") [1], with the
> reason being:
>
> The new helper untagged_addr_remote() has to be used when the address
> targets remote process. It requires the mmap lock for target mm to be
> taken.
>
> Although this may be needed, we cannot move the untagging below
> validating the start/end because we have not validated the start/end
> that will be used for the operation, or at least, isn't clear why it's
> okay?
>
> Can anyone tell me why the code today is correct? That is, how can we
> trust the validation of start/end is still okay after we change the
> start/end by untagging the start?
>
> I think we have to move the locking and the untagging above the
> validation for this to work as expected?
>
> [1] https://lore.kernel.org/all/20230312112612.31869-6-kirill.shutemov@xxxxxxxxxxxxxxx/
>
> Thanks,
> Liam
To avoid losing context from IRC discussion, seems to me the only check that
needs to be potentially moved is:
end = start + len;
if (end < start)
return -EINVAL;
However, MADV_HWPOISON, MADV_SOFT_OFFLINE seems fundamentally broken for tagged
addresses:
#ifdef CONFIG_MEMORY_FAILURE
if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE)
return madvise_inject_error(behavior, start, start + len_in);
#endif
^ this is invoked before untagged_addr_remote() is called (as no mmap lock is
acquired) and so no attempt at untagging happens at all...!
We do need to fix this... unless CONFIG_MEMORY_FAILURE somehow automagically
disallows address tagging...
Perhaps need in that case to detect if the address is tagged and do some
horror-show hack, maybe acquire lock and untag and drop lock in that case... Or
maybe make it arch-dependent since it seems only x86 needs to actually hold the
lock for untagging?
Other than this case I think we are good to just put:
end = start + len;
if (end < start)
return -EINVAL;
Below the untagged_addr_remote() invocation?
