Re: [PATCHv5 06/13] x86/mm: Provide ARCH_GET_UNTAG_MASK and ARCH_ENABLE_TAGGED_ADDR

[Alexander Potapenko]

On Wed, Jul 20, 2022 at 2:57 AM Kirill A. Shutemov
<kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Jul 18, 2022 at 07:47:44PM +0200, Alexander Potapenko wrote:
> > On Wed, Jul 13, 2022 at 1:13 AM Kirill A. Shutemov
> > <kirill.shutemov@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Add a couple of arch_prctl() handles:
> > >
> > >  - ARCH_ENABLE_TAGGED_ADDR enabled LAM. The argument is required number
> > >    of tag bits. It is rounded up to the nearest LAM mode that can
> > >    provide it. For now only LAM_U57 is supported, with 6 tag bits.
> > >
> > >  - ARCH_GET_UNTAG_MASK returns untag mask. It can indicates where tag
> > >    bits located in the address.
> > >
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > > ---
> > >  arch/x86/include/uapi/asm/prctl.h |  3 ++
> > >  arch/x86/kernel/process_64.c      | 60 ++++++++++++++++++++++++++++++-
> > >  2 files changed, 62 insertions(+), 1 deletion(-)
> >
> >
> > > +
> > > +static int prctl_enable_tagged_addr(struct mm_struct *mm, unsigned long nr_bits)
> > > +{
> > > +       int ret = 0;
> > > +
> > > +       if (!cpu_feature_enabled(X86_FEATURE_LAM))
> > > +               return -ENODEV;
> >
> > Hm, I used to think ENODEV is specific to devices, and -EINVAL is more
> > appropriate here.
> > On the other hand, e.g. prctl(PR_SET_SPECULATION_CTRL) can also return ENODEV...
>
> I'm fine either way. Although there are way too many -EINVALs around, so
> it does not communicate much to user.
>
> > >  long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
> > >  {
> > >         int ret = 0;
> > > @@ -829,7 +883,11 @@ long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
> > >         case ARCH_MAP_VDSO_64:
> > >                 return prctl_map_vdso(&vdso_image_64, arg2);
> > >  #endif
> > > -
> > > +       case ARCH_GET_UNTAG_MASK:
> > > +               return put_user(task->mm->context.untag_mask,
> > > +                               (unsigned long __user *)arg2);
> >
> > Can we have ARCH_GET_UNTAG_MASK return the same error value (ENODEV or
> > EINVAL) as ARCH_ENABLE_TAGGED_ADDR in the case the host doesn't
> > support LAM?
> > After all, the mask does not make much sense in this case.
>
> I'm not sure about this.
>
> As it is ARCH_GET_UNTAG_MASK returns -1UL mask if LAM is not present or
> not enabled. Applying this mask will give correct result for both.

Is anyone going to use this mask if tagging is unsupported?
Tools like HWASan won't even try to proceed in that case.

> Why is -ENODEV better here? Looks like just more work for userspace.

This boils down to the question of detecting LAM support I raised previously.
It's nice to have a syscall without side effects to check whether LAM
can be enabled at all (e.g. one can do the check in the parent process
and conditionally enable LAM in certain, but not all, child processes)
CPUID won't help here, because the presence of the LAM bit in CPUID
doesn't guarantee its support in the kernel, and every other solution
is more complicated than just issuing a system call.

Note that TBI has PR_GET_TAGGED_ADDR_CTRL, which can be used to detect
the presence of memory tagging support.

>
> >
> > > +       case ARCH_ENABLE_TAGGED_ADDR:
> > > +               return prctl_enable_tagged_addr(task->mm, arg2);
> > >         default:
> > >                 ret = -EINVAL;
> > >                 break;
> > > --
> > > 2.35.1
> > >
> >
> >
> > --
> > Alexander Potapenko
> > Software Engineer
> >
> > Google Germany GmbH
> > Erika-Mann-Straße, 33
> > 80636 München
> >
> > Geschäftsführer: Paul Manicle, Liana Sebastian
> > Registergericht und -nummer: Hamburg, HRB 86891
> > Sitz der Gesellschaft: Hamburg
>
> --
>  Kirill A. Shutemov



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg