Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230504170942.822147-1-revest@chromium.org> <20230504170942.822147-4-revest@chromium.org>
 <ZFVMLQHObyP4Na+j@arm.com>
In-Reply-To: <ZFVMLQHObyP4Na+j@arm.com>
From:   Florent Revest <revest@chromium.org>
Date:   Mon, 8 May 2023 14:11:54 +0200
Message-ID: <CABRcYmJ+8vUHDamVhRosY-X_m4ecVyahSektRjaxg4n_gG5VXQ@mail.gmail.com>
Subject: Re: [PATCH 3/4] mm: Add a NO_INHERIT flag to the PR_SET_MDWE prctl
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, anshuman.khandual@arm.com,
        joey.gouly@arm.com, mhocko@suse.com, keescook@chromium.org,
        david@redhat.com, peterx@redhat.com, izbyshev@ispras.ru,
        nd@arm.com, broonie@kernel.org, szabolcs.nagy@arm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 5, 2023 at 8:34=E2=80=AFPM Catalin Marinas <catalin.marinas@arm=
.com> wrote:
>
> On Thu, May 04, 2023 at 07:09:41PM +0200, Florent Revest wrote:
> > This extends the current PR_SET_MDWE prctl arg with a bit to indicate
> > that the process doesn't want MDWE protection to propagate to children.
> >
> > To implement this no-inherit mode, the tag in current->mm->flags must b=
e
> > absent from MMF_INIT_MASK. This means that the encoding for "MDWE but
> > without inherit" is different in the prctl than in the mm flags. This
> > leads to a bit of bit-mangling in the prctl implementation.
>
> That bit mangling is not that bad but it complicates the code a bit,
> especially if we'll add new bits in the future. We also need to check
> both the original and the no-inherit bits for each feature.

I agree :)

> Another question is whether we want to support more fine-grained
> inheriting or just a big knob that disables inheriting for all the
> (future) MDWE flags.

Yep, I can't think of a more fine-grained inheritance model off the
top of my head but it would be good to have a sane base for when the
need arises.

> I think a somewhat simpler way would be to clear the flags on fork(),
> either based on a big MMF_HAS_MDWE_NO_INHERIT knob or individual ones.
> Something like below (completely untested):
>
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredum=
p.h
> index 0ee96ea7a0e9..ca83a0c8d19c 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -91,4 +91,12 @@ static inline int get_dumpable(struct mm_struct *mm)
>                                  MMF_DISABLE_THP_MASK | MMF_HAS_MDWE_MASK=
)
>
>  #define MMF_VM_MERGE_ANY       29
> +
> +#define MMF_INIT_FLAGS(flags)  ({                              \
> +       unsigned long new_flags =3D flags;                        \
> +       if (new_flags & (1UL << MMF_HAS_MDWE_NO_INHERIT))       \
> +               new_flags &=3D ~(1UL << MMF_HAS_MDWE_MASK);       \
> +       new_flags & MMF_INIT_MASK;                              \
> +})
> +
>  #endif /* _LINUX_SCHED_COREDUMP_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index ed4e01daccaa..53f0b68a5451 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1288,7 +1288,7 @@ static struct mm_struct *mm_init(struct mm_struct *=
mm, struct task_struct *p,
>         hugetlb_count_init(mm);
>
>         if (current->mm) {
> -               mm->flags =3D current->mm->flags & MMF_INIT_MASK;
> +               mm->flags =3D MMF_INIT_FLAGS(current->mm->flags);
>                 mm->def_flags =3D current->mm->def_flags & VM_INIT_DEF_MA=
SK;
>         } else {
>                 mm->flags =3D default_dump_filter;
>
> The checks in MMF_INIT_FLAGS() can grow in time if we add more bits in
> there but we still only keep a single flag that determines whether the
> feature is enabled (maybe that's more like bikeshedding at this moment
> when we have a single bit).

Sounds good!

I had considered something like this but I was afraid I'd spill too
much logic into fork.c... I didn't think of making it a neat macro in
coredump.h. That's a good point, I'll do this in v2.

> (fun remark: I see you cc'ed nd@arm.com'; that's not a real person, it's
> what our IT folk asked us to add on cc so that the Exchange server
> doesn't append the legal disclaimer; most lists are covered already
> without such cc but I guess people feel safer to add it, just in case)

Ahah! I mostly just copied the cc list from Joey's series. I remember
wondering why I couldn't find any patch sent by this mysterious ND but
I thought that if they got such a cool username, surely they must have
been at ARM since the early days and have some important role... :)
Then... mister nd won't get to see my v2! Thanks for the heads up.