diff mbox series

[v2] cifs: Fix handling of OFD locks

Message ID 20180802185219.4122-1-paulo@paulo.ac (mailing list archive)
State New, archived
Headers show
Series [v2] cifs: Fix handling of OFD locks | expand

Commit Message

Paulo Alcantara (SUSE) Aug. 2, 2018, 6:52 p.m. UTC
OFD locks never conflict each other.

Signed-off-by: Paulo Alcantara <palcantara@suse.de>
---
 fs/cifs/cifsglob.h  |   1 +
 fs/cifs/cifsproto.h |   4 --
 fs/cifs/file.c      | 109 +++++++++++++++++++++++++++++---------------
 3 files changed, 74 insertions(+), 40 deletions(-)

Comments

Pavel Shilovsky Aug. 2, 2018, 10:17 p.m. UTC | #1
2018-08-02 11:52 GMT-07:00 Paulo Alcantara <paulo@paulo.ac>:
> OFD locks never conflict each other.

I guess it should be mentioned that OFD locks never conflicts if they
are set through the same FD.

>
> Signed-off-by: Paulo Alcantara <palcantara@suse.de>
> ---
>  fs/cifs/cifsglob.h  |   1 +
>  fs/cifs/cifsproto.h |   4 --
>  fs/cifs/file.c      | 109 +++++++++++++++++++++++++++++---------------
>  3 files changed, 74 insertions(+), 40 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0553929e8339..bb55e9aa4fe3 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1069,6 +1069,7 @@ struct cifsLockInfo {
>         __u64 length;
>         __u32 pid;
>         __u32 type;
> +       unsigned int flags; /* file lock flags */
>  };
>
>  /*
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 20adda4de83b..e6a72a3f597b 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -218,10 +218,6 @@ extern void cifs_umount(struct cifs_sb_info *);
>  extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
>  extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
>
> -extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
> -                                   __u64 length, __u8 type,
> -                                   struct cifsLockInfo **conf_lock,
> -                                   int rw_check);
>  extern void cifs_add_pending_open(struct cifs_fid *fid,
>                                   struct tcon_link *tlink,
>                                   struct cifs_pending_open *open);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 8d41ca7bfcf1..58ee94550e42 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -864,14 +864,15 @@ int cifs_closedir(struct inode *inode, struct file *file)
>  }
>
>  static struct cifsLockInfo *
> -cifs_lock_init(__u64 offset, __u64 length, __u8 type)
> +cifs_lock_init(struct file_lock *fl, __u8 type)
>  {
>         struct cifsLockInfo *lock =
>                 kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
>         if (!lock)
>                 return lock;
> -       lock->offset = offset;
> -       lock->length = length;
> +       lock->offset = fl->fl_start;
> +       lock->length = fl->fl_end - fl->fl_start + 1;
> +       lock->flags = fl->fl_flags;
>         lock->type = type;
>         lock->pid = current->tgid;
>         INIT_LIST_HEAD(&lock->blist);
> @@ -893,30 +894,60 @@ cifs_del_lock_waiters(struct cifsLockInfo *lock)
>  #define CIFS_READ_OP   1
>  #define CIFS_WRITE_OP  2
>
> -/* @rw_check : 0 - no op, 1 - read, 2 - write */
> -static bool
> -cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
> -                           __u64 length, __u8 type, struct cifsFileInfo *cfile,
> -                           struct cifsLockInfo **conf_lock, int rw_check)
> +static inline bool locks_overlap(struct cifsLockInfo *li, __u64 offset,
> +                                __u64 length)
> +{
> +       return offset + length > li->offset &&
> +               offset < li->offset + li->length;
> +}
> +
> +static inline bool
> +locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo *cur_cfile,
> +              struct cifsLockInfo *li, __u64 offset, __u64 length,
> +              __u8 type, unsigned int flags, struct TCP_Server_Info *server,
> +              int rw_check)
> +{
> +       if (!locks_overlap(li, offset, length))
> +               return false;
> +       if (!server->ops->compare_fids(cfile, cur_cfile))
> +               return true;

If FIDs don't match locks are considered conflicting which is not true
for shared locks.

> +       if (current->tgid != li->pid)
> +               return true;
> +
> +       if (rw_check == CIFS_LOCK_OP) {
> +               if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK))
> +                       return false;
> +               if ((flags & FL_POSIX) && (li->flags & FL_POSIX))
> +                       return false;

