diff mbox series

cifs: ignore FL_FLOCK locks in read/write

Message ID 20210223182726.31763-1-aaptel@suse.com (mailing list archive)
State New, archived
Headers show
Series cifs: ignore FL_FLOCK locks in read/write | expand

Commit Message

Aurélien Aptel Feb. 23, 2021, 6:27 p.m. UTC 
From: Aurelien Aptel <aaptel@suse.com>

flock(2)-type locks are advisory, they are not supposed to prevent IO
if mode would otherwise allow it. From man page:

   flock()  places  advisory  locks  only; given suitable permissions on a
   file, a process is free to ignore the use of flock() and perform I/O on
   the file.

Simple reproducer:

	#include <stdlib.h>
	#include <stdio.h>
	#include <errno.h>
	#include <sys/file.h>
	#include <sys/types.h>
	#include <sys/wait.h>
	#include <unistd.h>

	int main(int argc, char** argv)
	{
		const char* fn = argv[1] ? argv[1] : "aaa";
		int fd, status, rc;
		pid_t pid;

		fd = open(fn, O_RDWR|O_CREAT, S_IRWXU);
		pid = fork();

		if (pid == 0) {
			flock(fd, LOCK_SH);
			exit(0);
		}

		waitpid(pid, &status, 0);
		rc = write(fd, "xxx\n", 4);
		if (rc < 0) {
			perror("write");
			return 1;
		}

		puts("ok");
		return 0;
	}

If the locks are advisory the write() call is supposed to work
otherwise we are trying to write with only a read lock (aka shared
lock) so it fails.

Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/file.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Steve French Feb. 23, 2021, 6:39 p.m. UTC | #1 
Would be great if we had some simple reproducer like this in xfstests.

