msdosfs_rename(): more resilence against corruption
ClosedPublic
Actions

Authored by kib on Jan 17 2024, 11:09 PM.

Details

Reviewers

Commits

rG13ccb04589e2: msdosfs_integrity_error(): plug possible busy leak
rG661db9b390b4: msdosfs_rename(): implement several XXXs about downgrading to ro
rGbe0df84849ff: msdosfs_rename(): handle errors from msdosfs_lookup_ino()

Summary

msdosfs_rename(): handle errors from msdosfs_lookup_ino()

Properly working storage and correct filesystem structure indeed only
allow the EJUSTRETURN return code, but since the called function needs
to read directory blocks and (re)parse the content, the assert is not
neccessary hold.

PR:     276408

msdosfs_rename(): implement several XXXs about downgrading to ro

msdosfs_integrity_error(): plug possible busy leak

If taskqueue_enqueue() returned error, unbusy().
Handle parallel calls to msdosfs_integrity_error() by unbusying in
msdosfs_remount_ro() up to pending times.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Jan 17 2024, 11:09 PM

Herald added a subscriber: imp. · View Herald TranscriptJan 17 2024, 11:09 PM

kib requested review of this revision.Jan 17 2024, 11:09 PM

Suppose two threads call msdosfs_integrity_error() at around the same time. This function busies the mountpoint, then schedules a task to downgrade the mountpoint to RO. It's possible that both calls will be handled by a single call to msdosfs_remount_ro(pending = 2). There, the busy reference is released only once, so isn't it possible to leak?

Handle pending taskq

markj accepted this revision.Jan 18 2024, 3:49 PM

markj added inline comments.

sys/fs/msdosfs/msdosfs_vfsops.c
1023	I do not see how an error can arise in practice.

This revision is now accepted and ready to land.Jan 18 2024, 3:49 PM

kib marked an inline comment as done.Jan 18 2024, 4:04 PM

kib added inline comments.

sys/fs/msdosfs/msdosfs_vfsops.c
1023	In principle this is true right now, but for completeness it is better be handled.

Fix loop around vfs_unbusy():

-       } while (pending-- >= 0);
+       } while (--pending >= 0);

This revision now requires review to proceed.Jan 18 2024, 4:06 PM

markj accepted this revision.Jan 18 2024, 4:12 PM

This revision is now accepted and ready to land.Jan 18 2024, 4:12 PM

Closed by commit rGbe0df84849ff: msdosfs_rename(): handle errors from msdosfs_lookup_ino() (authored by kib). · Explain WhyJan 18 2024, 4:49 PM

This revision was automatically updated to reflect the committed changes.

kib added a commit: rGbe0df84849ff: msdosfs_rename(): handle errors from msdosfs_lookup_ino().

kib added a commit: rG661db9b390b4: msdosfs_rename(): implement several XXXs about downgrading to ro.

kib added a commit: rG13ccb04589e2: msdosfs_integrity_error(): plug possible busy leak.

Revision Contents
Changeset List

Path

Size

sys/

fs/

msdosfs/

msdosfs_vfsops.c

18 lines

msdosfs_vnops.c

19 lines

Diff 132931

View Options

sys/fs/msdosfs/msdosfs_vfsops.c