Here you are changing the behavior for POSIX locks that doesn't
reflect the patch title/message.

> +               if (li->type & server->vals->exclusive_lock_type)
> +                       return true;

With the change above locking a file with a shared lock already locked
by an exclusive lock from the same FD and PID won't be allowed which
contradict LockFileEx semantics
(https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex).

> +               if (type & server->vals->exclusive_lock_type)
> +                       return true;
> +               return false;
> +       }
> +
> +       /* shared lock prevents write op through the same fid */
> +       if (!(li->type & server->vals->shared_lock_type) ||
> +           rw_check != CIFS_WRITE_OP)
> +               return false;
> +
> +       return true;
> +}
> +
> +static inline bool __fid_lock_conflicts(struct cifs_fid_locks *fdlocks,
> +                                       __u64 offset, __u64 length,
> +                                       __u8 type, unsigned int flags,
> +                                       struct cifsFileInfo *cfile,
> +                                       struct cifsLockInfo **conf_lock,
> +                                       int rw_check)
>  {
>         struct cifsLockInfo *li;
>         struct cifsFileInfo *cur_cfile = fdlocks->cfile;
>         struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
>
>         list_for_each_entry(li, &fdlocks->locks, llist) {
> -               if (offset + length <= li->offset ||
> -                   offset >= li->offset + li->length)
> -                       continue;
> -               if (rw_check != CIFS_LOCK_OP && current->tgid == li->pid &&
> -                   server->ops->compare_fids(cfile, cur_cfile)) {
> -                       /* shared lock prevents write op through the same fid */
> -                       if (!(li->type & server->vals->shared_lock_type) ||
> -                           rw_check != CIFS_WRITE_OP)
> -                               continue;
> -               }
> -               if ((type & server->vals->shared_lock_type) &&
> -                   ((server->ops->compare_fids(cfile, cur_cfile) &&
> -                    current->tgid == li->pid) || type == li->type))
> +               if (!locks_conflict(cfile, cur_cfile, li, offset, length, type,
> +                                   flags, server, rw_check))

I am remembering that the existing logic is trying to match the SMB
server behavior for SMB locks (see LockFileEx), so we won't have
conflicts when we get a oplock/lease break and push byte-range locks
to the server.

This change will break the existing behavior for both POSIX and OFD
locks: while you allow locks to be set locally, server will reject
them and we will end up in a mixed state. Is it desired behavior?


>                         continue;
>                 if (conf_lock)
>                         *conf_lock = li;
> @@ -925,18 +956,19 @@ cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
>         return false;
>  }
>
> -bool
> -cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
> -                       __u8 type, struct cifsLockInfo **conf_lock,
> -                       int rw_check)
> +static bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
> +                                   __u64 length, __u8 type, unsigned int flags,
> +                                   struct cifsLockInfo **conf_lock,
> +                                   int rw_check)
>  {
>         bool rc = false;
>         struct cifs_fid_locks *cur;
>         struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry));
>
>         list_for_each_entry(cur, &cinode->llist, llist) {
> -               rc = cifs_find_fid_lock_conflict(cur, offset, length, type,
> -                                                cfile, conf_lock, rw_check);
> +               rc = __fid_lock_conflicts(cur, offset, length, type,
> +                                         flags, cfile, conf_lock,
> +                                         rw_check);
>                 if (rc)
>                         break;
>         }
> @@ -964,7 +996,8 @@ cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
>         down_read(&cinode->lock_sem);
>
>         exist = cifs_find_lock_conflict(cfile, offset, length, type,
> -                                       &conf_lock, CIFS_LOCK_OP);
> +                                       flock->fl_flags, &conf_lock,
> +                                       CIFS_LOCK_OP);
>         if (exist) {
>                 flock->fl_start = conf_lock->offset;
>                 flock->fl_end = conf_lock->offset + conf_lock->length - 1;
> @@ -1011,7 +1044,8 @@ cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock,
>         down_write(&cinode->lock_sem);
>
>         exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length,
> -                                       lock->type, &conf_lock, CIFS_LOCK_OP);
> +                                       lock->type, lock->flags, &conf_lock,
> +                                       CIFS_LOCK_OP);
>         if (!exist && cinode->can_cache_brlcks) {
>                 list_add_tail(&lock->llist, &cfile->llist->locks);
>                 up_write(&cinode->lock_sem);
> @@ -1307,8 +1341,15 @@ static void
>  cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
>                 bool *wait_flag, struct TCP_Server_Info *server)
>  {
> +       unsigned int flags = FL_POSIX | FL_OFDLCK | FL_FLOCK | FL_SLEEP |
> +               FL_ACCESS | FL_LEASE | FL_CLOSE;
> +
> +       if (flock->fl_flags & ~flags)
> +               cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
>         if (flock->fl_flags & FL_POSIX)
>                 cifs_dbg(FYI, "Posix\n");
> +       if (flock->fl_flags & FL_OFDLCK)
> +               cifs_dbg(FYI, "OFD lock\n");
>         if (flock->fl_flags & FL_FLOCK)
>                 cifs_dbg(FYI, "Flock\n");
>         if (flock->fl_flags & FL_SLEEP) {
> @@ -1319,10 +1360,6 @@ cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
>                 cifs_dbg(FYI, "Process suspended by mandatory locking - not implemented yet\n");
>         if (flock->fl_flags & FL_LEASE)
>                 cifs_dbg(FYI, "Lease on file - not implemented yet\n");
> -       if (flock->fl_flags &
> -           (~(FL_POSIX | FL_FLOCK | FL_SLEEP |
> -              FL_ACCESS | FL_LEASE | FL_CLOSE)))
> -               cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
>
>         *type = server->vals->large_lock_type;
>         if (flock->fl_type == F_WRLCK) {
> @@ -1584,7 +1621,7 @@ cifs_setlk(struct file *file, struct file_lock *flock, __u32 type,
>         if (lock) {
>                 struct cifsLockInfo *lock;
>
> -               lock = cifs_lock_init(flock->fl_start, length, type);
> +               lock = cifs_lock_init(flock, type);
>                 if (!lock)
>                         return -ENOMEM;
>
> @@ -2817,7 +2854,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
>                 goto out;
>
>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
> -                                    server->vals->exclusive_lock_type, NULL,
> +                                    server->vals->exclusive_lock_type, 0, NULL,
>                                      CIFS_WRITE_OP))
>                 rc = __generic_file_write_iter(iocb, from);
>         else
> @@ -3388,7 +3425,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,
> -                                    NULL, CIFS_READ_OP))
> +                                    0, NULL, CIFS_READ_OP))
>                 rc = generic_file_read_iter(iocb, to);
>         up_read(&cinode->lock_sem);
>         return rc;
> --
> 2.18.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paulo Alcantara Aug. 2, 2018, 11:18 p.m. UTC | #2
Hi Pavel,

Thanks for the review!

On August 2, 2018 7:17:42 PM GMT-03:00, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>2018-08-02 11:52 GMT-07:00 Paulo Alcantara <paulo@paulo.ac>:
>> OFD locks never conflict each other.
>
>I guess it should be mentioned that OFD locks never conflicts if they
>are set through the same FD.

OK.

>
>>
>> Signed-off-by: Paulo Alcantara <palcantara@suse.de>
>> ---
>>  fs/cifs/cifsglob.h  |   1 +
>>  fs/cifs/cifsproto.h |   4 --
>>  fs/cifs/file.c      | 109
>+++++++++++++++++++++++++++++---------------
>>  3 files changed, 74 insertions(+), 40 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 0553929e8339..bb55e9aa4fe3 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -1069,6 +1069,7 @@ struct cifsLockInfo {
>>         __u64 length;
>>         __u32 pid;
>>         __u32 type;
>> +       unsigned int flags; /* file lock flags */
>>  };
>>
>>  /*
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 20adda4de83b..e6a72a3f597b 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -218,10 +218,6 @@ extern void cifs_umount(struct cifs_sb_info *);
>>  extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
>>  extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
>>
>> -extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile,
>__u64 offset,
>> -                                   __u64 length, __u8 type,
>> -                                   struct cifsLockInfo **conf_lock,
>> -                                   int rw_check);
>>  extern void cifs_add_pending_open(struct cifs_fid *fid,
>>                                   struct tcon_link *tlink,
>>                                   struct cifs_pending_open *open);
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 8d41ca7bfcf1..58ee94550e42 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -864,14 +864,15 @@ int cifs_closedir(struct inode *inode, struct
>file *file)
>>  }
>>
>>  static struct cifsLockInfo *
>> -cifs_lock_init(__u64 offset, __u64 length, __u8 type)
>> +cifs_lock_init(struct file_lock *fl, __u8 type)
>>  {
>>         struct cifsLockInfo *lock =
>>                 kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
>>         if (!lock)
>>                 return lock;
>> -       lock->offset = offset;
>> -       lock->length = length;
>> +       lock->offset = fl->fl_start;
>> +       lock->length = fl->fl_end - fl->fl_start + 1;
>> +       lock->flags = fl->fl_flags;
>>         lock->type = type;
>>         lock->pid = current->tgid;
>>         INIT_LIST_HEAD(&lock->blist);
>> @@ -893,30 +894,60 @@ cifs_del_lock_waiters(struct cifsLockInfo
>*lock)
>>  #define CIFS_READ_OP   1
>>  #define CIFS_WRITE_OP  2
>>
>> -/* @rw_check : 0 - no op, 1 - read, 2 - write */
>> -static bool
>> -cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64
>offset,
>> -                           __u64 length, __u8 type, struct
>cifsFileInfo *cfile,
>> -                           struct cifsLockInfo **conf_lock, int
>rw_check)
>> +static inline bool locks_overlap(struct cifsLockInfo *li, __u64
>offset,
>> +                                __u64 length)
>> +{
>> +       return offset + length > li->offset &&
>> +               offset < li->offset + li->length;
>> +}
>> +
>> +static inline bool
>> +locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo
>*cur_cfile,
>> +              struct cifsLockInfo *li, __u64 offset, __u64 length,
>> +              __u8 type, unsigned int flags, struct TCP_Server_Info
>*server,
>> +              int rw_check)
>> +{
>> +       if (!locks_overlap(li, offset, length))
>> +               return false;
>> +       if (!server->ops->compare_fids(cfile, cur_cfile))
>> +               return true;
>
>If FIDs don't match locks are considered conflicting which is not true
>for shared locks.
>
>> +       if (current->tgid != li->pid)
>> +               return true;
>> +
>> +       if (rw_check == CIFS_LOCK_OP) {
>> +               if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK))
>> +                       return false;
>> +               if ((flags & FL_POSIX) && (li->flags & FL_POSIX))
>> +                       return false;
>
>Here you are changing the behavior for POSIX locks that doesn't
>reflect the patch title/message.
>
>> +               if (li->type & server->vals->exclusive_lock_type)
>> +                       return true;
>
>With the change above locking a file with a shared lock already locked
>by an exclusive lock from the same FD and PID won't be allowed which
>contradict LockFileEx semantics
>(https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-lockfileex).

