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

[Yafang Shao]

On Fri, Mar 27, 2020 at 4:18 AM Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>
> 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()).
>

Thanks for the explanation.
The nested attributes is only used in *outgoing* data, rather than the
'incoming' data.

> 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
>


Thanks
Yafang