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
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
