lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] kernel/taskstats: fix wrong nla type for {cgroup,task}stats policy
From
Date
On 3/26/20 2:28 PM, Johannes Berg wrote:
>
> And looking at this ... well, that code is completely wrong?
>
> E.g.
>
> rc = send_cmd(nl_sd, id, mypid, TASKSTATS_CMD_GET,
> cmd_type, &tid, sizeof(__u32));
>
> (cmd_type is one of TASKSTATS_CMD_ATTR_TGID, TASKSTATS_CMD_ATTR_PID)
>
> or it might do
>
> rc = send_cmd(nl_sd, id, mypid, CGROUPSTATS_CMD_GET,
> CGROUPSTATS_CMD_ATTR_FD, &cfd, sizeof(__u32));
>
> so clearly it wants to produce a u32 attribute.
>
> But then
>
> static int send_cmd(int sd, __u16 nlmsg_type, __u32 nlmsg_pid,
> __u8 genl_cmd, __u16 nla_type,
> void *nla_data, int nla_len)
> {
> ...
>
> na = (struct nlattr *) GENLMSG_DATA(&msg);
>
> // this is still fine
>
> na->nla_type = nla_type;
>
> // this is also fine
>
> na->nla_len = nla_len + 1 + NLA_HDRLEN;
>
> // but this??? the nla_len of a netlink attribute should just be
> // the len ... what's NLA_HDRLEN doing here? this isn't nested
> // here we end up just reserving 1+NLA_HDRLEN too much space
>
> memcpy(NLA_DATA(na), nla_data, nla_len);
>
> // but then it anyway only fills the first nla_len bytes, which
> // is just like a regular attribute.
>
> msg.n.nlmsg_len += NLMSG_ALIGN(na->nla_len);
> // note that this is also wrong - it should be
> // += NLA_ALIGN(NLA_HDRLEN + nla_len)
>
>
>
> So really I think what happened here is precisely what we wanted -
> David's kernel patch caught the broken userspace tool.

agreed. The tool needs to be fixed, not the kernel policy.

I do not get the error message with this change as Johannes points out
above:

diff --git a/tools/accounting/getdelays.c b/tools/accounting/getdelays.c
index 8cb504d30384..e90fd133df0e 100644
--- a/tools/accounting/getdelays.c
+++ b/tools/accounting/getdelays.c
@@ -136,7 +136,7 @@ static int send_cmd(int sd, __u16 nlmsg_type, __u32
nlmsg_pid,
msg.g.version = 0x1;
na = (struct nlattr *) GENLMSG_DATA(&msg);
na->nla_type = nla_type;
- na->nla_len = nla_len + 1 + NLA_HDRLEN;
+ na->nla_len = nla_len + NLA_HDRLEN;
memcpy(NLA_DATA(na), nla_data, nla_len);
msg.n.nlmsg_len += NLMSG_ALIGN(na->nla_len);
\
 
 \ /
  Last update: 2020-03-26 22:12    [W:0.087 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site