On Tue, Feb 23, 2021 at 12:27 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> From: Aurelien Aptel <aaptel@suse.com>
>
> flock(2)-type locks are advisory, they are not supposed to prevent IO
> if mode would otherwise allow it. From man page:
>
>    flock()  places  advisory  locks  only; given suitable permissions on a
>    file, a process is free to ignore the use of flock() and perform I/O on
>    the file.
>
> Simple reproducer:
>
>         #include <stdlib.h>
>         #include <stdio.h>
>         #include <errno.h>
>         #include <sys/file.h>
>         #include <sys/types.h>
>         #include <sys/wait.h>
>         #include <unistd.h>
>
>         int main(int argc, char** argv)
>         {
>                 const char* fn = argv[1] ? argv[1] : "aaa";
>                 int fd, status, rc;
>                 pid_t pid;
>
>                 fd = open(fn, O_RDWR|O_CREAT, S_IRWXU);
>                 pid = fork();
>
>                 if (pid == 0) {
>                         flock(fd, LOCK_SH);
>                         exit(0);
>                 }
>
>                 waitpid(pid, &status, 0);
>                 rc = write(fd, "xxx\n", 4);
>                 if (rc < 0) {
>                         perror("write");
>                         return 1;
>                 }
>
>                 puts("ok");
>                 return 0;
>         }
>
> If the locks are advisory the write() call is supposed to work
> otherwise we are trying to write with only a read lock (aka shared
> lock) so it fails.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/file.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6d001905c8e5..3e351a534720 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -3242,6 +3242,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>         struct inode *inode = file->f_mapping->host;
>         struct cifsInodeInfo *cinode = CIFS_I(inode);
>         struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
> +       struct cifsLockInfo *lock;
>         ssize_t rc;
>
>         inode_lock(inode);
> @@ -3257,7 +3258,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>
>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
>                                      server->vals->exclusive_lock_type, 0,
> -                                    NULL, CIFS_WRITE_OP))
> +                                    &lock, CIFS_WRITE_OP) || (lock->flags & FL_FLOCK))
>                 rc = __generic_file_write_iter(iocb, from);
>         else
>                 rc = -EACCES;
> @@ -3975,6 +3976,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
>         struct cifsFileInfo *cfile = (struct cifsFileInfo *)
>                                                 iocb->ki_filp->private_data;
>         struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> +       struct cifsLockInfo *lock;
>         int rc = -EACCES;
>
>         /*
> @@ -4000,7 +4002,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
>         down_read(&cinode->lock_sem);
>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
>                                      tcon->ses->server->vals->shared_lock_type,
> -                                    0, NULL, CIFS_READ_OP))
> +                                    0, &lock, CIFS_READ_OP) || (lock->flags & FL_FLOCK))
>                 rc = generic_file_read_iter(iocb, to);
>         up_read(&cinode->lock_sem);
>         return rc;
> --
> 2.30.0
>
Pavel Shilovsky Feb. 23, 2021, 6:58 p.m. UTC | #2 
If a flock is emulated on the server side with mandatory locks (which
is what we only have for SMB2 without POSIX extensions) then we should
maintain the same logic on the client. Otherwise you get different
behavior depending on the caching policies currently in effect on the
client side. You may consider testing with both modes when
leases/oplocks are on and off.

--
Best regards,
Pavel Shilovsky

вт, 23 февр. 2021 г. в 10:30, Aurélien Aptel <aaptel@suse.com>:

>
> From: Aurelien Aptel <aaptel@suse.com>
>
> flock(2)-type locks are advisory, they are not supposed to prevent IO
> if mode would otherwise allow it. From man page:
>
>    flock()  places  advisory  locks  only; given suitable permissions on a
>    file, a process is free to ignore the use of flock() and perform I/O on
>    the file.
>
> Simple reproducer:
>
>         #include <stdlib.h>
>         #include <stdio.h>
>         #include <errno.h>
>         #include <sys/file.h>
>         #include <sys/types.h>
>         #include <sys/wait.h>
>         #include <unistd.h>
>
>         int main(int argc, char** argv)
>         {
>                 const char* fn = argv[1] ? argv[1] : "aaa";
>                 int fd, status, rc;
>                 pid_t pid;
>
>                 fd = open(fn, O_RDWR|O_CREAT, S_IRWXU);
>                 pid = fork();
>
>                 if (pid == 0) {
>                         flock(fd, LOCK_SH);
>                         exit(0);
>                 }
>
>                 waitpid(pid, &status, 0);
>                 rc = write(fd, "xxx\n", 4);
>                 if (rc < 0) {
>                         perror("write");
>                         return 1;
>                 }
>
>                 puts("ok");
>                 return 0;
>         }
>
> If the locks are advisory the write() call is supposed to work
> otherwise we are trying to write with only a read lock (aka shared
> lock) so it fails.
>
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>  fs/cifs/file.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6d001905c8e5..3e351a534720 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -3242,6 +3242,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>         struct inode *inode = file->f_mapping->host;
>         struct cifsInodeInfo *cinode = CIFS_I(inode);
>         struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
> +       struct cifsLockInfo *lock;
>         ssize_t rc;
>
>         inode_lock(inode);
> @@ -3257,7 +3258,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>
>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
>                                      server->vals->exclusive_lock_type, 0,
> -                                    NULL, CIFS_WRITE_OP))
> +                                    &lock, CIFS_WRITE_OP) || (lock->flags & FL_FLOCK))
>                 rc = __generic_file_write_iter(iocb, from);
>         else
>                 rc = -EACCES;
> @@ -3975,6 +3976,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
>         struct cifsFileInfo *cfile = (struct cifsFileInfo *)
>                                                 iocb->ki_filp->private_data;
>         struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
> +       struct cifsLockInfo *lock;
>         int rc = -EACCES;
>
>         /*
> @@ -4000,7 +4002,7 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
>         down_read(&cinode->lock_sem);
>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
>                                      tcon->ses->server->vals->shared_lock_type,
> -                                    0, NULL, CIFS_READ_OP))
> +                                    0, &lock, CIFS_READ_OP) || (lock->flags & FL_FLOCK))
>                 rc = generic_file_read_iter(iocb, to);
>         up_read(&cinode->lock_sem);
>         return rc;
> --
> 2.30.0
>
Aurélien Aptel Feb. 24, 2021, 11:11 a.m. UTC | #3 
Pavel Shilovsky <piastryyy@gmail.com> writes:
> If a flock is emulated on the server side with mandatory locks (which
> is what we only have for SMB2 without POSIX extensions) then we should
> maintain the same logic on the client. Otherwise you get different
> behavior depending on the caching policies currently in effect on the
> client side. You may consider testing with both modes when
> leases/oplocks are on and off.

Hm.. you're right, the write will fail on the server side without
cache.

I guess we should document current cifs behaviour in the flock man page.

Cheers,
Pavel Shilovsky Feb. 25, 2021, 12:16 a.m. UTC | #4 
Agree - given the differences between semantics with and without POSIX
extensions we should document limitations that the CIFS client has in
respect to locking including flock, posix locks and ofd locks.
--
Best regards,
Pavel Shilovsky

ср, 24 февр. 2021 г. в 03:11, Aurélien Aptel <aaptel@suse.com>:
>
> Pavel Shilovsky <piastryyy@gmail.com> writes:
> > If a flock is emulated on the server side with mandatory locks (which
> > is what we only have for SMB2 without POSIX extensions) then we should
> > maintain the same logic on the client. Otherwise you get different
> > behavior depending on the caching policies currently in effect on the
> > client side. You may consider testing with both modes when
> > leases/oplocks are on and off.
>
> Hm.. you're right, the write will fail on the server side without
> cache.
>
> I guess we should document current cifs behaviour in the flock man page.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
diff mbox series

Patch

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6d001905c8e5..3e351a534720 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -3242,6 +3242,7 @@  cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 	struct inode *inode = file->f_mapping->host;
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
 	struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
+	struct cifsLockInfo *lock;
 	ssize_t rc;
 
 	inode_lock(inode);
@@ -3257,7 +3258,7 @@  cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
 				     server->vals->exclusive_lock_type, 0,
-				     NULL, CIFS_WRITE_OP))
+				     &lock, CIFS_WRITE_OP) || (lock->flags & FL_FLOCK))
 		rc = __generic_file_write_iter(iocb, from);
 	else
 		rc = -EACCES;
@@ -3975,6 +3976,7 @@  cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
 	struct cifsFileInfo *cfile = (struct cifsFileInfo *)
 						iocb->ki_filp->private_data;
 	struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
+	struct cifsLockInfo *lock;
 	int rc = -EACCES;
 
 	/*
@@ -4000,7 +4002,7 @@  cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
 	down_read(&cinode->lock_sem);
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(to),
 				     tcon->ses->server->vals->shared_lock_type,
-				     0, NULL, CIFS_READ_OP))
+				     0, &lock, CIFS_READ_OP) || (lock->flags & FL_FLOCK))
 		rc = generic_file_read_iter(iocb, to);
 	up_read(&cinode->lock_sem);
 	return rc;