| Message ID | d74d053a21d3cb886881bc15d779593b21159c2e.1530795845.git.sbrivio@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] cifs: Fix stack out-of-bounds in smb{2,3}_create_lease_buf() | expand |
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,
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.
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
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().
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
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
> 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
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?
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
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?
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 >
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
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 <<a href="mailto:piastryyy@gmail.com">piastryyy@gmail.com</a>> 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 <<a href="mailto:smfrench@gmail.com" target="_blank" rel="noreferrer">smfrench@gmail.com</a>>:<br> > Following Pavel's suggestion - this commit look ok?<br> <br> Looks good. We should probably indicate specific kernels that are affected:<br> <br> Cc: <<a href="mailto:stable@vger.kernel.org" target="_blank" rel="noreferrer">stable@vger.kernel.org</a>> # 3.18.x+<br> <br> --<br> Best regards,<br> Pavel Shilovsky<br> </blockquote></div>
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;
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(-)