Re: [PATCH v8 -tip 19/26] sched: Add a second-level tag for nested CGroup usecase

[Joel Fernandes]

On Fri, Oct 30, 2020 at 05:42:12PM -0700, Josh Don wrote:
> On Mon, Oct 19, 2020 at 6:45 PM Joel Fernandes (Google)
> <joel@xxxxxxxxxxxxxxxxx> wrote:
> >
> > +static unsigned long cpu_core_get_group_cookie(struct task_group *tg)
> > +{
> > +       unsigned long color = 0;
> > +
> > +       if (!tg)
> > +               return 0;
> > +
> > +       for (; tg; tg = tg->parent) {
> > +               if (tg->core_tag_color) {
> > +                       WARN_ON_ONCE(color);
> > +                       color = tg->core_tag_color;
> > +               }
> > +
> > +               if (tg->core_tagged) {
> > +                       unsigned long cookie = ((unsigned long)tg << 8) | color;
> > +                       cookie &= (1UL << (sizeof(unsigned long) * 4)) - 1;
> > +                       return cookie;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> 
> I'm a bit wary of how core_task_cookie and core_group_cookie are
> truncated to the lower half of their bits and combined into the
> overall core_cookie.  Now that core_group_cookie is further losing 8
> bits to color, that leaves (in the case of 32 bit unsigned long) only
> 8 bits to uniquely identify the group contribution to the cookie.
> 
> Also, I agree that 256 colors is likely adequate, but it would be nice
> to avoid this restriction.
> 
> I'd like to propose the following alternative, which involves creating
> a new struct to represent the core cookie:
> 
> struct core_cookie {
>   unsigned long task_cookie;
>   unsigned long group_cookie;
>   unsigned long color;
>   /* can be further extended with arbitrary fields */
> 
>   struct rb_node node;
>   refcount_t;
> };
> 
> struct rb_root core_cookies; /* (sorted), all active core_cookies */
> seqlock_t core_cookies_lock; /* protects against removal/addition to
> core_cookies */
> 
> struct task_struct {
>   ...
>   unsigned long core_cookie; /* (struct core_cookie *) */
> }
> 
> A given task stores the address of a core_cookie struct in its
> core_cookie field.  When we reconfigure a task's
> color/task_cookie/group_cookie, we can first look for an existing
> core_cookie that matches those settings, or create a new one.

Josh,

This sounds good to me.

Just to mention one thing, for stuff like the following, you'll have to write
functions that can do greater-than, less-than operations, etc.

static inline bool __sched_core_less(struct task_struct *a, struct task_struct *b)
{
	if (a->core_cookie < b->core_cookie)
		return true;

	if (a->core_cookie > b->core_cookie)
		return false;

And pretty much everywhere you do null-checks on core_cookie, or access
core_cookie for any other reasons.

Also there's kselftests that need trivial modifications to pass with the new
changes you propose.

Looking forward to the patch to do the improvement and thanks.

thanks,

 - Joel


> > More information about attacks:
> > For MDS, it is possible for syscalls, IRQ and softirq handlers to leak
> > data to either host or guest attackers. For L1TF, it is possible to leak
> > to guest attackers. There is no possible mitigation involving flushing
> > of buffers to avoid this since the execution of attacker and victims
> > happen concurrently on 2 or more HTs.
> > 
> > Cc: Julien Desfossez <jdesfossez@xxxxxxxxxxxxxxxx>
> > Cc: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> > Cc: Aaron Lu <aaron.lwe@xxxxxxxxx>
> > Cc: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>
> > Cc: Tim Chen <tim.c.chen@xxxxxxxxx>
> > Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > Co-developed-by: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
> > Tested-by: Julien Desfossez <jdesfossez@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Vineeth Pillai <viremana@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > ---
> >   .../admin-guide/kernel-parameters.txt         |   7 +
> >   include/linux/entry-common.h                  |   2 +-
> >   include/linux/sched.h                         |  12 +
> >   kernel/entry/common.c                         |  25 +-
> >   kernel/sched/core.c                           | 229 ++++++++++++++++++
> >   kernel/sched/sched.h                          |   3 +
> >   6 files changed, 275 insertions(+), 3 deletions(-)
> >
The issue is with code like this: