diff mbox

[v2] cifs: Fix stack out-of-bounds in smb{2,3}_create_lease_buf()

Message ID d74d053a21d3cb886881bc15d779593b21159c2e.1530795845.git.sbrivio@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Brivio July 5, 2018, 1:10 p.m. UTC
smb{2,3}_create_lease_buf() store a lease key in the lease
context for later usage on a lease break.

In most paths, the key is currently sourced from data that
happens to be on the stack near local variables for oplock in
SMB2_open() callers, e.g. from open_shroot(), whereas
smb2_open_file() properly allocates space on its stack for it.

The address of those local variables holding the oplock is then
passed to create_lease_buf handlers via SMB2_open(), and 16
bytes near oplock are used. This causes a stack out-of-bounds
access as reported by KASAN on SMB2.1 and SMB3 mounts (first
out-of-bounds access is shown here):

[  111.528823] BUG: KASAN: stack-out-of-bounds in smb3_create_lease_buf+0x399/0x3b0 [cifs]
[  111.530815] Read of size 8 at addr ffff88010829f249 by task mount.cifs/985
[  111.532838] CPU: 3 PID: 985 Comm: mount.cifs Not tainted 4.18.0-rc3+ #91
[  111.534656] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[  111.536838] Call Trace:
[  111.537528]  dump_stack+0xc2/0x16b
[  111.540890]  print_address_description+0x6a/0x270
[  111.542185]  kasan_report+0x258/0x380
[  111.544701]  smb3_create_lease_buf+0x399/0x3b0 [cifs]
[  111.546134]  SMB2_open+0x1ef8/0x4b70 [cifs]
[  111.575883]  open_shroot+0x339/0x550 [cifs]
[  111.591969]  smb3_qfs_tcon+0x32c/0x1e60 [cifs]
[  111.617405]  cifs_mount+0x4f3/0x2fc0 [cifs]
[  111.674332]  cifs_smb3_do_mount+0x263/0xf10 [cifs]
[  111.677915]  mount_fs+0x55/0x2b0
[  111.679504]  vfs_kern_mount.part.22+0xaa/0x430
[  111.684511]  do_mount+0xc40/0x2660
[  111.698301]  ksys_mount+0x80/0xd0
[  111.701541]  do_syscall_64+0x14e/0x4b0
[  111.711807]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  111.713665] RIP: 0033:0x7f372385b5fa
[  111.715311] Code: 48 8b 0d 99 78 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 66 78 2c 00 f7 d8 64 89 01 48
[  111.720330] RSP: 002b:00007ffff27049d8 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5
[  111.722601] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f372385b5fa
[  111.724842] RDX: 000055c2ecdc73b2 RSI: 000055c2ecdc73f9 RDI: 00007ffff270580f
[  111.727083] RBP: 00007ffff2705804 R08: 000055c2ee976060 R09: 0000000000001000
[  111.729319] R10: 0000000000000000 R11: 0000000000000206 R12: 00007f3723f4d000
[  111.731615] R13: 000055c2ee976060 R14: 00007f3723f4f90f R15: 0000000000000000

[  111.735448] The buggy address belongs to the page:
[  111.737420] page:ffffea000420a7c0 count:0 mapcount:0 mapping:0000000000000000 index:0x0
[  111.739890] flags: 0x17ffffc0000000()
[  111.741750] raw: 0017ffffc0000000 0000000000000000 dead000000000200 0000000000000000
[  111.744216] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
[  111.746679] page dumped because: kasan: bad access detected

[  111.750482] Memory state around the buggy address:
[  111.752562]  ffff88010829f100: 00 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00
[  111.754991]  ffff88010829f180: 00 00 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
[  111.757401] >ffff88010829f200: 00 00 00 00 00 f1 f1 f1 f1 01 f2 f2 f2 f2 f2 f2
[  111.759801]                                               ^
[  111.762034]  ffff88010829f280: f2 02 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00
[  111.764486]  ffff88010829f300: f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[  111.766913] ==================================================================

Lease keys are however already generated and stored in fid data
on open and create paths: pass them down to the lease context
creation handlers and use them.

Suggested-by: Aurélien Aptel <aaptel@suse.com>
Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2: As suggested by Aurélien, don't generate the lease key using random
    bytes: it's already in fid->lease_key, just use it

 fs/cifs/cifsglob.h |  2 +-
 fs/cifs/smb2file.c | 11 ++++-------
 fs/cifs/smb2ops.c  |  9 +++------
 fs/cifs/smb2pdu.c  |  7 ++++---
 fs/cifs/smb2pdu.h  |  6 ++----
 5 files changed, 14 insertions(+), 21 deletions(-)

Comments

Aurélien Aptel July 5, 2018, 2:32 p.m. UTC | #1
Great! It would be nice to run xfstests after applying the patch as I
find it strange that oplocks worked before this fix. If they start
working now they might be buggy.

Otherwise looks good.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Cheers,
Stefano Brivio July 5, 2018, 3:09 p.m. UTC | #2
On Thu, 05 Jul 2018 16:32:14 +0200
Aurélien Aptel <aaptel@suse.com> wrote:

> Great! It would be nice to run xfstests after applying the patch as I
> find it strange that oplocks worked before this fix. If they start
> working now they might be buggy.

Right, I will run them soon.
Steve French July 6, 2018, 5:49 a.m. UTC | #3
Interesting that I am getting failures on generic/002 and generic/010
I need to recheck configuration.  In my test example target server was
4.7.6-Ubuntu on ext4 rather than Windows or Azure or mainline Samba
(and not on XFS or btrfs) etc
On Fri, Jul 6, 2018 at 6:02 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 05 Jul 2018 16:32:14 +0200
> Aurélien Aptel <aaptel@suse.com> wrote:
>
> > Great! It would be nice to run xfstests after applying the patch as I
> > find it strange that oplocks worked before this fix. If they start
> > working now they might be buggy.
>
> So I ran xfstests with and without both patches, results are the same:
>
> SECTION       -- smb3
> =========================
> Ran: cifs/001 generic/001 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/008 generic/009 generic/010 generic/012 generic/013 generic/014 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/024 generic/025 generic/026 generic/027 generic/028 generic/029 generic/030 generic/031 generic/032 generic/033 generic/034 generic/036 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/069 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/080 generic/082 generic/083 generic/084 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/095 generic/096 generic/098 generic/099 generic/100 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/109 generic/110 generic/111 generic/112 generic/113 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/123 generic/124 generic/128 generic/132 generic/133 generic/134 generic/135 generic/136 generic/137 generic/138 generic/139 generic/140 generic/141 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/169 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/198 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/207 generic/208 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/216 generic/217 generic/218 generic/219 generic/220 generic/221 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/239 generic/240 generic/241 generic/242 generic/243 generic/244 generic/246 generic/247 generic/248 generic/249 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/257 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/285 generic/286 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/308 generic/309 generic/310 generic/311 generic/312 generic/315 generic/316 generic/320 generic/321 generic/322 generic/323 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/339 generic/340 generic/341 generic/342 generic/343 generic/344 generic/345 generic/346 generic/347 generic/348 generic/352 generic/353 generic/354 generic/355 generic/356 generic/357 generic/358 generic/359 generic/360 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/378 generic/384 generic/386 generic/389 generic/391 generic/394 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/428 generic/432 generic/433 generic/436 generic/437 generic/443 generic/444 generic/445 generic/448 generic/450 generic/451 generic/463 generic/465 generic/467 generic/468 generic/469 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/478 generic/479 generic/480 generic/481 generic/482 generic/483 generic/484 generic/485 generic/486 generic/487 generic/488 generic/489 generic/490 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> Not run: generic/003 generic/004 generic/008 generic/009 generic/012 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/025 generic/026 generic/027 generic/031 generic/032 generic/033 generic/034 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/082 generic/083 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/096 generic/099 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/110 generic/111 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/134 generic/136 generic/137 generic/138 generic/139 generic/140 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/213 generic/214 generic/216 generic/217 generic/218 generic/219 generic/220 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/240 generic/241 generic/242 generic/243 generic/244 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/310 generic/311 generic/312 generic/316 generic/320 generic/321 generic/322 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/341 generic/342 generic/343 generic/347 generic/348 generic/352 generic/353 generic/356 generic/357 generic/358 generic/359 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/384 generic/386 generic/389 generic/391 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/432 generic/433 generic/444 generic/450 generic/463 generic/467 generic/468 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/479 generic/480 generic/481 generic/482 generic/483 generic/485 generic/486 generic/487 generic/488 generic/489 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> Failures: generic/123 generic/128 generic/355 generic/378 generic/478 generic/484
> Failed 6 of 397 tests
>
> And actually I think that oplocks would have also worked before: the
> lease key is generated with generate_random_uuid(), and reading random
> bytes from the stack also generated a random lease key, which was
> properly stored in the context and then used by smb3_parse_lease_buf().
>
> --
> Stefano
> --
> 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
Stefano Brivio July 6, 2018, 10:59 a.m. UTC | #4
On Thu, 05 Jul 2018 16:32:14 +0200
Aurélien Aptel <aaptel@suse.com> wrote:

> Great! It would be nice to run xfstests after applying the patch as I
> find it strange that oplocks worked before this fix. If they start
> working now they might be buggy.

So I ran xfstests with and without both patches, results are the same:

SECTION       -- smb3
=========================
Ran: cifs/001 generic/001 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/008 generic/009 generic/010 generic/012 generic/013 generic/014 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/024 generic/025 generic/026 generic/027 generic/028 generic/029 generic/030 generic/031 generic/032 generic/033 generic/034 generic/036 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/069 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/080 generic/082 generic/083 generic/084 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/095 generic/096 generic/098 generic/099 gene
 ric/100 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/109 generic/110 generic/111 generic/112 generic/113 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/123 generic/124 generic/128 generic/132 generic/133 generic/134 generic/135 generic/136 generic/137 generic/138 generic/139 generic/140 generic/141 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/169 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/1
 91 generic/194 generic/195 generic/196 generic/197 generic/198 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/207 generic/208 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/216 generic/217 generic/218 generic/219 generic/220 generic/221 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/239 generic/240 generic/241 generic/242 generic/243 generic/244 generic/246 generic/247 generic/248 generic/249 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/257 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 ge
 neric/285 generic/286 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/308 generic/309 generic/310 generic/311 generic/312 generic/315 generic/316 generic/320 generic/321 generic/322 generic/323 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/339 generic/340 generic/341 generic/342 generic/343 generic/344 generic/345 generic/346 generic/347 generic/348 generic/352 generic/353 generic/354 generic/355 generic/356 generic/357 generic/358 generic/359 generic/360 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/378 generic
 /384 generic/386 generic/389 generic/391 generic/394 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/428 generic/432 generic/433 generic/436 generic/437 generic/443 generic/444 generic/445 generic/448 generic/450 generic/451 generic/463 generic/465 generic/467 generic/468 generic/469 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/478 generic/479 generic/480 generic/481 generic/482 generic/483 generic/484 generic/485 generic/486 generic/487 generic/488 generic/489 generic/490 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
Not run: generic/003 generic/004 generic/008 generic/009 generic/012 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/025 generic/026 generic/027 generic/031 generic/032 generic/033 generic/034 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/082 generic/083 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/096 generic/099 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/110 generic/111 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/134 generic/1
 36 generic/137 generic/138 generic/139 generic/140 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/213 generic/214 generic/216 generic/217 generic/218 generic/219 generic/220 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 ge
 neric/233 generic/234 generic/235 generic/238 generic/240 generic/241 generic/242 generic/243 generic/244 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/310 generic/311 generic/312 generic/316 generic/320 generic/321 generic/322 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/341 generic/342 generic/343 generic/347 generic
 /348 generic/352 generic/353 generic/356 generic/357 generic/358 generic/359 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/384 generic/386 generic/389 generic/391 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/432 generic/433 generic/444 generic/450 generic/463 generic/467 generic/468 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/479 generic/480 generic/481 generic/482 generic/483 generic/485 generic/486 generic/487 generic/488 generic/489 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
Failures: generic/123 generic/128 generic/355 generic/378 generic/478 generic/484
Failed 6 of 397 tests

And actually I think that oplocks would have also worked before: the
lease key is generated with generate_random_uuid(), and reading random
bytes from the stack also generated a random lease key, which was
properly stored in the context and then used by smb3_parse_lease_buf().
Steve French July 6, 2018, 2:56 p.m. UTC | #5
Mystery mostly solved ...

generic/002 was a library incompatibility - failing due to Ubuntu
upgrade, had to delete dbtest (or make clean) and rebuild xfstests
generic/010 is testing link count on hardlinks and seems to mostly
work.  My theory is that when I run without actime=0 we have a minor
problem with caching of the link count - worth investigating but also
possible that Samba sometimes returns the wrong linkcount.  This test
usually passes for me.
On Fri, Jul 6, 2018 at 12:49 AM Steve French <smfrench@gmail.com> wrote:
>
> Interesting that I am getting failures on generic/002 and generic/010
> I need to recheck configuration.  In my test example target server was
> 4.7.6-Ubuntu on ext4 rather than Windows or Azure or mainline Samba
> (and not on XFS or btrfs) etc
> On Fri, Jul 6, 2018 at 6:02 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Thu, 05 Jul 2018 16:32:14 +0200
> > Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > > Great! It would be nice to run xfstests after applying the patch as I
> > > find it strange that oplocks worked before this fix. If they start
> > > working now they might be buggy.
> >
> > So I ran xfstests with and without both patches, results are the same:
> >
> > SECTION       -- smb3
> > =========================
> > Ran: cifs/001 generic/001 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/008 generic/009 generic/010 generic/012 generic/013 generic/014 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/024 generic/025 generic/026 generic/027 generic/028 generic/029 generic/030 generic/031 generic/032 generic/033 generic/034 generic/036 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/069 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/080 generic/082 generic/083 generic/084 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/095 generic/096 generic/098 generic/099 generic/100 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/109 generic/110 generic/111 generic/112 generic/113 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/123 generic/124 generic/128 generic/132 generic/133 generic/134 generic/135 generic/136 generic/137 generic/138 generic/139 generic/140 generic/141 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/169 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/198 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/207 generic/208 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/216 generic/217 generic/218 generic/219 generic/220 generic/221 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/239 generic/240 generic/241 generic/242 generic/243 generic/244 generic/246 generic/247 generic/248 generic/249 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/257 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/285 generic/286 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/308 generic/309 generic/310 generic/311 generic/312 generic/315 generic/316 generic/320 generic/321 generic/322 generic/323 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/339 generic/340 generic/341 generic/342 generic/343 generic/344 generic/345 generic/346 generic/347 generic/348 generic/352 generic/353 generic/354 generic/355 generic/356 generic/357 generic/358 generic/359 generic/360 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/378 generic/384 generic/386 generic/389 generic/391 generic/394 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/428 generic/432 generic/433 generic/436 generic/437 generic/443 generic/444 generic/445 generic/448 generic/450 generic/451 generic/463 generic/465 generic/467 generic/468 generic/469 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/478 generic/479 generic/480 generic/481 generic/482 generic/483 generic/484 generic/485 generic/486 generic/487 generic/488 generic/489 generic/490 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> > Not run: generic/003 generic/004 generic/008 generic/009 generic/012 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/025 generic/026 generic/027 generic/031 generic/032 generic/033 generic/034 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/082 generic/083 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/096 generic/099 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/110 generic/111 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/134 generic/136 generic/137 generic/138 generic/139 generic/140 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/213 generic/214 generic/216 generic/217 generic/218 generic/219 generic/220 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/240 generic/241 generic/242 generic/243 generic/244 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/310 generic/311 generic/312 generic/316 generic/320 generic/321 generic/322 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/341 generic/342 generic/343 generic/347 generic/348 generic/352 generic/353 generic/356 generic/357 generic/358 generic/359 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/384 generic/386 generic/389 generic/391 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/432 generic/433 generic/444 generic/450 generic/463 generic/467 generic/468 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/479 generic/480 generic/481 generic/482 generic/483 generic/485 generic/486 generic/487 generic/488 generic/489 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> > Failures: generic/123 generic/128 generic/355 generic/378 generic/478 generic/484
> > Failed 6 of 397 tests
> >
> > And actually I think that oplocks would have also worked before: the
> > lease key is generated with generate_random_uuid(), and reading random
> > bytes from the stack also generated a random lease key, which was
> > properly stored in the context and then used by smb3_parse_lease_buf().
> >
> > --
> > Stefano
> > --
> > 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
>
>
>
> --
> Thanks,
>
> Steve
Steve French July 7, 2018, 5:53 p.m. UTC | #6
Interesting results - test 129 wasn't too bad when I tried it (it is
on the "very slow" exclude list so isn't usually run vs. cifs)

# ./check -cifs generic/129
FSTYP         -- cifs
PLATFORM      -- Linux/x86_64 Ubuntu-17-Virtual-Machine 4.18.0-041800rc3-generic
MKFS_OPTIONS  -- //localhost/scratch
MOUNT_OPTIONS --
-ousername=sfrench,vers=3.0,mfsymlinks,actimeo=0,noperm,password=505Falkirk!
//localhost/scratch /mnt-local-xfstest/scratch

generic/129 38s ... 46s
Ran: generic/129
Passed all 1 tests
On Fri, Jul 6, 2018 at 6:02 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Thu, 05 Jul 2018 16:32:14 +0200
> Aurélien Aptel <aaptel@suse.com> wrote:
>
> > Great! It would be nice to run xfstests after applying the patch as I
> > find it strange that oplocks worked before this fix. If they start
> > working now they might be buggy.
>
> So I ran xfstests with and without both patches, results are the same:
>
> SECTION       -- smb3
> =========================
> Ran: cifs/001 generic/001 generic/002 generic/003 generic/004 generic/005 generic/006 generic/007 generic/008 generic/009 generic/010 generic/012 generic/013 generic/014 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/024 generic/025 generic/026 generic/027 generic/028 generic/029 generic/030 generic/031 generic/032 generic/033 generic/034 generic/036 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/069 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/080 generic/082 generic/083 generic/084 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/095 generic/096 generic/098 generic/099 generic/100 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/109 generic/110 generic/111 generic/112 generic/113 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/123 generic/124 generic/128 generic/132 generic/133 generic/134 generic/135 generic/136 generic/137 generic/138 generic/139 generic/140 generic/141 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/169 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/198 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/207 generic/208 generic/210 generic/211 generic/212 generic/213 generic/214 generic/215 generic/216 generic/217 generic/218 generic/219 generic/220 generic/221 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/239 generic/240 generic/241 generic/242 generic/243 generic/244 generic/246 generic/247 generic/248 generic/249 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/257 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/285 generic/286 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/308 generic/309 generic/310 generic/311 generic/312 generic/315 generic/316 generic/320 generic/321 generic/322 generic/323 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/339 generic/340 generic/341 generic/342 generic/343 generic/344 generic/345 generic/346 generic/347 generic/348 generic/352 generic/353 generic/354 generic/355 generic/356 generic/357 generic/358 generic/359 generic/360 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/378 generic/384 generic/386 generic/389 generic/391 generic/394 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/428 generic/432 generic/433 generic/436 generic/437 generic/443 generic/444 generic/445 generic/448 generic/450 generic/451 generic/463 generic/465 generic/467 generic/468 generic/469 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/478 generic/479 generic/480 generic/481 generic/482 generic/483 generic/484 generic/485 generic/486 generic/487 generic/488 generic/489 generic/490 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> Not run: generic/003 generic/004 generic/008 generic/009 generic/012 generic/015 generic/016 generic/017 generic/018 generic/021 generic/022 generic/025 generic/026 generic/027 generic/031 generic/032 generic/033 generic/034 generic/038 generic/039 generic/040 generic/041 generic/043 generic/044 generic/045 generic/046 generic/047 generic/048 generic/049 generic/050 generic/051 generic/052 generic/053 generic/054 generic/055 generic/056 generic/057 generic/058 generic/059 generic/060 generic/061 generic/063 generic/064 generic/065 generic/066 generic/067 generic/068 generic/072 generic/073 generic/076 generic/077 generic/078 generic/079 generic/082 generic/083 generic/085 generic/086 generic/090 generic/092 generic/093 generic/094 generic/096 generic/099 generic/101 generic/102 generic/103 generic/104 generic/105 generic/106 generic/107 generic/108 generic/110 generic/111 generic/114 generic/115 generic/116 generic/118 generic/119 generic/121 generic/122 generic/134 generic/136 generic/137 generic/138 generic/139 generic/140 generic/142 generic/143 generic/144 generic/145 generic/146 generic/147 generic/148 generic/149 generic/150 generic/151 generic/152 generic/153 generic/154 generic/155 generic/156 generic/157 generic/158 generic/159 generic/160 generic/161 generic/162 generic/163 generic/164 generic/165 generic/166 generic/167 generic/168 generic/170 generic/171 generic/172 generic/173 generic/174 generic/175 generic/176 generic/177 generic/178 generic/179 generic/180 generic/181 generic/182 generic/183 generic/185 generic/186 generic/187 generic/188 generic/189 generic/190 generic/191 generic/194 generic/195 generic/196 generic/197 generic/199 generic/200 generic/201 generic/202 generic/203 generic/204 generic/205 generic/206 generic/213 generic/214 generic/216 generic/217 generic/218 generic/219 generic/220 generic/222 generic/223 generic/224 generic/225 generic/226 generic/227 generic/228 generic/229 generic/230 generic/231 generic/232 generic/233 generic/234 generic/235 generic/238 generic/240 generic/241 generic/242 generic/243 generic/244 generic/250 generic/252 generic/253 generic/254 generic/255 generic/256 generic/259 generic/260 generic/261 generic/262 generic/264 generic/265 generic/266 generic/267 generic/268 generic/269 generic/271 generic/272 generic/273 generic/274 generic/275 generic/276 generic/278 generic/279 generic/280 generic/281 generic/282 generic/283 generic/284 generic/287 generic/288 generic/289 generic/290 generic/291 generic/292 generic/293 generic/295 generic/296 generic/297 generic/298 generic/299 generic/300 generic/301 generic/302 generic/303 generic/304 generic/305 generic/307 generic/310 generic/311 generic/312 generic/316 generic/320 generic/321 generic/322 generic/324 generic/325 generic/326 generic/327 generic/328 generic/329 generic/330 generic/331 generic/332 generic/333 generic/334 generic/335 generic/336 generic/338 generic/341 generic/342 generic/343 generic/347 generic/348 generic/352 generic/353 generic/356 generic/357 generic/358 generic/359 generic/361 generic/362 generic/363 generic/364 generic/365 generic/366 generic/367 generic/368 generic/369 generic/370 generic/371 generic/372 generic/373 generic/374 generic/375 generic/376 generic/384 generic/386 generic/389 generic/391 generic/400 generic/404 generic/407 generic/408 generic/418 generic/420 generic/426 generic/432 generic/433 generic/444 generic/450 generic/463 generic/467 generic/468 generic/470 generic/471 generic/472 generic/474 generic/475 generic/476 generic/477 generic/479 generic/480 generic/481 generic/482 generic/483 generic/485 generic/486 generic/487 generic/488 generic/489 generic/491 generic/492 generic/493 generic/494 generic/495 generic/496 generic/497 generic/498 shared/001 shared/002 shared/003 shared/004 shared/006 shared/008 shared/009 shared/010 shared/032 shared/272 shared/289 shared/298
> Failures: generic/123 generic/128 generic/355 generic/378 generic/478 generic/484
> Failed 6 of 397 tests
>
> And actually I think that oplocks would have also worked before: the
> lease key is generated with generate_random_uuid(), and reading random
> bytes from the stack also generated a random lease key, which was
> properly stored in the context and then used by smb3_parse_lease_buf().
>
> --
> Stefano
> --
> 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
Pavel Shilovsky July 26, 2018, 10:30 p.m. UTC | #7
> Suggested-by: Aurélien Aptel <aaptel@suse.com>
> Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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.

--
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
Stefano Brivio July 27, 2018, 12:26 p.m. UTC | #8
Hi Pavel,

On Thu, 26 Jul 2018 15:30:32 -0700
Pavel Shilovsky <piastryyy@gmail.com> wrote:

> > Suggested-by: Aurélien Aptel <aaptel@suse.com>
> > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> 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.

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?
Pavel Shilovsky July 27, 2018, 6:12 p.m. UTC | #9
2018-07-27 5:26 GMT-07:00 Stefano Brivio <sbrivio@redhat.com>:
> Hi Pavel,
>
> On Thu, 26 Jul 2018 15:30:32 -0700
> Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
>> > Suggested-by: Aurélien Aptel <aaptel@suse.com>
>> > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
>> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
>>
>> 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.

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.

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.

Anyway, thanks for fixing the problem and cleaning up the code - now
it looks more intuitive and the behavior is predictable!

--
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
Stefano Brivio July 28, 2018, 2 a.m. UTC | #10
On Fri, 27 Jul 2018 11:12:09 -0700
Pavel Shilovsky <piastryyy@gmail.com> wrote:

> 2018-07-27 5:26 GMT-07:00 Stefano Brivio <sbrivio@redhat.com>:
> > Hi Pavel,
> >
> > On Thu, 26 Jul 2018 15:30:32 -0700
> > Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >  
> >> > Suggested-by: Aurélien Aptel <aaptel@suse.com>
> >> > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
> >> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> >>
> >> 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?
Steve French July 28, 2018, 3:13 a.m. UTC | #11
Following Pavel's suggestion - this commit look ok?



