[PATCH] cifs: ignore FL_FLOCK locks in read/write

[PATCH] cifs: ignore FL_FLOCK locks in read/write

[Aurélien Aptel]

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

Re: [PATCH] cifs: ignore FL_FLOCK locks in read/write

[Steve French]

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
>


-- 
Thanks,

Steve

Re: [PATCH] cifs: ignore FL_FLOCK locks in read/write

[Pavel Shilovsky]

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
>

Re: [PATCH] cifs: ignore FL_FLOCK locks in read/write

[Aurélien Aptel]

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)

Re: [PATCH] cifs: ignore FL_FLOCK locks in read/write

[Pavel Shilovsky]

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)
>