From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753642AbdDZQis (ORCPT ); Wed, 26 Apr 2017 12:38:48 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:34628 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752740AbdDZQij (ORCPT ); Wed, 26 Apr 2017 12:38:39 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Kirill Tkhai Cc: Oleg Nesterov , , , , , , , , , , , , , , References: <149245014695.17600.12640895883798122726.stgit@localhost.localdomain> <149245057248.17600.1341652606136269734.stgit@localhost.localdomain> <20170426155352.GA12131@redhat.com> <785e1986-da03-72aa-06c0-234ed2dbc0fd@virtuozzo.com> <005f52d9-efbe-9eaa-7f36-19945c8b06c3@virtuozzo.com> Date: Wed, 26 Apr 2017 11:32:22 -0500 In-Reply-To: <005f52d9-efbe-9eaa-7f36-19945c8b06c3@virtuozzo.com> (Kirill Tkhai's message of "Wed, 26 Apr 2017 19:33:46 +0300") Message-ID: <87h91bcep5.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1d3PxW-0003HG-OR;;;mid=<87h91bcep5.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.233.227;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/gDyfBylctWkoWAbFKqiQsX7s4QjveYa0= X-SA-Exim-Connect-IP: 67.3.233.227 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 1.5 XMNoVowels Alpha-numberic number with no vowels * 0.7 XMSubLong Long Subject * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa06 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_02 5+ unique symbols in subject * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa06 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Kirill Tkhai X-Spam-Relay-Country: X-Spam-Timing: total 5303 ms - load_scoreonly_sql: 0.03 (0.0%), signal_user_changed: 2.7 (0.1%), b_tie_ro: 1.94 (0.0%), parse: 0.79 (0.0%), extract_message_metadata: 11 (0.2%), get_uri_detail_list: 2.0 (0.0%), tests_pri_-1000: 6 (0.1%), tests_pri_-950: 1.21 (0.0%), tests_pri_-900: 0.99 (0.0%), tests_pri_-400: 25 (0.5%), check_bayes: 24 (0.5%), b_tokenize: 9 (0.2%), b_tok_get_all: 8 (0.2%), b_comp_prob: 2.3 (0.0%), b_tok_touch_all: 3.2 (0.1%), b_finish: 0.64 (0.0%), tests_pri_0: 321 (6.1%), check_dkim_signature: 0.54 (0.0%), check_dkim_adsp: 2.4 (0.0%), tests_pri_500: 4930 (93.0%), poll_dns_idle: 4923 (92.8%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 2/2] pid_ns: Introduce ioctl to set vector of ns_last_pid's on ns hierarhy X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kirill Tkhai writes: > On 26.04.2017 19:11, Kirill Tkhai wrote: >> On 26.04.2017 18:53, Oleg Nesterov wrote: >>> 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? >> >> Sure, it's impossible to pick a pid_ns, if there is no the pid_ns's tasks. I added >> it under impression of >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=dfda351c729733a401981e8738ce497eaffcaa00 >> but here it's completely wrong. It will be removed in v2. >> >>>> + 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. >> >> Since it's a generic structure for different types of the requests, it may be extended >> in the future. We won't be able to add new fields, if we compose the structure the way >> you suggested, will we? > > Though, we may go this way if just do the fields generic: > > struct pidns_ioc_req { > unsigned int req; > unsigned int data_size; > union { > pid_t pid[0]; > }; > }; > > Ok, I'll rework the patchset in this way. You don't need that. That is what new ioctl numbers are for. Interfaces to the kernel don't need to become multiplexors to prepare for the future when there is already an appropriate multiplexing interface in place. Eric