Quoting J. Bruce Fields (bfields@xxxxxxxxxxxx):
> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
