Re: [RFC][PATCH] Pid namespaces vs locks interaction

["Serge E. Hallyn" <serue@us.ibm.com>]

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