Messages in this thread | | | Date | Wed, 26 Apr 2017 17:53:53 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 2/2] pid_ns: Introduce ioctl to set vector of ns_last_pid's on ns hierarhy |
| |
On 04/17, Kirill Tkhai wrote: > > +struct pidns_ioc_req { > +/* Set vector of last pids in namespace hierarchy */ > +#define PIDNS_REQ_SET_LAST_PID_VEC 0x1 > + unsigned int req; > + void __user *data; > + unsigned int data_size; > + char std_fields[0]; > +};
see below,
> +static long set_last_pid_vec(struct pid_namespace *pid_ns, > + struct pidns_ioc_req *req) > +{ > + char *str, *p; > + int ret = 0; > + pid_t pid; > + > + read_lock(&tasklist_lock); > + if (!pid_ns->child_reaper) > + ret = -EINVAL; > + read_unlock(&tasklist_lock); > + if (ret) > + return ret;
why do you need to check ->child_reaper under tasklist_lock? this looks pointless.
In fact I do not understand how it is possible to hit pid_ns->child_reaper == NULL, there must be at least one task in this namespace, otherwise you can't open a file which has f_op == ns_file_operations, no?
> + if (req->data_size >= PAGE_SIZE) > + return -EINVAL; > + str = vmalloc(req->data_size + 1);
then I don't understand why it makes sense to use vmalloc()
> + if (!str) > + return -ENOMEM; > + if (copy_from_user(str, req->data, req->data_size)) { > + ret = -EFAULT; > + goto out_vfree; > + } > + str[req->data_size] = '\0'; > + > + p = str; > + while (p && *p != '\0') { > + if (!ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN)) { > + ret = -EPERM; > + goto out_vfree; > + } > + > + if (sscanf(p, "%d", &pid) != 1 || pid < 0 || pid > pid_max) { > + ret = -EINVAL; > + goto out_vfree; > + }
Well, this is ioctl(), do we really want to parse the strings?
Can't we make
struct pidns_ioc_req { ... int nr_pids; pid_t pids[0]; }
and just use get_user() in a loop? This way we can avoid vmalloc() or anything else altogether.
Oleg.
| |