From f559db56904cacfe20027d710608e981087c1ddd Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Wed, 2 Dec 2020 19:38:49 -0800 Subject: [PATCH] Do not unconditionally allocate in kernel.FDTable.setAll(). `slice := *(*[]unsafe.Pointer)(...)` makes a copy of the slice header, which then escapes because of the conditional `atomic.StorePointer(&f.slice, &slice)` from table expansion. This occurs even when the table doesn't expand, and when it can't (e.g. `close()` => `f.setAll(nil)`). Fix this by avoiding the copy until after table expansion. Before this CL: ``` TEXT pkg/sentry/kernel/kernel.(*FDTable).setAll(SB) pkg/sentry/kernel/fd_table_unsafe.go fd_table_unsafe.go:119 0x7f00005f50e0 64488b0c25f8ffffff MOVQ FS:0xfffffff8, CX fd_table_unsafe.go:119 0x7f00005f50e9 483b6110 CMPQ 0x10(CX), SP fd_table_unsafe.go:119 0x7f00005f50ed 0f864d040000 JBE 0x7f00005f5540 fd_table_unsafe.go:119 0x7f00005f50f3 4883c480 ADDQ $-0x80, SP fd_table_unsafe.go:119 0x7f00005f50f7 48896c2478 MOVQ BP, 0x78(SP) fd_table_unsafe.go:119 0x7f00005f50fc 488d6c2478 LEAQ 0x78(SP), BP fd_table_unsafe.go:120 0x7f00005f5101 488b8424a8000000 MOVQ 0xa8(SP), AX fd_table_unsafe.go:120 0x7f00005f5109 4885c0 TESTQ AX, AX fd_table_unsafe.go:120 0x7f00005f510c 7411 JE 0x7f00005f511f fd_table_unsafe.go:120 0x7f00005f510e 488b8c24b0000000 MOVQ 0xb0(SP), CX fd_table_unsafe.go:120 0x7f00005f5116 4885c9 TESTQ CX, CX fd_table_unsafe.go:120 0x7f00005f5119 0f8500040000 JNE 0x7f00005f551f fd_table_unsafe.go:124 0x7f00005f511f 488d05da115700 LEAQ 0x5711da(IP), AX fd_table_unsafe.go:124 0x7f00005f5126 48890424 MOVQ AX, 0(SP) fd_table_unsafe.go:124 0x7f00005f512a e8d19fa1ff CALL runtime.newobject(SB) fd_table_unsafe.go:124 0x7f00005f512f 488b7c2408 MOVQ 0x8(SP), DI fd_table_unsafe.go:124 0x7f00005f5134 488b842488000000 MOVQ 0x88(SP), AX fd_table_unsafe.go:124 0x7f00005f513c 488b4820 MOVQ 0x20(AX), CX fd_table_unsafe.go:124 0x7f00005f5140 488b5108 MOVQ 0x8(CX), DX fd_table_unsafe.go:124 0x7f00005f5144 488b19 MOVQ 0(CX), BX fd_table_unsafe.go:124 0x7f00005f5147 488b4910 MOVQ 0x10(CX), CX fd_table_unsafe.go:124 0x7f00005f514b 48895708 MOVQ DX, 0x8(DI) fd_table_unsafe.go:124 0x7f00005f514f 48894f10 MOVQ CX, 0x10(DI) fd_table_unsafe.go:124 0x7f00005f5153 833df6e1120100 CMPL $0x0, runtime.writeBarrier(SB) fd_table_unsafe.go:124 0x7f00005f515a 660f1f440000 NOPW 0(AX)(AX*1) fd_table_unsafe.go:124 0x7f00005f5160 0f8589030000 JNE 0x7f00005f54ef fd_table_unsafe.go:124 0x7f00005f5166 48891f MOVQ BX, 0(DI) fd_table_unsafe.go:124 0x7f00005f5169 48897c2470 MOVQ DI, 0x70(SP) fd_table_unsafe.go:127 0x7f00005f516e 8bb424a0000000 MOVL 0xa0(SP), SI fd_table_unsafe.go:127 0x7f00005f5175 39d6 CMPL DX, SI fd_table_unsafe.go:127 0x7f00005f5177 0f8c5f030000 JL 0x7f00005f54dc ... ``` After this CL: ``` TEXT pkg/sentry/kernel/kernel.(*FDTable).setAll(SB) pkg/sentry/kernel/fd_table_unsafe.go fd_table_unsafe.go:119 0x7f00005f50e0 64488b0c25f8ffffff MOVQ FS:0xfffffff8, CX fd_table_unsafe.go:119 0x7f00005f50e9 488d4424e8 LEAQ -0x18(SP), AX fd_table_unsafe.go:119 0x7f00005f50ee 483b4110 CMPQ 0x10(CX), AX fd_table_unsafe.go:119 0x7f00005f50f2 0f868e040000 JBE 0x7f00005f5586 fd_table_unsafe.go:119 0x7f00005f50f8 4881ec98000000 SUBQ $0x98, SP fd_table_unsafe.go:119 0x7f00005f50ff 4889ac2490000000 MOVQ BP, 0x90(SP) fd_table_unsafe.go:119 0x7f00005f5107 488dac2490000000 LEAQ 0x90(SP), BP fd_table_unsafe.go:120 0x7f00005f510f 488b9424c0000000 MOVQ 0xc0(SP), DX fd_table_unsafe.go:120 0x7f00005f5117 660f1f840000000000 NOPW 0(AX)(AX*1) fd_table_unsafe.go:120 0x7f00005f5120 4885d2 TESTQ DX, DX fd_table_unsafe.go:120 0x7f00005f5123 0f8406040000 JE 0x7f00005f552f fd_table_unsafe.go:120 0x7f00005f5129 488b9c24c8000000 MOVQ 0xc8(SP), BX fd_table_unsafe.go:120 0x7f00005f5131 4885db TESTQ BX, BX fd_table_unsafe.go:120 0x7f00005f5134 0f852b040000 JNE 0x7f00005f5565 fd_table_unsafe.go:124 0x7f00005f513a 488bb424a0000000 MOVQ 0xa0(SP), SI fd_table_unsafe.go:124 0x7f00005f5142 488b7e20 MOVQ 0x20(SI), DI fd_table_unsafe.go:127 0x7f00005f5146 4c8b4708 MOVQ 0x8(DI), R8 fd_table_unsafe.go:127 0x7f00005f514a 448b8c24b8000000 MOVL 0xb8(SP), R9 fd_table_unsafe.go:127 0x7f00005f5152 4539c1 CMPL R8, R9 fd_table_unsafe.go:127 0x7f00005f5155 0f8d4a020000 JGE 0x7f00005f53a5 ... ``` PiperOrigin-RevId: 345363242 --- pkg/sentry/kernel/fd_table_unsafe.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/kernel/fd_table_unsafe.go b/pkg/sentry/kernel/fd_table_unsafe.go index 470d8bf83..f17f9c59c 100644 --- a/pkg/sentry/kernel/fd_table_unsafe.go +++ b/pkg/sentry/kernel/fd_table_unsafe.go @@ -121,18 +121,21 @@ func (f *FDTable) setAll(ctx context.Context, fd int32, file *fs.File, fileVFS2 panic("VFS1 and VFS2 files set") } - slice := *(*[]unsafe.Pointer)(atomic.LoadPointer(&f.slice)) + slicePtr := (*[]unsafe.Pointer)(atomic.LoadPointer(&f.slice)) // Grow the table as required. - if last := int32(len(slice)); fd >= last { + if last := int32(len(*slicePtr)); fd >= last { end := fd + 1 if end < 2*last { end = 2 * last } - slice = append(slice, make([]unsafe.Pointer, end-last)...) - atomic.StorePointer(&f.slice, unsafe.Pointer(&slice)) + newSlice := append(*slicePtr, make([]unsafe.Pointer, end-last)...) + slicePtr = &newSlice + atomic.StorePointer(&f.slice, unsafe.Pointer(slicePtr)) } + slice := *slicePtr + var desc *descriptor if file != nil || fileVFS2 != nil { desc = &descriptor{