You're right. I will fix it.

>
>> +               if (type & server->vals->exclusive_lock_type)
>> +                       return true;
>> +               return false;
>> +       }
>> +
>> +       /* shared lock prevents write op through the same fid */
>> +       if (!(li->type & server->vals->shared_lock_type) ||
>> +           rw_check != CIFS_WRITE_OP)
>> +               return false;
>> +
>> +       return true;
>> +}
>> +
>> +static inline bool __fid_lock_conflicts(struct cifs_fid_locks
>*fdlocks,
>> +                                       __u64 offset, __u64 length,
>> +                                       __u8 type, unsigned int
>flags,
>> +                                       struct cifsFileInfo *cfile,
>> +                                       struct cifsLockInfo
>**conf_lock,
>> +                                       int rw_check)
>>  {
>>         struct cifsLockInfo *li;
>>         struct cifsFileInfo *cur_cfile = fdlocks->cfile;
>>         struct TCP_Server_Info *server =
>tlink_tcon(cfile->tlink)->ses->server;
>>
>>         list_for_each_entry(li, &fdlocks->locks, llist) {
>> -               if (offset + length <= li->offset ||
>> -                   offset >= li->offset + li->length)
>> -                       continue;
>> -               if (rw_check != CIFS_LOCK_OP && current->tgid ==
>li->pid &&
>> -                   server->ops->compare_fids(cfile, cur_cfile)) {
>> -                       /* shared lock prevents write op through the
>same fid */
>> -                       if (!(li->type &
>server->vals->shared_lock_type) ||
>> -                           rw_check != CIFS_WRITE_OP)
>> -                               continue;
>> -               }
>> -               if ((type & server->vals->shared_lock_type) &&
>> -                   ((server->ops->compare_fids(cfile, cur_cfile) &&
>> -                    current->tgid == li->pid) || type == li->type))
>> +               if (!locks_conflict(cfile, cur_cfile, li, offset,
>length, type,
>> +                                   flags, server, rw_check))
>
>I am remembering that the existing logic is trying to match the SMB
>server behavior for SMB locks (see LockFileEx), so we won't have
>conflicts when we get a oplock/lease break and push byte-range locks
>to the server.
>
>This change will break the existing behavior for both POSIX and OFD
>locks: while you allow locks to be set locally, server will reject
>them and we will end up in a mixed state. Is it desired behavior?

Nope, it isn't. Your comments make sense and the logic is broken. I will fix it in next version.

>
>
>>                         continue;
>>                 if (conf_lock)
>>                         *conf_lock = li;
>> @@ -925,18 +956,19 @@ cifs_find_fid_lock_conflict(struct
>cifs_fid_locks *fdlocks, __u64 offset,
>>         return false;
>>  }
>>
>> -bool
>> -cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
>__u64 length,
>> -                       __u8 type, struct cifsLockInfo **conf_lock,
>> -                       int rw_check)
>> +static bool cifs_find_lock_conflict(struct cifsFileInfo *cfile,
>__u64 offset,
>> +                                   __u64 length, __u8 type, unsigned
>int flags,
>> +                                   struct cifsLockInfo **conf_lock,
>> +                                   int rw_check)
>>  {
>>         bool rc = false;
>>         struct cifs_fid_locks *cur;
>>         struct cifsInodeInfo *cinode =
>CIFS_I(d_inode(cfile->dentry));
>>
>>         list_for_each_entry(cur, &cinode->llist, llist) {
>> -               rc = cifs_find_fid_lock_conflict(cur, offset, length,
>type,
>> -                                                cfile, conf_lock,
>rw_check);
>> +               rc = __fid_lock_conflicts(cur, offset, length, type,
>> +                                         flags, cfile, conf_lock,
>> +                                         rw_check);
>>                 if (rc)
>>                         break;
>>         }
>> @@ -964,7 +996,8 @@ cifs_lock_test(struct cifsFileInfo *cfile, __u64
>offset, __u64 length,
>>         down_read(&cinode->lock_sem);
>>
>>         exist = cifs_find_lock_conflict(cfile, offset, length, type,
>> -                                       &conf_lock, CIFS_LOCK_OP);
>> +                                       flock->fl_flags, &conf_lock,
>> +                                       CIFS_LOCK_OP);
>>         if (exist) {
>>                 flock->fl_start = conf_lock->offset;
>>                 flock->fl_end = conf_lock->offset + conf_lock->length
>- 1;
>> @@ -1011,7 +1044,8 @@ cifs_lock_add_if(struct cifsFileInfo *cfile,
>struct cifsLockInfo *lock,
>>         down_write(&cinode->lock_sem);
>>
>>         exist = cifs_find_lock_conflict(cfile, lock->offset,
>lock->length,
>> -                                       lock->type, &conf_lock,
>CIFS_LOCK_OP);
>> +                                       lock->type, lock->flags,
>&conf_lock,
>> +                                       CIFS_LOCK_OP);
>>         if (!exist && cinode->can_cache_brlcks) {
>>                 list_add_tail(&lock->llist, &cfile->llist->locks);
>>                 up_write(&cinode->lock_sem);
>> @@ -1307,8 +1341,15 @@ static void
>>  cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int
>*unlock,
>>                 bool *wait_flag, struct TCP_Server_Info *server)
>>  {
>> +       unsigned int flags = FL_POSIX | FL_OFDLCK | FL_FLOCK |
>FL_SLEEP |
>> +               FL_ACCESS | FL_LEASE | FL_CLOSE;
>> +
>> +       if (flock->fl_flags & ~flags)
>> +               cifs_dbg(FYI, "Unknown lock flags 0x%x\n",
>flock->fl_flags);
>>         if (flock->fl_flags & FL_POSIX)
>>                 cifs_dbg(FYI, "Posix\n");
>> +       if (flock->fl_flags & FL_OFDLCK)
>> +               cifs_dbg(FYI, "OFD lock\n");
>>         if (flock->fl_flags & FL_FLOCK)
>>                 cifs_dbg(FYI, "Flock\n");
>>         if (flock->fl_flags & FL_SLEEP) {
>> @@ -1319,10 +1360,6 @@ cifs_read_flock(struct file_lock *flock, __u32
>*type, int *lock, int *unlock,
>>                 cifs_dbg(FYI, "Process suspended by mandatory locking
>- not implemented yet\n");
>>         if (flock->fl_flags & FL_LEASE)
>>                 cifs_dbg(FYI, "Lease on file - not implemented
>yet\n");
>> -       if (flock->fl_flags &
>> -           (~(FL_POSIX | FL_FLOCK | FL_SLEEP |
>> -              FL_ACCESS | FL_LEASE | FL_CLOSE)))
>> -               cifs_dbg(FYI, "Unknown lock flags 0x%x\n",
>flock->fl_flags);
>>
>>         *type = server->vals->large_lock_type;
>>         if (flock->fl_type == F_WRLCK) {
>> @@ -1584,7 +1621,7 @@ cifs_setlk(struct file *file, struct file_lock
>*flock, __u32 type,
>>         if (lock) {
>>                 struct cifsLockInfo *lock;
>>
>> -               lock = cifs_lock_init(flock->fl_start, length, type);
>> +               lock = cifs_lock_init(flock, type);
>>                 if (!lock)
>>                         return -ENOMEM;
>>
>> @@ -2817,7 +2854,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter
>*from)
>>                 goto out;
>>
>>         if (!cifs_find_lock_conflict(cfile, iocb->ki_pos,
>iov_iter_count(from),
>> -                                   
>server->vals->exclusive_lock_type, NULL,
>> +                                   
>server->vals->exclusive_lock_type, 0, NULL,
>>                                      CIFS_WRITE_OP))
>>                 rc = __generic_file_write_iter(iocb, from);
>>         else
>> @@ -3388,7 +3425,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,
>> -                                    NULL, CIFS_READ_OP))
>> +                                    0, NULL, CIFS_READ_OP))
>>                 rc = generic_file_read_iter(iocb, to);
>>         up_read(&cinode->lock_sem);
>>         return rc;
>> --
>> 2.18.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs"
>in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>Best regards,
>Pavel Shilovsky
diff mbox series

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 0553929e8339..bb55e9aa4fe3 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1069,6 +1069,7 @@  struct cifsLockInfo {
 	__u64 length;
 	__u32 pid;
 	__u32 type;
+	unsigned int flags; /* file lock flags */
 };
 
 /*
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 20adda4de83b..e6a72a3f597b 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -218,10 +218,6 @@  extern void cifs_umount(struct cifs_sb_info *);
 extern void cifs_mark_open_files_invalid(struct cifs_tcon *tcon);
 extern void cifs_reopen_persistent_handles(struct cifs_tcon *tcon);
 
-extern bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
-				    __u64 length, __u8 type,
-				    struct cifsLockInfo **conf_lock,
-				    int rw_check);
 extern void cifs_add_pending_open(struct cifs_fid *fid,
 				  struct tcon_link *tlink,
 				  struct cifs_pending_open *open);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8d41ca7bfcf1..58ee94550e42 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -864,14 +864,15 @@  int cifs_closedir(struct inode *inode, struct file *file)
 }
 
 static struct cifsLockInfo *
-cifs_lock_init(__u64 offset, __u64 length, __u8 type)
+cifs_lock_init(struct file_lock *fl, __u8 type)
 {
 	struct cifsLockInfo *lock =
 		kmalloc(sizeof(struct cifsLockInfo), GFP_KERNEL);
 	if (!lock)
 		return lock;
-	lock->offset = offset;
-	lock->length = length;
+	lock->offset = fl->fl_start;
+	lock->length = fl->fl_end - fl->fl_start + 1;
+	lock->flags = fl->fl_flags;
 	lock->type = type;
 	lock->pid = current->tgid;
 	INIT_LIST_HEAD(&lock->blist);
@@ -893,30 +894,60 @@  cifs_del_lock_waiters(struct cifsLockInfo *lock)
 #define CIFS_READ_OP	1
 #define CIFS_WRITE_OP	2
 
-/* @rw_check : 0 - no op, 1 - read, 2 - write */
-static bool
-cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
-			    __u64 length, __u8 type, struct cifsFileInfo *cfile,
-			    struct cifsLockInfo **conf_lock, int rw_check)
+static inline bool locks_overlap(struct cifsLockInfo *li, __u64 offset,
+				 __u64 length)
+{
+	return offset + length > li->offset &&
+		offset < li->offset + li->length;
+}
+
+static inline bool
+locks_conflict(struct cifsFileInfo *cfile, struct cifsFileInfo *cur_cfile,
+	       struct cifsLockInfo *li, __u64 offset, __u64 length,
+	       __u8 type, unsigned int flags, struct TCP_Server_Info *server,
+	       int rw_check)
+{
+	if (!locks_overlap(li, offset, length))
+		return false;
+	if (!server->ops->compare_fids(cfile, cur_cfile))
+		return true;
+	if (current->tgid != li->pid)
+		return true;
+
+	if (rw_check == CIFS_LOCK_OP) {
+		if ((flags & FL_OFDLCK) && (li->flags & FL_OFDLCK))
+			return false;
+		if ((flags & FL_POSIX) && (li->flags & FL_POSIX))
+			return false;
+		if (li->type & server->vals->exclusive_lock_type)
+			return true;
+		if (type & server->vals->exclusive_lock_type)
+			return true;
+		return false;
+	}
+
+	/* shared lock prevents write op through the same fid */
+	if (!(li->type & server->vals->shared_lock_type) ||
+	    rw_check != CIFS_WRITE_OP)
+		return false;
+
+	return true;
+}
+
+static inline bool __fid_lock_conflicts(struct cifs_fid_locks *fdlocks,
+					__u64 offset, __u64 length,
+					__u8 type, unsigned int flags,
+					struct cifsFileInfo *cfile,
+					struct cifsLockInfo **conf_lock,
+					int rw_check)
 {
 	struct cifsLockInfo *li;
 	struct cifsFileInfo *cur_cfile = fdlocks->cfile;
 	struct TCP_Server_Info *server = tlink_tcon(cfile->tlink)->ses->server;
 
 	list_for_each_entry(li, &fdlocks->locks, llist) {
-		if (offset + length <= li->offset ||
-		    offset >= li->offset + li->length)
-			continue;
-		if (rw_check != CIFS_LOCK_OP && current->tgid == li->pid &&
-		    server->ops->compare_fids(cfile, cur_cfile)) {
-			/* shared lock prevents write op through the same fid */
-			if (!(li->type & server->vals->shared_lock_type) ||
-			    rw_check != CIFS_WRITE_OP)
-				continue;
-		}
-		if ((type & server->vals->shared_lock_type) &&
-		    ((server->ops->compare_fids(cfile, cur_cfile) &&
-		     current->tgid == li->pid) || type == li->type))
+		if (!locks_conflict(cfile, cur_cfile, li, offset, length, type,
+				    flags, server, rw_check))
 			continue;
 		if (conf_lock)
 			*conf_lock = li;
@@ -925,18 +956,19 @@  cifs_find_fid_lock_conflict(struct cifs_fid_locks *fdlocks, __u64 offset,
 	return false;
 }
 
-bool
-cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
-			__u8 type, struct cifsLockInfo **conf_lock,
-			int rw_check)
+static bool cifs_find_lock_conflict(struct cifsFileInfo *cfile, __u64 offset,
+				    __u64 length, __u8 type, unsigned int flags,
+				    struct cifsLockInfo **conf_lock,
+				    int rw_check)
 {
 	bool rc = false;
 	struct cifs_fid_locks *cur;
 	struct cifsInodeInfo *cinode = CIFS_I(d_inode(cfile->dentry));
 
 	list_for_each_entry(cur, &cinode->llist, llist) {
-		rc = cifs_find_fid_lock_conflict(cur, offset, length, type,
-						 cfile, conf_lock, rw_check);
+		rc = __fid_lock_conflicts(cur, offset, length, type,
+					  flags, cfile, conf_lock,
+					  rw_check);
 		if (rc)
 			break;
 	}
@@ -964,7 +996,8 @@  cifs_lock_test(struct cifsFileInfo *cfile, __u64 offset, __u64 length,
 	down_read(&cinode->lock_sem);
 
 	exist = cifs_find_lock_conflict(cfile, offset, length, type,
-					&conf_lock, CIFS_LOCK_OP);
+					flock->fl_flags, &conf_lock,
+					CIFS_LOCK_OP);
 	if (exist) {
 		flock->fl_start = conf_lock->offset;
 		flock->fl_end = conf_lock->offset + conf_lock->length - 1;
@@ -1011,7 +1044,8 @@  cifs_lock_add_if(struct cifsFileInfo *cfile, struct cifsLockInfo *lock,
 	down_write(&cinode->lock_sem);
 
 	exist = cifs_find_lock_conflict(cfile, lock->offset, lock->length,
-					lock->type, &conf_lock, CIFS_LOCK_OP);
+					lock->type, lock->flags, &conf_lock,
+					CIFS_LOCK_OP);
 	if (!exist && cinode->can_cache_brlcks) {
 		list_add_tail(&lock->llist, &cfile->llist->locks);
 		up_write(&cinode->lock_sem);
@@ -1307,8 +1341,15 @@  static void
 cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
 		bool *wait_flag, struct TCP_Server_Info *server)
 {
+	unsigned int flags = FL_POSIX | FL_OFDLCK | FL_FLOCK | FL_SLEEP |
+		FL_ACCESS | FL_LEASE | FL_CLOSE;
+
+	if (flock->fl_flags & ~flags)
+		cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
 	if (flock->fl_flags & FL_POSIX)
 		cifs_dbg(FYI, "Posix\n");
+	if (flock->fl_flags & FL_OFDLCK)
+		cifs_dbg(FYI, "OFD lock\n");
 	if (flock->fl_flags & FL_FLOCK)
 		cifs_dbg(FYI, "Flock\n");
 	if (flock->fl_flags & FL_SLEEP) {
@@ -1319,10 +1360,6 @@  cifs_read_flock(struct file_lock *flock, __u32 *type, int *lock, int *unlock,
 		cifs_dbg(FYI, "Process suspended by mandatory locking - not implemented yet\n");
 	if (flock->fl_flags & FL_LEASE)
 		cifs_dbg(FYI, "Lease on file - not implemented yet\n");
-	if (flock->fl_flags &
-	    (~(FL_POSIX | FL_FLOCK | FL_SLEEP |
-	       FL_ACCESS | FL_LEASE | FL_CLOSE)))
-		cifs_dbg(FYI, "Unknown lock flags 0x%x\n", flock->fl_flags);
 
 	*type = server->vals->large_lock_type;
 	if (flock->fl_type == F_WRLCK) {
@@ -1584,7 +1621,7 @@  cifs_setlk(struct file *file, struct file_lock *flock, __u32 type,
 	if (lock) {
 		struct cifsLockInfo *lock;
 
-		lock = cifs_lock_init(flock->fl_start, length, type);
+		lock = cifs_lock_init(flock, type);
 		if (!lock)
 			return -ENOMEM;
 
@@ -2817,7 +2854,7 @@  cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 
 	if (!cifs_find_lock_conflict(cfile, iocb->ki_pos, iov_iter_count(from),
-				     server->vals->exclusive_lock_type, NULL,
+				     server->vals->exclusive_lock_type, 0, NULL,
 				     CIFS_WRITE_OP))
 		rc = __generic_file_write_iter(iocb, from);
 	else
@@ -3388,7 +3425,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,
-				     NULL, CIFS_READ_OP))
+				     0, NULL, CIFS_READ_OP))
 		rc = generic_file_read_iter(iocb, to);
 	up_read(&cinode->lock_sem);
 	return rc;