| Message ID | f4908c50d34aed05a93f74302f1a6ce410ec9d15.1496781719.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote: > Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to > handle the case where a remote filesystem directly sets fl_pid. In that > case, the fl_pid should not be translated into a local pid namespace. If > the filesystem implements the lock operation, set a flag to return the > lock's fl_pid value directly, rather translate it. > Actually, you're not translating anything for F_GETLK until we get to this patch. Patch #2 in this series removes the fl_nspid field, but the pid translation isn't fixed until here. That does mean a nominal regression here in how fl_pid is reported between the two. Would it be best to squash #2 and #3 together? Or maybe just go ahead and universally translate the fl_pid field until you add the flag in this patch? Also to make sure I understand: task->tgid will always represent the task's pid in the init_pid_ns, right? Other than the minor bisectability concern, I think this looks good. Nice work! > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/locks.c | 22 ++++++++++++++++++---- > include/linux/fs.h | 1 + > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 8d48e4c42ed3..206a46d28bbd 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -2034,8 +2034,10 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > */ > int vfs_test_lock(struct file *filp, struct file_lock *fl) > { > - if (filp->f_op->lock && is_remote_lock(filp)) > + if (filp->f_op->lock && is_remote_lock(filp)) { > + fl->fl_flags |= FL_PID_PRIV; > return filp->f_op->lock(filp, F_GETLK, fl); > + } > posix_test_lock(filp, fl); > return 0; > } > @@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns) > return vnr; > } > > +static pid_t flock_translate_pid(struct file_lock *fl) > +{ > + if (IS_OFDLCK(fl)) > + return -1; > + if (fl->fl_flags & FL_PID_PRIV) > + return fl->fl_pid; > + return locks_translate_pid(fl->fl_pid, task_active_pid_ns(current)); > +} > + > static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) > { > - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; > + flock->l_pid = flock_translate_pid(fl); > #if BITS_PER_LONG == 32 > /* > * Make sure we can represent the posix lock via > @@ -2084,7 +2095,7 @@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) > #if BITS_PER_LONG == 32 > static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl) > { > - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; > + flock->l_pid = flock_translate_pid(fl); > flock->l_start = fl->fl_start; > flock->l_len = fl->fl_end == OFFSET_MAX ? 0 : > fl->fl_end - fl->fl_start + 1; > @@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > unsigned int fl_pid; > struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info; > > - fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); > + if (fl->fl_flags & FL_PID_PRIV) > + fl_pid = fl->fl_pid; > + else > + fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); > /* > * If there isn't a fl_pid don't display who is waiting on > * the lock if we are called from locks_show, or if we are > diff --git a/include/linux/fs.h b/include/linux/fs.h > index b013fac515f7..179496a9719d 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -908,6 +908,7 @@ static inline struct file *get_file(struct file *f) > #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ > #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ > #define FL_LAYOUT 2048 /* outstanding pNFS layout */ > +#define FL_PID_PRIV 4096 /* F_GETLK should report fl_pid */ > > #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE) >
On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote: > Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to > handle the case where a remote filesystem directly sets fl_pid. In that > case, the fl_pid should not be translated into a local pid namespace. If > the filesystem implements the lock operation, set a flag to return the > lock's fl_pid value directly, rather translate it. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/locks.c | 22 ++++++++++++++++++---- > include/linux/fs.h | 1 + > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 8d48e4c42ed3..206a46d28bbd 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -2034,8 +2034,10 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) > */ > int vfs_test_lock(struct file *filp, struct file_lock *fl) > { > - if (filp->f_op->lock && is_remote_lock(filp)) > + if (filp->f_op->lock && is_remote_lock(filp)) { > + fl->fl_flags |= FL_PID_PRIV; > return filp->f_op->lock(filp, F_GETLK, fl); > + } > posix_test_lock(filp, fl); > return 0; > } I do have one concern here with setting the flag at this point. It's possible with NFS that we'll end up setting a lock locally if we have a delegation (also true for CIFS and probably Ceph -- maybe others too). Here though, you're setting FL_PID_PRIV so those locks will always show a l_pid of current->tgid, even when you're querying it from a different pid namespace. If we want to go this route, then you'll also need to fix NFS to clear that flag when it sets the lock locally, and audit other fs' that have a ->lock operation for the same thing. However, I think it might be cleaner to have the filesystems set that flag to opt out of the default pid translation. Thoughts? > @@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns) > return vnr; > } > > +static pid_t flock_translate_pid(struct file_lock *fl) > +{ > + if (IS_OFDLCK(fl)) > + return -1; > + if (fl->fl_flags & FL_PID_PRIV) > + return fl->fl_pid; > + return locks_translate_pid(fl->fl_pid, task_active_pid_ns(current)); > +} > + > static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) > { > - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; > + flock->l_pid = flock_translate_pid(fl); > #if BITS_PER_LONG == 32 > /* > * Make sure we can represent the posix lock via > @@ -2084,7 +2095,7 @@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) > #if BITS_PER_LONG == 32 > static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl) > { > - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; > + flock->l_pid = flock_translate_pid(fl); > flock->l_start = fl->fl_start; > flock->l_len = fl->fl_end == OFFSET_MAX ? 0 : > fl->fl_end - fl->fl_start + 1; > @@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > unsigned int fl_pid; > struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info; > > - fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); > + if (fl->fl_flags & FL_PID_PRIV) > + fl_pid = fl->fl_pid; > + else > + fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); > /* > * If there isn't a fl_pid don't display who is waiting on > * the lock if we are called from locks_show, or if we are > diff --git a/include/linux/fs.h b/include/linux/fs.h > index b013fac515f7..179496a9719d 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -908,6 +908,7 @@ static inline struct file *get_file(struct file *f) > #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ > #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ > #define FL_LAYOUT 2048 /* outstanding pNFS layout */ > +#define FL_PID_PRIV 4096 /* F_GETLK should report fl_pid */ > > #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE) >
Apologies for the delayed response.. On 7 Jun 2017, at 7:40, Jeff Layton wrote: > On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote: >> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to >> handle the case where a remote filesystem directly sets fl_pid. In that >> case, the fl_pid should not be translated into a local pid namespace. If >> the filesystem implements the lock operation, set a flag to return the >> lock's fl_pid value directly, rather translate it. >> > > Actually, you're not translating anything for F_GETLK until we get to > this patch. Patch #2 in this series removes the fl_nspid field, but the > pid translation isn't fixed until here. That does mean a nominal > regression here in how fl_pid is reported between the two. Good catch. > Would it be best to squash #2 and #3 together? Or maybe just go ahead > and universally translate the fl_pid field until you add the flag in > this patch? I'll send a v4 that universally translates the fl_pid field until this patch. I think the first two patches should be separate. > Also to make sure I understand: task->tgid will always represent the > task's pid in the init_pid_ns, right? Yes. (I was incorrect on IRC last week), as is seen in kernel/fork.c: 1748 if (pid != &init_struct_pid) { 1749 pid = alloc_pid(p->nsproxy->pid_ns_for_children); ... 1754 } ... 1785 p->pid = pid_nr(pid); 1786 if (clone_flags & CLONE_THREAD) { ... 1789 p->tgid = current->tgid; 1790 } else { ... 1796 p->tgid = p->pid; 1797 } .. and include/linux/pid.h: 153 /* 154 * the helpers to get the pid's id seen from different namespaces ... 156 * pid_nr() : global id, i.e. the id seen from the init namespace; ... 162 */ 163 164 static inline pid_t pid_nr(struct pid *pid) 165 { 166 pid_t nr = 0; 167 if (pid) 168 nr = pid->numbers[0].nr; 169 return nr; 170 } Ben
On 19 Jun 2017, at 8:37, Benjamin Coddington wrote: > Apologies for the delayed response.. > > On 7 Jun 2017, at 7:40, Jeff Layton wrote: > >> On Tue, 2017-06-06 at 16:45 -0400, Benjamin Coddington wrote: >>> Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to >>> handle the case where a remote filesystem directly sets fl_pid. In that >>> case, the fl_pid should not be translated into a local pid namespace. If >>> the filesystem implements the lock operation, set a flag to return the >>> lock's fl_pid value directly, rather translate it. >>> >> >> Actually, you're not translating anything for F_GETLK until we get to >> this patch. Patch #2 in this series removes the fl_nspid field, but the >> pid translation isn't fixed until here. That does mean a nominal >> regression here in how fl_pid is reported between the two. > > Good catch. > >> Would it be best to squash #2 and #3 together? Or maybe just go ahead >> and universally translate the fl_pid field until you add the flag in >> this patch? > > I'll send a v4 that universally translates the fl_pid field until this > patch. I think the first two patches should be separate. Ah, but /2 and 3/ should just be squashed, yes I agree with that. Ben
diff --git a/fs/locks.c b/fs/locks.c index 8d48e4c42ed3..206a46d28bbd 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -2034,8 +2034,10 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd) */ int vfs_test_lock(struct file *filp, struct file_lock *fl) { - if (filp->f_op->lock && is_remote_lock(filp)) + if (filp->f_op->lock && is_remote_lock(filp)) { + fl->fl_flags |= FL_PID_PRIV; return filp->f_op->lock(filp, F_GETLK, fl); + } posix_test_lock(filp, fl); return 0; } @@ -2060,9 +2062,18 @@ static pid_t locks_translate_pid(int init_nr, struct pid_namespace *ns) return vnr; } +static pid_t flock_translate_pid(struct file_lock *fl) +{ + if (IS_OFDLCK(fl)) + return -1; + if (fl->fl_flags & FL_PID_PRIV) + return fl->fl_pid; + return locks_translate_pid(fl->fl_pid, task_active_pid_ns(current)); +} + static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) { - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; + flock->l_pid = flock_translate_pid(fl); #if BITS_PER_LONG == 32 /* * Make sure we can represent the posix lock via @@ -2084,7 +2095,7 @@ static int posix_lock_to_flock(struct flock *flock, struct file_lock *fl) #if BITS_PER_LONG == 32 static void posix_lock_to_flock64(struct flock64 *flock, struct file_lock *fl) { - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; + flock->l_pid = flock_translate_pid(fl); flock->l_start = fl->fl_start; flock->l_len = fl->fl_end == OFFSET_MAX ? 0 : fl->fl_end - fl->fl_start + 1; @@ -2598,7 +2609,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, unsigned int fl_pid; struct pid_namespace *proc_pidns = file_inode(f->file)->i_sb->s_fs_info; - fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); + if (fl->fl_flags & FL_PID_PRIV) + fl_pid = fl->fl_pid; + else + fl_pid = locks_translate_pid(fl->fl_pid, proc_pidns); /* * If there isn't a fl_pid don't display who is waiting on * the lock if we are called from locks_show, or if we are diff --git a/include/linux/fs.h b/include/linux/fs.h index b013fac515f7..179496a9719d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -908,6 +908,7 @@ static inline struct file *get_file(struct file *f) #define FL_UNLOCK_PENDING 512 /* Lease is being broken */ #define FL_OFDLCK 1024 /* lock is "owned" by struct file */ #define FL_LAYOUT 2048 /* outstanding pNFS layout */ +#define FL_PID_PRIV 4096 /* F_GETLK should report fl_pid */ #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
Now that we're translating fl_pid for F_GETLK and /proc/locks, we need to handle the case where a remote filesystem directly sets fl_pid. In that case, the fl_pid should not be translated into a local pid namespace. If the filesystem implements the lock operation, set a flag to return the lock's fl_pid value directly, rather translate it. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/locks.c | 22 ++++++++++++++++++---- include/linux/fs.h | 1 + 2 files changed, 19 insertions(+), 4 deletions(-)