On Fri, Jul 27, 2018 at 9:00 PM Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 27 Jul 2018 11:12:09 -0700
> Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> > 2018-07-27 5:26 GMT-07:00 Stefano Brivio <sbrivio@redhat.com>:
> > > Hi Pavel,
> > >
> > > On Thu, 26 Jul 2018 15:30:32 -0700
> > > Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > >> > Suggested-by: Aurélien Aptel <aaptel@suse.com>
> > >> > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases")
> > >> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > >>
> > >> 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
>
Pavel Shilovsky Aug. 1, 2018, 9:33 p.m. UTC | #12
2018-07-27 20:13 GMT-07:00 Steve French <smfrench@gmail.com>:
> Following Pavel's suggestion - this commit look ok?

Looks good. We should probably indicate specific kernels that are affected:

Cc: <stable@vger.kernel.org> # 3.18.x+

--
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
Steve French Aug. 1, 2018, 9:42 p.m. UTC | #13
Yes. Will fix

On Wed, Aug 1, 2018, 14:33 Pavel Shilovsky <piastryyy@gmail.com> wrote:

> 2018-07-27 20:13 GMT-07:00 Steve French <smfrench@gmail.com>:
> > Following Pavel's suggestion - this commit look ok?
>
> Looks good. We should probably indicate specific kernels that are affected:
>
> Cc: <stable@vger.kernel.org> # 3.18.x+
>
> --
> Best regards,
> Pavel Shilovsky
>
<div dir="auto">Yes. Will fix</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 1, 2018, 14:33 Pavel Shilovsky &lt;<a href="mailto:piastryyy@gmail.com">piastryyy@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">2018-07-27 20:13 GMT-07:00 Steve French &lt;<a href="mailto:smfrench@gmail.com" target="_blank" rel="noreferrer">smfrench@gmail.com</a>&gt;:<br>
&gt; Following Pavel&#39;s suggestion - this commit look ok?<br>
<br>
Looks good. We should probably indicate specific kernels that are affected:<br>
<br>
Cc: &lt;<a href="mailto:stable@vger.kernel.org" target="_blank" rel="noreferrer">stable@vger.kernel.org</a>&gt; # 3.18.x+<br>
<br>
--<br>
Best regards,<br>
Pavel Shilovsky<br>
</blockquote></div>
diff mbox

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index bd78da59a4fd..07a698d7b103 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -423,7 +423,7 @@  struct smb_version_operations {
 	void (*set_oplock_level)(struct cifsInodeInfo *, __u32, unsigned int,
 				 bool *);
 	/* create lease context buffer for CREATE request */
-	char * (*create_lease_buf)(u8 *, u8);
+	char * (*create_lease_buf)(u8 *lease_key, u8 oplock);
 	/* parse lease context buffer and return oplock/epoch info */
 	__u8 (*parse_lease_buf)(void *buf, unsigned int *epoch, char *lkey);
 	ssize_t (*copychunk_range)(const unsigned int,
diff --git a/fs/cifs/smb2file.c b/fs/cifs/smb2file.c
index 788412675723..4ed10dd086e6 100644
--- a/fs/cifs/smb2file.c
+++ b/fs/cifs/smb2file.c
@@ -41,7 +41,7 @@  smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms,
 	int rc;
 	__le16 *smb2_path;
 	struct smb2_file_all_info *smb2_data = NULL;
-	__u8 smb2_oplock[17];
+	__u8 smb2_oplock;
 	struct cifs_fid *fid = oparms->fid;
 	struct network_resiliency_req nr_ioctl_req;
 
@@ -59,12 +59,9 @@  smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms,
 	}
 
 	oparms->desired_access |= FILE_READ_ATTRIBUTES;
-	*smb2_oplock = SMB2_OPLOCK_LEVEL_BATCH;
+	smb2_oplock = SMB2_OPLOCK_LEVEL_BATCH;
 
-	if (oparms->tcon->ses->server->capabilities & SMB2_GLOBAL_CAP_LEASING)
-		memcpy(smb2_oplock + 1, fid->lease_key, SMB2_LEASE_KEY_SIZE);
-
-	rc = SMB2_open(xid, oparms, smb2_path, smb2_oplock, smb2_data, NULL,
+	rc = SMB2_open(xid, oparms, smb2_path, &smb2_oplock, smb2_data, NULL,
 		       NULL);
 	if (rc)
 		goto out;
@@ -101,7 +98,7 @@  smb2_open_file(const unsigned int xid, struct cifs_open_parms *oparms,
 		move_smb2_info_to_cifs(buf, smb2_data);
 	}
 
-	*oplock = *smb2_oplock;
+	*oplock = smb2_oplock;
 out:
 	kfree(smb2_data);
 	kfree(smb2_path);
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 0356b5559c71..f684871a6d3d 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -2219,8 +2219,7 @@  smb2_create_lease_buf(u8 *lease_key, u8 oplock)
 	if (!buf)
 		return NULL;
 
-	buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key));
-	buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8)));
+	memcpy(&buf->lcontext.LeaseKey, lease_key, SMB2_LEASE_KEY_SIZE);
 	buf->lcontext.LeaseState = map_oplock_to_lease(oplock);
 
 	buf->ccontext.DataOffset = cpu_to_le16(offsetof
@@ -2246,8 +2245,7 @@  smb3_create_lease_buf(u8 *lease_key, u8 oplock)
 	if (!buf)
 		return NULL;
 
-	buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key));
-	buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8)));
+	memcpy(&buf->lcontext.LeaseKey, lease_key, SMB2_LEASE_KEY_SIZE);
 	buf->lcontext.LeaseState = map_oplock_to_lease(oplock);
 
 	buf->ccontext.DataOffset = cpu_to_le16(offsetof
@@ -2284,8 +2282,7 @@  smb3_parse_lease_buf(void *buf, unsigned int *epoch, char *lease_key)
 	if (lc->lcontext.LeaseFlags & SMB2_LEASE_FLAG_BREAK_IN_PROGRESS)
 		return SMB2_OPLOCK_LEVEL_NOCHANGE;
 	if (lease_key)
-		memcpy(lease_key, &lc->lcontext.LeaseKeyLow,
-		       SMB2_LEASE_KEY_SIZE);
+		memcpy(lease_key, &lc->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE);
 	return le32_to_cpu(lc->lcontext.LeaseState);
 }
 
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 810b85787c91..b49b6ecf5b6f 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1707,12 +1707,12 @@  parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp,
 
 static int
 add_lease_context(struct TCP_Server_Info *server, struct kvec *iov,
-		  unsigned int *num_iovec, __u8 *oplock)
+		  unsigned int *num_iovec, u8 *lease_key, __u8 *oplock)
 {
 	struct smb2_create_req *req = iov[0].iov_base;
 	unsigned int num = *num_iovec;
 
-	iov[num].iov_base = server->ops->create_lease_buf(oplock+1, *oplock);
+	iov[num].iov_base = server->ops->create_lease_buf(lease_key, *oplock);
 	if (iov[num].iov_base == NULL)
 		return -ENOMEM;
 	iov[num].iov_len = server->vals->create_lease_size;
@@ -2172,7 +2172,8 @@  SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
 	    *oplock == SMB2_OPLOCK_LEVEL_NONE)
 		req->RequestedOplockLevel = *oplock;
 	else {
-		rc = add_lease_context(server, iov, &n_iov, oplock);
+		rc = add_lease_context(server, iov, &n_iov,
+				       oparms->fid->lease_key, oplock);
 		if (rc) {
 			cifs_small_buf_release(req);
 			kfree(copy_path);
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index 824dddeee3f2..a671adcc44a6 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -678,16 +678,14 @@  struct create_context {
 #define SMB2_LEASE_KEY_SIZE 16
 
 struct lease_context {
-	__le64 LeaseKeyLow;
-	__le64 LeaseKeyHigh;
+	u8 LeaseKey[SMB2_LEASE_KEY_SIZE];
 	__le32 LeaseState;
 	__le32 LeaseFlags;
 	__le64 LeaseDuration;
 } __packed;
 
 struct lease_context_v2 {
-	__le64 LeaseKeyLow;
-	__le64 LeaseKeyHigh;
+	u8 LeaseKey[SMB2_LEASE_KEY_SIZE];
 	__le32 LeaseState;
 	__le32 LeaseFlags;
 	__le64 LeaseDuration;