linux-kernel - Re: [PATCH] ksmbd: use F_SETLK to force vfs_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKYAXd97orfDc7VA+fae7tLjTufMksQTepYKazY=gX7UkG0+rw@mail.gmail.com>
Date:   Wed, 22 Dec 2021 14:25:16 +0900
From:   Namjae Jeon <linkinjeon@...nel.org>
To:     Vasily Averin <vvs@...tuozzo.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steve French <sfrench@...ba.org>,
        Hyunchul Lee <hyc.lee@...il.com>, kernel@...nvz.org,
        linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ksmbd: use F_SETLK to force vfs_file_lock() to return asynchronously

2021-12-22 13:32 GMT+09:00, Vasily Averin <vvs@...tuozzo.com>:
> On 22.12.2021 05:50, Namjae Jeon wrote:
>> 2021-12-21 22:08 GMT+09:00, Vasily Averin <vvs@...tuozzo.com>:
>>> On 21.12.2021 15:02, Namjae Jeon wrote:
>>>> 2021-12-19 18:34 GMT+09:00, Vasily Averin <vvs@...tuozzo.com>:
>>>>> To avoid possible deadlock ksmbd should process locks asynchronously.
>>>>> Callers expecting vfs_file_locks() to return asynchronously should
>>>>> only
>>>>> use F_SETLK, not F_SETLKW.
>>>> Should I check this patch instead of
>>>> [PATCH] ksmbd: force "fail immediately" flag on fs with its own ->lock
>>>> ?
>>>
>>> no, these patches are independent and both ones are required.
>>> current patch fixes incorrect kernel thread behaviour:
>>> kernel threads should not use F_SETLKW for locking requests.
>> How does this patch work? posix_lock_file in vfs_lock_file() does not use
>> cmd.
>> And your patch still leaves FL_SLEEP.
>
> "use F_SETLK, not F_SETLKW" was copy-pasted from requirement described in
> comment above vfs_lock_file().
>
> posix_lock_file() is not used in all ->lock() functions, and use F_SETLKW
> forces some of affected filesystem use blocking locks:
What I'm saying is that when we apply "ksmbd: force "fail immediately"
flag on fs with its own ->lock ", this patch is meaningless. How is
->lock() with F_SETLKW called?

>
> fs/ceph/locks.c::ceph_lock()
>         /* set wait bit as appropriate, then make command as Ceph expects
> it*/
>         if (IS_GETLK(cmd))
>                 op = CEPH_MDS_OP_GETFILELOCK;
>         else if (IS_SETLKW(cmd))
>                 wait = 1
>
> nfs v3 handles it in nlmclnt_proc
> fs/lockd/clntproc.c::nlmclnt_proc
>         if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
>                 if (fl->fl_type != F_UNLCK) {
>                         call->a_args.block = IS_SETLKW(cmd) ? 1 : 0;
>
>
> nvs v4 handles it in nfs4_retry_setlk()
> fs/nfs/nfs4proc.c()::nfs4_retry_setlk()
>         while(!signalled()) {
>                 status = nfs4_proc_setlk(state, cmd, request);
>                 if ((status != -EAGAIN) || IS_SETLK(cmd))
>                         break;
>
> gfs2_lock and ocfs calls dlm_posix_lock()
> dlm_posix_lock::dlm_posix_lock()
> op->info.wait           = IS_SETLKW(cmd)
>
> So if ksmbd will try to export these file systems it can be deadlocked on
> blocking locks processing,
> even with my patch dropped FL_SLEEP.
>
> To be honest, some other filesystems, contrary, ignores cmd and handles
> FL_SLEEP instead:
> cifs_lock and fuse_setlk.  Moreover, locks_lock_file_wait() is widely used
> too,
> (and can block if FL_SLEEP is set). Some of these cases looks like bugs,
> its careful processing requires some time, therefore right now, to quickly
> work around
> all these cases kernel export threads  (nfsd/lockd/ksmbd) can drop to
> FL_SLEEP flag).
>
> I think it makes sense to create new bug on bugzilla.kernel.org, explain all
> details of this problem,
> and keep you informed about progress with filesystems fixes. When all file
> systems will be fixed,
> it allows you to revert "ksmbd: force "fail immediately" flag on fs with its
> own ->lock"
>
> Thank you,
> 	Vasily Averin
>
>>> "[PATCH] ksmbd: force "fail immediately" flag on fs with its own ->lock"
>>> tries to workaround the incorrect behaviour of some exported
>>> filesystems.
>>>
>>> Currently this way is used in nfsd and lockd, however it is not fully
>>> correct,
>>> and I still search some better solution, so perhaps
>>> '[PATCH] ksmbd: force "fail immediately" flag on fs with its own ->lock'
>>> will be dropped later.
>>>
>>> Thank you,
>>> 	Vasily Averin
>>>
>>>>> Signed-off-by: Vasily Averin <vvs@...tuozzo.com>
>>>>> ---
>>>>>  fs/ksmbd/smb2pdu.c | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>>>>> index 0c020deb76bb..34f333549767 100644
>>>>> --- a/fs/ksmbd/smb2pdu.c
>>>>> +++ b/fs/ksmbd/smb2pdu.c
>>>>> @@ -6646,13 +6646,13 @@ static int smb2_set_flock_flags(struct
>>>>> file_lock
>>>>> *flock, int flags)
>>>>>  	switch (flags) {
>>>>>  	case SMB2_LOCKFLAG_SHARED:
>>>>>  		ksmbd_debug(SMB, "received shared request\n");
>>>>> -		cmd = F_SETLKW;
>>>>> +		cmd = F_SETLK;
>>>>>  		flock->fl_type = F_RDLCK;
>>>>>  		flock->fl_flags |= FL_SLEEP;
>>>>>  		break;
>>>>>  	case SMB2_LOCKFLAG_EXCLUSIVE:
>>>>>  		ksmbd_debug(SMB, "received exclusive request\n");
>>>>> -		cmd = F_SETLKW;
>>>>> +		cmd = F_SETLK;
>>>>>  		flock->fl_type = F_WRLCK;
>>>>>  		flock->fl_flags |= FL_SLEEP;
>>>>>  		break;
>>>>> --
>>>>> 2.25.1
>>>>>
>>>>>
>>>
>>>
>
>