Following Pavel's suggestion - this commit look ok?
On Fri, Jul 27, 2018 at 9:00 PM Stefano Brivio <sbrivio@xxxxxxxxxx> wrote:
On Fri, 27 Jul 2018 11:12:09 -0700
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2018-07-27 5:26 GMT-07:00 Stefano Brivio <sbrivio@xxxxxxxxxx>:
> > Hi Pavel,
> >
> > On Thu, 26 Jul 2018 15:30:32 -0700
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> >> > Suggested-by: Aurélien Aptel <aaptel@xxxxxxxx>
> >> > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
> >> > Signed-off-by: Stefano Brivio <sbrivio@xxxxxxxxxx>
> >>
> >> The commit listed above is not correct. The one that introduced the
> >> problem is a93864d93977b99bda6c348a09b90a3d7ef8db3a
> >> (https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commitdiff;h=a93864d93977b99bda6c348a09b90a3d7ef8db3a;hp=2fbb56446fde14a80790de9b182ae6f7c36a039a)
> >> was merged recently, so, there is no need to push to stable kernels as
> >> it might be seemed previously looking at the problematic commit.
> >
>
> Hi Stefano,
>
> > Maybe I'm missing something, but commit b8c32dbb0deb ("CIFS: Request
> > SMB2.1 leases") introduces:
> >
> > buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key));
> > buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8)));
> >
> > in create_lease_buf(). If we reach that coming from smb2_open_file(),
> > it's fine, but if we're coming from other callers of SMB2_open() (see e.g.
> > smb2_query_dir_first() at b8c32dbb0deb) we hit the same issue, don't we?
>
> create_lease_buf() is called by add_lease_context() which is called if
> (server->capabilities & SMB2_GLOBAL_CAP_LEASING) and oplock is not
> NONE. smb2_query_dir_first() sets oplock level to NONE, so is not
> affected by this issue. Commit
> a93864d93977b99bda6c348a09b90a3d7ef8db3a changed a lease level to
> LEVEL_II that's why you caught the issue.
Ah, I see, thanks for pointing that out!
> Actually, when I grep'ed for "SMB2_OPLOCK_LEVEL" in the code I noticed
> that the problem existed even before: if we mount with mfsymlink
> option we should hit the same bug (since kernel v3.18). Thus, the
> proper fix needs to be pushed to stable but the current patch relies
> on fid->lease_key which was added very recently.
So the Fixes: tag should reference commit 5ab97578cbb3 ("Add mfsymlinks
support for SMB2.1/SMB3. Part 1 create symlink"), but just like commit
a93864d93977 ("cifs: add lease tracking to the cached root fid") it also
looks innocent without commit b8c32dbb0deb ("CIFS: Request SMB2.1 leases").
In the end, I think that the original Fixes: tag is not that wrong,
because that commit somehow introduced a trap multiple authors fell into,
even though it didn't introduce functional issues by itself.
But sure, for stable trees purposes, I see the issue.
> I would say that we need another patch for that: we don't need to
> request a lease in mfsymlink code, so we can just change
> smb3_create_mf_symlink() and smb3_query_mf_symlink() to use
> SMB2_OPLOCK_LEVEL_NONE instead. This can be done in both mainline and
> stable trees, so, we can merge it easily.
Yes, that looks way easier. I guess you'll submit that patch, correct?
--
Stefano
--
Thanks,
Steve
Steve
From f322424a82c1e6d04946b1dfdc8a643b83cac631 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@xxxxxxxxxxxxx> Date: Fri, 27 Jul 2018 22:01:49 -0500 Subject: [PATCH] smb3: don't request leases in symlink creation and query Fixes problem pointed out by Pavel in discussions about commit 729c0c9dd55204f0c9a823ac8a7bfa83d36c7e78 Signed-off-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx> Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx> CC: Stable <stable@xxxxxxxxxxxxxxx> --- fs/cifs/link.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/cifs/link.c b/fs/cifs/link.c index de41f96aba49..2148b0f60e5e 100644 --- a/fs/cifs/link.c +++ b/fs/cifs/link.c @@ -396,7 +396,7 @@ smb3_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon, struct cifs_io_parms io_parms; int buf_type = CIFS_NO_BUFFER; __le16 *utf16_path; - __u8 oplock = SMB2_OPLOCK_LEVEL_II; + __u8 oplock = SMB2_OPLOCK_LEVEL_NONE; struct smb2_file_all_info *pfile_info = NULL; oparms.tcon = tcon; @@ -459,7 +459,7 @@ smb3_create_mf_symlink(unsigned int xid, struct cifs_tcon *tcon, struct cifs_io_parms io_parms; int create_options = CREATE_NOT_DIR; __le16 *utf16_path; - __u8 oplock = SMB2_OPLOCK_LEVEL_EXCLUSIVE; + __u8 oplock = SMB2_OPLOCK_LEVEL_NONE; struct kvec iov[2]; if (backup_cred(cifs_sb)) -- 2.17.1
