Re: [PATCH] per-task delay accounting taskstats interface: control exit data through cpumasks]

Andrew Morton
2006-07-06T23:12:05Z

Thomas Graf <tgraf@suug.ch> wrote:
>
> * Andrew Morton <akpm@osdl.org> 2006-07-06 14:48
> > In the interests of keeping this work decoupled from netlink enhancements
> > I'd propose the below. Is it bad to modify the data at nla_data()?
>
> Yes, it points into a skb data buffer which may be shared by sitting
> on other queues if the message is to be broadcasted. In this case it
> would be harmless but the policy is to leave it unmodified.

Yup, sleazy-but-safe.

>
> > --- a/kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix
> > +++ a/kernel/taskstats.c
> > @@ -299,6 +299,23 @@ cleanup:
> > return 0;
> > }
> >
> > +static int parse(struct nlattr *na, cpumask_t *mask)
> > +{
> > + char *data;
> > + int len;
> > +
> > + if (na == NULL)
> > + return 1;
> > + len = nla_len(na);
> > + if (len > TASKSTATS_CPUMASK_MAXLEN)
> > + return -E2BIG;
> > + if (len < 1)
> > + return -EINVAL;
> > + data = nla_data(na);
> > + data[len - 1] = '\0';
> > + return cpulist_parse(data, *mask);
> > +}
>
> Usually this is done by using strlcpy:

hm, nla_strlcpy() looks more complex than it needs to be. We really need
nla_kstrndup() ;)

Oh well. This?

diff -puN kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix kernel/taskstats.c
--- a/kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix
+++ a/kernel/taskstats.c
@@ -299,6 +299,28 @@ cleanup:
return 0;
}

+static int parse(struct nlattr *na, cpumask_t *mask)
+{
+ char *data;
+ int len;
+ int ret;
+
+ if (na == NULL)
+ return 1;
+ len = nla_len(na);
+ if (len > TASKSTATS_CPUMASK_MAXLEN)
+ return -E2BIG;
+ if (len < 1)
+ return -EINVAL;
+ data = kmalloc(len, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+ nla_strlcpy(data, na, len);
+ ret = cpulist_parse(data, *mask);
+ kfree(data);
+ return ret;
+}
+
static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
{
int rc = 0;
@@ -309,27 +331,17 @@ static int taskstats_user_cmd(struct sk_
struct nlattr *na;
cpumask_t mask;

- if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
- na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
- if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
- return -E2BIG;
- rc = cpulist_parse((char *)nla_data(na), mask);
- if (rc)
- return rc;
- rc = add_del_listener(info->snd_pid, &mask, REGISTER);
+ rc = parse(info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK], &mask);
+ if (rc < 0)
return rc;
- }
+ if (rc == 0)
+ return add_del_listener(info->snd_pid, &mask, REGISTER);

- if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]) {
- na = info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK];
- if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
- return -E2BIG;
- rc = cpulist_parse((char *)nla_data(na), mask);
- if (rc)
- return rc;
- rc = add_del_listener(info->snd_pid, &mask, DEREGISTER);
+ rc = parse(info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK], &mask);
+ if (rc < 0)
return rc;
- }
+ if (rc == 0)
+ return add_del_listener(info->snd_pid, &mask, DEREGISTER);

/*
* Size includes space for nested attributes
_