Re: [patch v12 13/13] task isolation: only TIF_TASK_ISOL if task isolation is enabled

Marcelo Tosatti
2022-05-03T21:02:29Z

On Wed, Apr 27, 2022 at 09:45:54AM +0200, Thomas Gleixner wrote:
> On Tue, Mar 15 2022 at 12:31, Marcelo Tosatti wrote:
>
> $Subject does not qualify as a parseable sentence.
>
> > This avoids processing of TIF_TASK_ISOL, when returning to userspace,
> > for tasks which do not have task isolation configured.
>
> That's how kernel development works, right:
>
> 1) Add half baken stuff
> 2) Apply duct tape on top
>
> You know exactly, that this is _not_ the way it works.
>
> This whole thing is half thought out tinkerware with [ill|un]defined
> semantics.

It seems to be inline with the remaining TIF_ bits:

if (ti_work & _TIF_NOTIFY_RESUME)
tracehook_notify_resume(regs);

+ if (ti_work & _TIF_TASK_ISOL)
+ task_isol_exit_to_user_mode();
+

And there is even:

--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -83,6 +83,7 @@ struct thread_info {
#define TIF_NEED_RESCHED 3 /* rescheduling necessary */
#define TIF_SINGLESTEP 4 /* reenable singlestep on user return*/
#define TIF_SSBD 5 /* Speculative store bypass disable */
+#define TIF_TASK_ISOL 6 /* task isolation work pending */
#define TIF_SPEC_IB 9 /* Indirect branch speculation mitigation */
#define TIF_SPEC_L1D_FLUSH 10 /* Flush L1D on mm switches (processes) */
#define TIF_USER_RETURN_NOTIFY 11 /* notify kernel of userspace return */

So the purpose of TIF_TASK_ISOL is to condense in a single bit the
question: "is there task isolation work pending?"

By looking at the code, we see the sites where this bit is set are:

1) Task isolation configuration.

@@ -251,6 +257,11 @@ static int cfg_feat_quiesce_set(unsigned
info->quiesce_mask = i_qctrl->quiesce_mask;
info->oneshot_mask = i_qctrl->quiesce_oneshot_mask;
info->conf_mask |= ISOL_F_QUIESCE;
+
+ if ((info->active_mask & ISOL_F_QUIESCE) &&
+ (info->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
+ set_thread_flag(TIF_TASK_ISOL);
+
ret = 0;

2) copy_process (clone / fork):

@@ -303,6 +314,7 @@ int __copy_task_isol(struct task_struct
new_info->active_mask = info->active_mask;

tsk->task_isol_info = new_info;
+ set_ti_thread_flag(task_thread_info(tsk), TIF_TASK_ISOL);

return 0;
}

3) task isolation activation:

@@ -330,6 +342,10 @@ int prctl_task_isol_activate_set(unsigne
info->active_mask = active_mask;
ret = 0;

+ if ((info->active_mask & ISOL_F_QUIESCE) &&
+ (info->quiesce_mask & ISOL_F_QUIESCE_VMSTATS))
+ set_thread_flag(TIF_TASK_ISOL);
+
out:
return ret;

Would you prefer an explanation, in words, when these bits are set, when
they are cleared?

So the meaning is:

+#define TIF_TASK_ISOL 6 /* task isolation work pending */

And the definition is "task isolation work" depends on what task
isolation features are implemented.

(honestly, seem pretty clear to me, but willing to improve...).