Message-ID: <20140902184456.GC31793@fieldses.org>
Date:	Tue, 2 Sep 2014 14:44:56 -0400
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Pavel Emelyanov <xemul@...allels.com>
Cc:	Jeff Layton <jlayton@...chiereds.net>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-api@...r.kernel.org
Subject: Re: [PATCH] locks: Ability to test for flock presence on fd

On Tue, Sep 02, 2014 at 09:17:34PM +0400, Pavel Emelyanov wrote:
> Hi,
> 
> There's a problem with getting information about who has a flock on
> a specific file. The thing is that the "owner" field, that is shown in
> /proc/locks is the pid of the task who created the flock, not the one
> who _may_ hold it.
> 
> If the flock creator shared the file with some other task (by forking
> or via scm_rights) and then died or closed the file, the information
> shown in proc no longer corresponds to the reality.
> 
> This is critical for CRIU project, that tries to dump (and restore)
> the state of running tasks. For example, let's take two tasks A and B
> both opened a file "/foo", one of tasks places a LOCK_SH lock on the 
> file and then "obfuscated" the owner field in /proc/locks. After this
> we have no ways to find out who is the lock holder.
> 
> I'd like to note, that for LOCK_EX this problem is not critical -- we
> may go to both tasks and "ask" them to LOCK_EX the file again (we can
> do it in CRIU, I can tell more if required). The one who succeeds is 
> the lock holder.

It could be both, actually, right?

> With LOCK_SH this doesn't work. Trying to drop the
> lock doesn't work either, as flock(LOCK_UN) reports 0 in both cases:
> if the file is locked and if it is not.
> 
> That said, I'd like to propose the LOCK_TEST flag to the flock call,
> that would check whether the lock of the given type (LOCK_SH or LOCK_EX)
> exists on the file we test. It's not the same as the existing in-kernel
> FL_ACCESS flag, which checks whether the new lock is possible, but
> it's a new FL_TEST flag, that checks whether the existing lock is there.
> 
> What do you think?

I guess I can't see anything really wrong with it.

It ignores the (poorly documented) LOCK_MAND case, but maybe that's OK.

Would it make sense to return the lock type held instead, so you could
do one flock(fd, LOCK_TEST) instead of flock(fd, LOCK_TEST|LOCK_SH) and
flock(fd, LOCK_TEST|LOCK_EX) ?

It'd be nice if we could fix /proc/locks.  (You'd think I'd know better,
but I've certainly been confused before when /proc/locks told me a lock
was owned by a nonexistant PID.)  But as long as multiple PIDs can "own"
a flock and as long as there's no simple ID we can use to refer to a
given struct file, I don't see an easy solution.

--b.


> 
> Signed-off-by: Pavel Emelyanov <xemul@...allels.com>
> 
> ---
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index bb08857..50842bf 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -830,7 +830,7 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
>  	int found = 0;
>  	LIST_HEAD(dispose);
>  
> -	if (!(request->fl_flags & FL_ACCESS) && (request->fl_type != F_UNLCK)) {
> +	if (!(request->fl_flags & (FL_ACCESS|FL_TEST)) && (request->fl_type != F_UNLCK)) {
>  		new_fl = locks_alloc_lock();
>  		if (!new_fl)
>  			return -ENOMEM;
> @@ -850,11 +850,18 @@ static int flock_lock_file(struct file *filp, struct file_lock *request)
>  			continue;
>  		if (request->fl_type == fl->fl_type)
>  			goto out;
> +		if (request->fl_flags & FL_TEST)
> +			break;
>  		found = 1;
>  		locks_delete_lock(before, &dispose);
>  		break;
>  	}
>  
> +	if (request->fl_flags & FL_TEST) {
> +		error = -ENOENT;
> +		goto out;
> +	}
> +
>  	if (request->fl_type == F_UNLCK) {
>  		if ((request->fl_flags & FL_EXISTS) && !found)
>  			error = -ENOENT;
> @@ -1852,15 +1859,16 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  {
>  	struct fd f = fdget(fd);
>  	struct file_lock *lock;
> -	int can_sleep, unlock;
> +	int can_sleep, unlock, test;
>  	int error;
>  
>  	error = -EBADF;
>  	if (!f.file)
>  		goto out;
>  
> +	test = (cmd & LOCK_TEST);
>  	can_sleep = !(cmd & LOCK_NB);
> -	cmd &= ~LOCK_NB;
> +	cmd &= ~(LOCK_NB|LOCK_TEST);
>  	unlock = (cmd == LOCK_UN);
>  
>  	if (!unlock && !(cmd & LOCK_MAND) &&
> @@ -1872,6 +1880,8 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
>  		goto out_putf;
>  	if (can_sleep)
>  		lock->fl_flags |= FL_SLEEP;
> +	if (test)
> +		lock->fl_flags |= FL_TEST;
>  
>  	error = security_file_lock(f.file, lock->fl_type);
>  	if (error)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9418772..9230e1d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -844,6 +844,7 @@ static inline struct file *get_file(struct file *f)
>  #define FL_DOWNGRADE_PENDING	256 /* Lease is being downgraded */
>  #define FL_UNLOCK_PENDING	512 /* Lease is being broken */
>  #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
> +#define FL_TEST		2048
>  
>  /*
>   * Special return value from posix_lock_file() and vfs_lock_file() for
> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> index 7543b3e..7302e36 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -184,6 +184,7 @@ struct f_owner_ex {
>  #define LOCK_READ	64	/* which allows concurrent read operations */
>  #define LOCK_WRITE	128	/* which allows concurrent write operations */
>  #define LOCK_RW		192	/* which allows concurrent read & write ops */
> +#define LOCK_TEST	256	/* check for my SH|EX locks present */
>  
>  #define F_LINUX_SPECIFIC_BASE	1024
>  
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists