On 8/8/19 10:27 AM, Catalin Marinas wrote:

On Wed, Aug 07, 2019 at 01:38:16PM -0700, Dave Hansen wrote:
Extending the interface is still possible even with the current
proposal, by changing arg2 etc. We also don't seem to be consistent in
sys_prctl().

We are not consistent because it took a long time to learn this lesson,
but I think this is a best-practice that we follow for new ones.

quoted

Also, shouldn't this be converted over to an arch_prctl()?

What do you mean by arch_prctl()? We don't have such thing, apart from
maybe arch_prctl_spec_ctrl_*(). We achieve the same thing with the
{SET,GET}_TAGGED_ADDR_CTRL macros. They could be renamed to
arch_prctl_tagged_addr_{set,get} or something but I don't see much
point.

Silly me.  We have an x86-specific:

	SYSCALL_DEFINE2(arch_prctl, int , option, unsigned long , arg2)

I guess there's no ARM equivalent so you're stuck with the generic one.

What would be better (for a separate patch series) is to clean up
sys_prctl() and move the arch-specific options into separate
arch_prctl() under arch/*/kernel/. But it's not really for this series.

I think it does make sense for truly arch-specific features to stay out
of the arch-generic prctl().  Yes, I know I've personally violated this
in the past. :)

quoted

What is the scope of these prctl()'s?  Are they thread-scoped or
process-scoped?  Can two threads in the same process run with different
tagging ABI modes?

Good point. They are thread-scoped and this should be made clear in the
doc. Two threads can have different modes.

The expectation is that this is invoked early during process start (by
the dynamic loader or libc init) while in single-thread mode and
subsequent threads will inherit the same mode. However, other uses are
possible.

If that's the expectation, it would be really nice to codify it.
Basically, you can't enable the feature if another thread is already
been forked off.

That said, do we have a precedent for changing user ABI from the kernel
cmd line? 'noexec32', 'vsyscall' I think come close. With the prctl()
for opt-in, controlling this from the cmd line is not too bad (though my
preference is still for the sysctl).

There are certainly user-visible things like being able to select
various CPU features.

quoted

quoted

+When a process has successfully enabled the new ABI by invoking
+prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE), the following
+behaviours are guaranteed:
+
+- Every currently available syscall, except the cases mentioned in section
+  3, can accept any valid tagged pointer. The same rule is applicable to
+  any syscall introduced in the future.
+
+- The syscall behaviour is undefined for non valid tagged pointers.

Do you really mean "undefined"?  I mean, a bad pointer is a bad pointer.
 Why should it matter if it's a tagged bad pointer or an untagged bad
pointer?

Szabolcs already replied here. We may have tagged pointers that can be
dereferenced just fine but being passed to the kernel may not be well
defined (e.g. some driver doing a find_vma() that fails unless it
explicitly untags the address). It's as undefined as the current
behaviour (without these patches) guarantees.

It might just be nicer to point out what this features *changes* about
invalid pointer handling, which is nothing. :)  Maybe:

	The syscall behaviour for invalid pointers is the same for both
	tagged and untagged pointers.

quoted

quoted

+- prctl(PR_SET_MM, ``*``, ...) other than arg2 PR_SET_MM_MAP and
+  PR_SET_MM_MAP_SIZE.
+
+- prctl(PR_SET_MM, PR_SET_MM_MAP{,_SIZE}, ...) struct prctl_mm_map fields.
+
+Any attempt to use non-zero tagged pointers will lead to undefined
+behaviour.

I wonder if you want to generalize this a bit.  I think you're saying
that parts of the ABI that modify the *layout* of the address space
never accept tagged pointers.

I guess our difficulty in specifying this may have been caused by
over-generalising. For example, madvise/mprotect came under the same
category but there is a use-case for malloc'ed pointers (and tagged) to
the kernel (e.g. MADV_DONTNEED). If we can restrict the meaning to
address space *layout* manipulation, we'd have mmap/mremap/munmap,
brk/sbrk, prctl(PR_SET_MM). Did I miss anything?. Other related syscalls
like mprotect/madvise preserve the layout while only changing permissions,
backing store, so the would be allowed to accept tags.

shmat() comes to mind.  I also did a quick grep for mmap_sem taken for
write and didn't see anything else obvious jump out at me.

quoted

It looks like the TAG_SHIFT and tag size are pretty baked into the
aarch64 architecture.  But, are you confident that no future
implementations will want different positions or sizes?  (obviously
controlled by other TCR_EL1 bits)

For the top-byte-ignore (TBI), that's been baked in the architecture
since ARMv8.0 and we'll have to keep the backwards compatible mode. As
the name implies, it's the top byte of the address and that's what the
document above refers to.

With MTE, I can't exclude other configurations in the future but I'd
expect the kernel to present the option as a new HWCAP and the user to
explicitly opt in via a new prctl() flag. I seriously doubt we'd break
existing binaries. So, yes TAG_SHIFT may be different but so would the
prctl() above.

Basically, what you have is a "turn tagging on" and "turn tagging off"
call which are binary: all on or all off.  How about exposing a mask:

	/* Replace hard-coded mask size/position: */
	unsigned long mask = prctl(GET_POSSIBLE_TAGGED_ADDR_BITS);

	if (mask == 0)
		// no tagging is supported obviously

	prctl(SET_TAGGED_ADDR_BITS, mask);

	// now userspace knows via 'mask' where the tag bits are



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel