Skip to content

Commit aff4c90

Browse files
ayushr2gvisor-bot
authored andcommitted
Fix directfs restore for deleted regular file when read handle is not available
In such a situation, we can not rely on traditional methods (like openHandle()) to open a readable handle. This is because, for directfs to open a handle, it needs to re-walk the file via the parent using openat(parentFD, name). This does not work for deleted files, it will fail with ENOENT. runsc gofer works around this by using /proc/self/fd/ to re-open the control FD in the desired mode. However, the sentry does not have access to any procfs instance (for security). See fcbc289 ("runsc: umount /proc in the sandbox namespace"). This change makes directfs just use the control FD to fetch file data. The control FD should not be used for IO. We make an exception here for S/R. Fixes #11903 PiperOrigin-RevId: 782379453
1 parent de480b5 commit aff4c90

File tree

5 files changed

+71
-20
lines changed

5 files changed

+71
-20
lines changed

pkg/sentry/fsimpl/gofer/dentry_impl.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,6 +589,38 @@ func (d *dentry) restoreFile(ctx context.Context, opts *vfs.CompleteRestoreOptio
589589
}
590590
}
591591

592+
// Precondition: d.handleMu is read locked.
593+
func (d *dentry) readHandleForDeleted(ctx context.Context) (handle, error) {
594+
if d.isReadHandleOk() {
595+
return d.readHandle(), nil
596+
}
597+
switch dt := d.impl.(type) {
598+
case *lisafsDentry:
599+
// ensureSharedHandle locks handleMu for write. Unlock it temporarily.
600+
d.handleMu.RUnlock()
601+
err := d.ensureSharedHandle(ctx, true /* read */, false /* write */, false /* trunc */)
602+
d.handleMu.RLock()
603+
if err != nil {
604+
return handle{}, fmt.Errorf("failed to open read handle: %w", err)
605+
}
606+
return d.readHandle(), nil
607+
case *directfsDentry:
608+
// The sentry does not have access to any procfs mount which it could use
609+
// to re-open dt.controlFD with a different mode (via /proc/self/fd/). The
610+
// file is unlinked, so we can't use openat(parent.controlFD, name) either.
611+
// dt.controlFD must be a read-only FD (see tryOpen() documentation). Just
612+
// seek the control FD to 0 and return it. The control FD is not used for
613+
// reading by the sentry, so this should be safe.
614+
// TODO(b/431481259): Use dentry.ensureSharedHandle() here as well.
615+
if _, err := unix.Seek(dt.controlFD, 0, unix.SEEK_SET); err != nil {
616+
return handle{}, fmt.Errorf("failed to seek control FD to 0: %w", err)
617+
}
618+
return handle{fd: int32(dt.controlFD)}, nil
619+
default:
620+
panic("unknown dentry implementation")
621+
}
622+
}
623+
592624
// doRevalidation calls into r.start's dentry implementation to perform
593625
// revalidation on all the dentries contained in r.
594626
//

pkg/sentry/fsimpl/gofer/directfs_dentry.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,7 @@ func (d *directfsDentry) openHandle(ctx context.Context, flags uint32) (handle,
169169

170170
// The only way to re-open an FD with different flags is via procfs or
171171
// openat(2) from the parent. Procfs does not exist here. So use parent.
172+
// TODO(b/431481259): This does not work for deleted files.
172173
flags |= hostOpenFlags
173174
openFD, err := unix.Openat(parent.impl.(*directfsDentry).controlFD, d.name, int(flags), 0)
174175
if err != nil {

pkg/sentry/fsimpl/gofer/gofer.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1971,20 +1971,11 @@ func (d *dentry) removeXattr(ctx context.Context, creds *auth.Credentials, name
19711971
func (d *dentry) ensureSharedHandle(ctx context.Context, read, write, trunc bool) error {
19721972
// O_TRUNC unconditionally requires us to obtain a new handle (opened with
19731973
// O_TRUNC).
1974-
if !trunc {
1975-
d.handleMu.RLock()
1976-
canReuseCurHandle := (!read || d.isReadHandleOk()) && (!write || d.isWriteHandleOk())
1977-
d.handleMu.RUnlock()
1978-
if canReuseCurHandle {
1979-
// Current handles are sufficient.
1980-
return nil
1981-
}
1982-
}
1983-
19841974
d.handleMu.Lock()
19851975
needNewHandle := (read && !d.isReadHandleOk()) || (write && !d.isWriteHandleOk()) || trunc
19861976
if !needNewHandle {
19871977
d.handleMu.Unlock()
1978+
// Current handles are sufficient.
19881979
return nil
19891980
}
19901981

pkg/sentry/fsimpl/gofer/save_restore.go

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -152,16 +152,9 @@ func (d *dentry) prepareSaveDeletedRegularFile(ctx context.Context) error {
152152
// Fetch an appropriate handle to read the deleted file.
153153
d.handleMu.RLock()
154154
defer d.handleMu.RUnlock()
155-
var h handle
156-
if d.isReadHandleOk() {
157-
h = d.readHandle()
158-
} else {
159-
var err error
160-
h, err = d.openHandle(ctx, true /* read */, false /* write */, false /* trunc */)
161-
if err != nil {
162-
return fmt.Errorf("failed to open read handle for deleted file %q: %w", genericDebugPathname(d.fs, d), err)
163-
}
164-
defer h.close(ctx)
155+
h, err := d.readHandleForDeleted(ctx)
156+
if err != nil {
157+
return fmt.Errorf("failed to open read handle for deleted file %q: %w", genericDebugPathname(d.fs, d), err)
165158
}
166159
// Read the file data and store it in d.savedDeletedData.
167160
d.dataMu.RLock()

test/syscalls/linux/unlink.cc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,40 @@ TEST(UnlinkTest, UnlinkWithOpenFDs) {
295295
EXPECT_STREQ(buf, kHello);
296296
}
297297

298+
// The primary goal of this test is to ensure that a write-only FD to a deleted
299+
// file is savable.
300+
TEST(UnlinkTest, UnlinkWithOpenFDsWriteOnly) {
301+
// TODO(b/400287667): Enable save/restore for local gofer.
302+
DisableSave ds;
303+
if (IsRunningOnRunsc()) {
304+
ds.reset();
305+
}
306+
307+
// Create a file.
308+
TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
309+
310+
// Open the file with O_WRONLY.
311+
FileDescriptor file_fd =
312+
ASSERT_NO_ERRNO_AND_VALUE(Open(file.path(), O_WRONLY, 0222));
313+
314+
// Unlink the file.
315+
EXPECT_THAT(unlink(file.path().c_str()), SyscallSucceeds());
316+
317+
// Write to the file.
318+
const char kHello[] = "hello";
319+
ASSERT_THAT(write(file_fd.get(), kHello, sizeof(kHello)),
320+
SyscallSucceedsWithValue(sizeof(kHello)));
321+
322+
// TODO(b/400287667): When running with local gofer AND cache policy = none,
323+
// stat-ing a deleted file returns ENOENT.
324+
if (IsRunningOnRunsc()) {
325+
// Stat the file to verify its size.
326+
struct stat st;
327+
ASSERT_THAT(fstat(file_fd.get(), &st), SyscallSucceeds());
328+
EXPECT_EQ(st.st_size, sizeof(kHello));
329+
}
330+
}
331+
298332
} // namespace
299333

300334
} // namespace testing

0 commit comments

Comments
 (0)