Re: [PATCH v2] locks: change POSIX lock ownership on execve when files_struct is displaced

[ebiederm@xmission.com (Eric W. Biederman)]

Jeff Layton <jlayton@kernel.org> writes:

> From: Jeff Layton <jlayton@redhat.com>
>
> POSIX mandates that open fds and their associated file locks should be
> preserved across an execve. This works, unless the process is
> multithreaded at the time that execve is called.

Would this perhaps work better if we moved unshare_files to after or
inside of de_thread.  That would remove any cases where fd->count is > 1
simply because you are multi-threaded.  It would only leave the strange
cases where files struct is shared between different processes.

> In that case, we'll end up unsharing the files_struct but the locks will
> still have their fl_owner set to the address of the old one. Eventually,
> when the other threads die and the last reference to the old
> files_struct is put, any POSIX locks get torn down since it looks like
> a close occurred on them.
>
> The result is that all of your open files will be intact with none of
> the locks you held before execve. The simple answer to this is "use OFD
> locks", but this is a nasty surprise and it violates the spec.
>
> On a successful execve, change ownership of any POSIX file_locks
> associated with the old files_struct to the new one, if we ended up
> swapping it out.

If we can move unshare_files I believe the need for changing the
ownership would go away.  Which seems like easier to understand
and simpler code in the end.  With fewer surprises.

Eric


> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>


> ---
>  fs/exec.c          |  4 +++-
>  fs/locks.c         | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |  8 ++++++++
>  3 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 7eb8d21bcab9..35b05376bf78 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1812,8 +1812,10 @@ static int do_execveat_common(int fd, struct filename *filename,
>  	free_bprm(bprm);
>  	kfree(pathbuf);
>  	putname(filename);
> -	if (displaced)
> +	if (displaced) {
> +		posix_change_lock_owners(current->files, displaced);
>  		put_files_struct(displaced);
> +	}
>  	return retval;
>  
>  out:
> diff --git a/fs/locks.c b/fs/locks.c
> index d6ff4beb70ce..ab428ca8bb11 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -993,6 +993,66 @@ static int flock_lock_inode(struct inode *inode, struct file_lock *request)
>  	return error;
>  }
>  
> +struct posix_change_lock_owners_arg {
> +	fl_owner_t old;
> +	fl_owner_t new;
> +};
> +
> +static int posix_change_lock_owners_cb(const void *varg, struct file *file,
> +					  unsigned int fd)
> +{
> +	const struct posix_change_lock_owners_arg *arg = varg;
> +	struct inode *inode = file_inode(file);
> +	struct file_lock_context *ctx;
> +	struct file_lock *fl, *tmp;
> +
> +	/* If there is no context, then no locks need to be changed */
> +	ctx = locks_get_lock_context(inode, F_UNLCK);
> +	if (!ctx)
> +		return 0;
> +
> +	percpu_down_read_preempt_disable(&file_rwsem);
> +	spin_lock(&ctx->flc_lock);
> +	/* Find the first lock with the old owner */
> +	list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
> +		if (fl->fl_owner == arg->old)
> +			break;
> +	}
> +
> +	list_for_each_entry_safe_from(fl, tmp, &ctx->flc_posix, fl_list) {
> +		if (fl->fl_owner != arg->old)
> +			break;
> +
> +		/* This should only be used for normal userland lockmanager */
> +		if (fl->fl_lmops) {
> +			WARN_ON_ONCE(1);
> +			break;
> +		}
> +		fl->fl_owner = arg->new;
> +	}
> +	spin_unlock(&ctx->flc_lock);
> +	percpu_up_read_preempt_enable(&file_rwsem);
> +	return 0;
> +}
> +
> +/**
> + * posix_change_lock_owners - change lock owners from old files_struct to new
> + * @files: new files struct to own locks
> + * @old: old files struct that previously held locks
> + *
> + * On execve, a process may end up with a new files_struct. In that case, we
> + * must change all of the locks that were owned by the previous files_struct
> + * to the new one.
> + */
> +void posix_change_lock_owners(struct files_struct *new,
> +			      struct files_struct *old)
> +{
> +	struct posix_change_lock_owners_arg arg = { .old = old,
> +						    .new = new };
> +
> +	iterate_fd(new, 0, posix_change_lock_owners_cb, &arg);
> +}
> +
>  static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>  			    struct file_lock *conflock)
>  {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 79c413985305..65fa99707bf9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1098,6 +1098,8 @@ extern int lease_modify(struct file_lock *, int, struct list_head *);
>  struct files_struct;
>  extern void show_fd_locks(struct seq_file *f,
>  			 struct file *filp, struct files_struct *files);
> +extern void posix_change_lock_owners(struct files_struct *new,
> +				     struct files_struct *old);
>  #else /* !CONFIG_FILE_LOCKING */
>  static inline int fcntl_getlk(struct file *file, unsigned int cmd,
>  			      struct flock __user *user)
> @@ -1232,6 +1234,12 @@ static inline int lease_modify(struct file_lock *fl, int arg,
>  struct files_struct;
>  static inline void show_fd_locks(struct seq_file *f,
>  			struct file *filp, struct files_struct *files) {}
> +
> +static inline void posix_change_lock_owners(struct files_struct *new,
> +					    struct files_struct *old)
> +{
> +}
> +
>  #endif /* !CONFIG_FILE_LOCKING */
>  
>  static inline struct inode *file_inode(const struct file *f)