Re: [PATCH] kernel/taskstats: fix wrong nla type for {cgroup,task}stats policy

[Johannes Berg]

On Thu, 2020-03-26 at 13:08 -0700, Andrew Morton wrote:

> > After our server is upgraded to a newer kernel, we found that it
> > continuesly print a warning in the kernel message. The warning is,
> > [832984.946322] netlink: 'irmas.lc': attribute type 1 has an invalid length.
> > 
> > irmas.lc is one of our container monitor daemons, and it will use
> > CGROUPSTATS_CMD_GET to get the cgroupstats, that is similar with
> > tools/accounting/getdelays.c. We can also produce this warning with
> > getdelays. For example, after running bellow command
> > 	$ ./getdelays -C /sys/fs/cgroup/memory
> > then you can find a warning in dmesg,
> > [61607.229318] netlink: 'getdelays': attribute type 1 has an invalid length.
> > 
> > This warning is introduced in commit 6e237d099fac ("netlink: Relax attr
> > validation for fixed length types"), which is used to check whether
> > attributes using types NLA_U* and NLA_S* have an exact length.
> > 
> > Regarding this issue, the root cause is cgroupstats_cmd_get_policy defines
> > a wrong type as NLA_U32, while it should be NLA_NESTED an its minimal
> > length is NLA_HDRLEN. That is similar to taskstats_cmd_get_policy.
> > 
> > As this behavior change really breaks our application, we'd better
> > cc stable as well.

Can you explain how it breaks the application? I mean, it's really only
printing a message to the kernel log in this case? At least that's what
you're describing.

I think you may be describing it wrong, because an NLA_NESTED is allowed
to be *empty* (but otherwise must have at least 4 bytes just like an
NLA_U32).

That said, I'm not even sure I agree that this fix is right? See below.

> Is it correct to say that although the code has always been incorrect,
> but only kernels after 6e237d099fac need this change?  If so, I'll add
> Fixes:6e237d099fac to guide the -stable backporting.

That doesn't really seem right - 6e237d099fac *relaxed* the checks. If
anything then it ought to point to 28033ae4e0f5 which may have actually
returned an error; but again, need to understand better what really the
issue is.

> > diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> > index e2ac0e3..b90a520 100644
> > --- a/kernel/taskstats.c
> > +++ b/kernel/taskstats.c
> > @@ -35,8 +35,8 @@
> >  static struct genl_family family;
> >  
> >  static const struct nla_policy taskstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
> > -	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
> > -	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
> > +	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_NESTED },
> > +	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_NESTED },


I'm not sure where this is coming from - the kernel evidently uses them
as nested attributes in *outgoing* data (see mk_reply()), but as NLA_U32
in *incoming* data, (see cmd_attr_pid() and cmd_attr_tgid()).

I would generally recommend not doing such a thing as it's messy, but we
do have quite a few such instances cases. In all those cases must the
policy list the incoming policy since that's what the kernel uses to
validate the attributes.

IOW, this part of the change seems _wrong_.


> >   * Make sure they are always aligned.
> >   */
> >  static const struct nla_policy cgroupstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] = {
> > -	[CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_U32 },
> > +	[CGROUPSTATS_CMD_ATTR_FD] = { .type = NLA_NESTED },
> >  };

And same here, actually.

johannes