From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Serge E. Hallyn" Subject: Re: [RFC][PATCH] Pid namespaces vs locks interaction Date: Fri, 7 Dec 2007 08:51:37 -0600 Message-ID: <20071207145137.GA4435@sergelap.austin.ibm.com> References: <200712061411.32159.vgusev@openvz.org> <200712061557.29543.vgusev@openvz.org> <20071206221510.GF10953@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Vitaliy Gusev , linux-fsdevel@vger.kernel.org To: "J. Bruce Fields" Return-path: Received: from e2.ny.us.ibm.com ([32.97.182.142]:58975 "EHLO e2.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbXLGOwj (ORCPT ); Fri, 7 Dec 2007 09:52:39 -0500 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e2.ny.us.ibm.com (8.13.8/8.13.8) with ESMTP id lB7Eq50b009636 for ; Fri, 7 Dec 2007 09:52:05 -0500 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id lB7Eq5Pt136832 for ; Fri, 7 Dec 2007 09:52:05 -0500 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id lB7Eq4aW024307 for ; Fri, 7 Dec 2007 09:52:05 -0500 Content-Disposition: inline In-Reply-To: <20071206221510.GF10953@fieldses.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Quoting J. Bruce Fields (bfields@fieldses.org): > On Thu, Dec 06, 2007 at 03:57:29PM +0300, Vitaliy Gusev wrote: > > I am working on pid namespaces vs locks interaction and want to evaluate the > > idea. > > fcntl(F_GETLK,..) can return pid of process for not current pid namespace (if > > process is belonged to the several namespaces). It is true also for pids > > in /proc/locks. So correct behavior is saving pointer to the struct pid of > > the process lock owner. > > Forgive me, I'm not familiar with pid namespaces. Exactly what bug does > this patch aim to fix? When a task is created inside a private pid namespace, it may know itself as pid 5, while it's "global" pid is 1237. So if it owned a lock, it would be reported as being owned by 1237. The patch replaces the pid number, which may signify different tasks in different namespaces, with the 'struct pid', which uniquely identifies a task. > > @@ -673,14 +682,16 @@ posix_test_lock(struct file *filp, struct file_lock *fl) > > if (posix_locks_conflict(fl, cfl)) > > break; > > } > > - if (cfl) > > + if (cfl) { > > __locks_copy_lock(fl, cfl); > > - else > > + if (cfl->fl_nspid) > > + fl->fl_pid = pid_nr_ns(cfl->fl_nspid, > > + task_active_pid_ns(current)); > > What does pid_nr_ns() do? I took a quick look at the implementation and > didn't get it. For the given 'struct pid', which is a unique light-weight task identifier, it returns the pid number by which it is known in the pid namespace sent as the second argument. So if a process in the initial pid namespace queries the process id of task 1237 mentioned above, pid_nr_ns will return 1237, while a task in the private namespace will get 5. > I tend to think that the pid returned by fcntl(.,F_GETLK,.) shouldn't be > taken too seriously--it may be helpful when debugging--e.g. it might > help an administrator looking for clues as to who's holding some > annoying lock. But it probably shouldn't be depended on for the > correctness of an application. Maybe I'm wrong and there's some reason > we should worry about it more. > > It's also likely to be wrong in the presence of locks held on behalf of > nfs clients. Your stance sounds sane. So I'm ok leaving it as is, or doing the hard work to replace pid_t fl_pid with struct pid fl_pid altogether and having a separate struct user_flock which has a pid number. The problem with the patch as it stands is that at any point you now don't know whether fl_pid is simply unused, is the global pid, or is the pid in a private namespace. Sounds impossible to maintain. -serge