summaryrefslogtreecommitdiff
path: root/sys/dev/dkwedge
AgeCommit message (Collapse)Author
2023-05-22dk(4): Add locking notes.riastradh
2023-05-22dk(4): Explain why no need for device reference in dksize, dkdump.riastradh
2023-05-22dk(4): Strengthen preconditions of various devsw operations.riastradh
These can only happen between dkopen and dkclose, so there's no need to test -- we can assert instead that the wedge exists and is fully initialized.
2023-05-22dk(4): Strengthen dkclose preconditions.riastradh
Like dkopen, except it is possible for this to be called after the wedge has transitioned to dying. XXX sc_state read here races with sc_state write in dkwedge_detach. Could change this to atomic_load/store.
2023-05-22dk(4): Strengthen dkopen preconditions.riastradh
This cannot be called before dkwedge_attach for the same unit returns, so sc->sc_dev is guaranteed to be set to a nonnull device_t and the state is guaranteed not to be larval. And this cannot be called concurrently with dkwedge_detach, or after dkwedge_detach does vdevgone until another wedge with the same number is attached (which can't happen until dkwedge_detach completes), so the state is guaranteed not to be dying or dead. Hence sc->sc_dev != NULL and sc->sc_state == DKW_STATE_RUNNING.
2023-05-22dk(4): Prevent race between dkwedge_get_parent_name and wedge detach.riastradh
Still races with parent detach but maybe this is better. XXX Maybe we should ditch dkwedge_get_parent_name -- it's used only by rf_containsboot, which kinda suggests it shouldn't exist.
2023-05-22dk(4): Split unsafe lookups into safe subroutines and unsafe wrappers.riastradh
No functional change intended. Eventually we should adjust the callers to use the safe subroutines instead and device_release when done.
2023-05-22dk(4): Don't hold lock around uiomove in dkwedge_list.riastradh
Instead, hold a device reference. dkwedge_detach will not run until the device reference is released.
2023-05-22dk(4): Skip larval wedges in various lookup routines.riastradh
These have not yet finished a concurent dkwedge_attach, so there's nothing we can safely do with them. Just pretend they don't exist -- as if we had arrived at the lookup a moment earlier.
2023-05-22dk(4): Simplify dkwedge_delall by detaching directly.riastradh
No need for O(n^2) algorithm and potentially racy lookups -- not that n is large enough for n^2 to matter, but the mechanism is simpler this way.
2023-05-22dk(4): Use device_lookup_private for dkwedge_lookup.riastradh
No longer necessary to go through the dkwedges array. Currently device_lookup_private still involves touching other global locks, but that will change eventually to a lockless pserialized fast path.
2023-05-22dk(4): dkunit is no longer needed; nix it.riastradh
dkwedges array indexing now coincides with autoconf device numbering.
2023-05-22dk(4): Use config_attach_pseudo_acquire to create wedges.riastradh
This way, indexing of the dkwedges array coincides with numbering of autoconf dk(4) instances. As a side effect, this plugs a race in dkwedge_add with concurrent drvctl -r. There are a lot of such races in dk(4) left -- to be addressed with more device references.
2023-05-13dk(4): Need pdk->dk_openlock to read pdk->dk_wedges.riastradh
2023-05-10dk(4): Make it clearer that dkopen EROFS branch doesn't leak.riastradh
It looked like we may need to sometimes call dklastclose in error branch for the case of (flags & ~sc->sc_mode & FWRITE) != 0, but it is not actually possible to reach that case: if the caller requested read/write, and the parent is read-only, and it is the first time we've opened the parent, then dkfirstopen will fail with EROFS so we never get there. But this is confusing and it looked like the error branch is wrong, so let's rearrange the conditional to make it clearer that we cannot goto out after dkfirstopen has succeeded. And then assert that the case cannot happen when we do call dkfirstopen.
2023-05-09dk(4): Fix typo: sc_state, not sc_satte.riastradh
Had tested a patch series, but not every patch in it, and I inadvertently fixed the typo in a later patch in the series, not in the one I committed.
2023-05-09dk(4): Omit needless sc_iopend, sc_dkdrn mechanism.riastradh
vdevgone guarantees that all instances are closed by the time it returns, which in turn guarantees all I/O operations (read, write, ioctl, &c.) have completed, and, if the block device is open, vinvalbuf(V_SAVE) -> vflushbuf has completed, which forces all buffered transfers to be issued and waits for them to complete. So by the time vdevgone returns, no further transfers can be submitted and the bufq must be empty.
2023-05-09ioctl(DIOCRMWEDGES): Delete only idle wedges.riastradh
Don't forcibly delete busy wedges. Reported-by: syzbot+e46f31fe56e04f567d88@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=8a00fd7f2e7459748d7a274098180a4708ff0f61 Fixes accidental destruction of the busy wedge that the root file system is mounted on, triggered by syzbot's ioctl(DIOCRMWEDGES).
2023-05-09dk(4): dkclose must handle a dying wedge too to close the parent.riastradh
Otherwise the parent open leaks on detach (or revoke) when the wedge was open and had to be forcibly closed. Reported-by: syzbot+e46f31fe56e04f567d88@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=8a00fd7f2e7459748d7a274098180a4708ff0f61 Fixes assertion sc->sc_dk.dk_openmask == 0.
2023-04-29dk(4): Rename label for consistency. No functional change intended.riastradh
2023-04-29dk(4): Fix lock assertion in size increase: parent's, not wedge's.riastradh
Reported-by: syzbot+d4dc610473cacc5183dd@syzkaller.appspotmail.com https://syzkaller.appspot.com/bug?id=e18ddae8283d6fab44cfb1ac7e3f8e791f8c0700
2023-04-22dk(4): Convert tests to assertions in various devsw operations.riastradh
.d_cancel, .d_strategy, .d_read, .d_write, .d_ioctl, and .d_discard are only ever used between successful .d_open return and entry to .d_close. .d_open doesn't return until sc is nonnull and sc_state is RUNNING, and dkwedge_detach waits for the last .d_close before setting sc_state to DEAD. So there is no possibility for sc to be null or for sc_state to be anything other than RUNNING or DYING. There is a small functional change here but only in the event of a race: in the short window between when dkwedge_detach is entered, and when .d_close runs, any I/O operations (read, write, ioctl, &c.) may be issued that would have failed with ENXIO before. This shouldn't matter for anything: disk I/O operations are supposed to complete reasonably promptly, and these operations _could_ have begun milliseconds prior, before dkwedge_detach was entered, so it's not a significant distinction. Notes: - .d_open must still contend with trying to open a nonexistent wedge, of course. - .d_close must also contend with closing a nonexistent wedge, in case there were two calls to open in quick succession and the first failed while the second hadn't yet determined it would fail. - .d_size and .d_dump are used from ddb without any open/close.
2023-04-22dk(4): Fix racy access to sc->sc_dk.dk_openmask in dkwedge_delall1.riastradh
Need sc->sc_parent->dk_rawlock for this, as used in dkopen/dkclose.
2023-04-21dk(4): Narrow the scope of the device numbering lookup on detach.riastradh
Just need it for vdevgone, order relative to other things in detach doesn't matter. No functional change intended.
2023-04-21dk(4): dkdump: Simplify. No functional change intended.riastradh
2023-04-21dk(4): Omit needless locking in dksize, dkdump.riastradh
All the members these use are stable after initialization, except for the wedge size, which dkwedge_size safely reads a snapshot of without locking in the caller.
2023-04-21dk(4): Take a read-lock on dkwedges_lock if we're only reading.riastradh
- dkwedge_find_by_name - dkwedge_find_by_parent - dkwedge_print_wnames
2023-04-21dk(4): Set .d_cfdriver and .d_devtounit to plug open/detach race.riastradh
This way, opening dkN or rdkN will wait if attach or detach is still in progress, and vdevgone will wake up such pending opens and make them fail. So it is no longer possible for a wedge to be detached after dkopen has already started using it. For now, we use a custom .d_devtounit function that looks up the autoconf unit number via the dkwedges array, which conceivably may use an independent unit numbering system -- nothing guarantees they match up. (In practice they will mostly match up, but concurrent wedge creation could lead to different numbering.) Eventually this should be changed so the two numbering systems match, which would let us delete the new dkunit function and just use dev_minor_unit like many other drivers can.
2023-04-21dk(4): Use disk_begindetach and rely on vdevgone to close instances.riastradh
The first step is to decide whether we can detach (if forced, yes; if not forced, only if not already open), and prevent new opens if so. There's no need to start closing open instances at this point -- we're just making a decision to detach, and preventing new opens by transitioning state that dkopen will respect[*]. The second step is to force all open instances to close. This is done by vdevgone. By the time vdevgone returns, there can be no open instances, so if there _were_ any, closing them via vdevgone will have passed through dklastclose. After that point, there can be no opens and no I/O operations, so dk_openmask must already be zero and the bufq must be empty. Thus, there's no need to have an explicit call to dklastclose (via dkwedge_cleanup_parent) before or after making the decision to detach. [*] Currently access to this state is racy: nothing serializes dkwedge_detach's state transition with dkopen's test. TBD in a separate commit shortly.
2023-04-21dk(4): Fix callout detach race.riastradh
1. Set a flag sc_iostop under the lock sc_iolock so dkwedge_detach and dkstart don't race over it. 2. Decline to schedule the callout if sc_iostop is set. The callout is already only ever scheduled while the lock is held. 3. Use callout_halt to wait for any concurrent callout to complete. At this point, it can't reschedule itself. Without this change, the callout could be concurrently rescheduling itself as we issue callout_stop, leading to use-after-free later.
2023-04-21dk(4): Add null d_cancel routine to devsw.riastradh
This way, dkclose is guaranteed that dkopen, dkread, dkwrite, dkioctl, &c., have all returned before it runs. For block opens, setting d_cancel also guarantees that any buffered writes are flushed with vinvalbuf before dkclose is called.
2023-04-21dk(4): Require dk_openlock in dk_set_geometry.riastradh
Not strictly necessary but this makes reasoning easier and documents with an assertion how disk_set_info is serialized.
2023-04-21dk(4): Assert dkwedges[unit] is the sc we're about to free.riastradh
2023-04-21dk(4): Assert parent vp is nonnull before we stash it away.riastradh
Let's enable early attribution if this goes wrong. If it's not the parent's first open, also assert the parent vp is already nonnull.
2023-04-21dk(4): Don't touch dkwedges or ndkwedges outside dkwedges_lock.riastradh
2023-04-21dk(4): Move CFDRIVER_DECL and CFATTACH_DECL3_NEW earlier in file.riastradh
Follows the pattern of most drivers, and will be necessary for referencing dk_cd in dk_bdevsw and dk_cdevsw soon, to prevent open/detach races. No functional change intended.
2023-04-21dk(4): Prevent races in access to struct dkwedge_softc::sc_size.riastradh
Rules: 1. Only ever increases, never decreases. (Decreases require removing and readding the wedge.) 2. Increases are serialized by dk_openlock. 3. Reads can happen unlocked in any context where the softc is valid. Access is gathered into dkwedge_size* subroutines -- don't touch sc_size outside these. For now, we use rwlock(9) to keep the reasoning simple. This should be done with atomics on 64-bit platforms and a seqlock on 32-bit platforms to avoid contention. However, we can do that in a later change.
2023-04-21dk(4): <sys/rwlock.h> for rwlock(9).riastradh
2023-04-21dk(4): KNF: Sort includes.riastradh
No functional change intended.
2023-04-21dk(4): ENXIO, not ENODEV, means no such device.riastradh
ENXIO is `device not configured', meaning there is no such device. ENODEV is `operation not supported by device', meaning the device is there but refuses the operation, like writing to a read-only medium. Exception: For undefined ioctl commands, it's not ENODEV _or_ ENXIO, but rather ENOTTY, because why make any of this obvious when you could make it obscure Unix lore?
2023-04-21dk(4): Fix typo in comment: dkstrategy, not dkstragegy.riastradh
No functional change intended.
2023-04-21dk(4): Omit needless void * cast.riastradh
No functional change intended.
2023-04-21dk(4): KNF: Whitespace.riastradh
No functional change intended.
2023-04-21dk(4): KNF: return (v) -> return v.riastradh
No functional change intended.
2023-04-21dk(4): Avoid holding dkwedges_lock while allocating array.riastradh
This is not great -- we shouldn't be choosing the unit number here anyway; we should just let autoconf do it for us -- but it's better than potentially blocking any dk_openlock or dk_rawlock (which are sometimes held when waiting for dkwedges_lock) for memory allocation.
2023-04-21dk(4): Restore assertions in dklastclose.riastradh
We only enter dklastclose if the wedge is open (sc->sc_dk.dk_openmask != 0), which can happen only if dkfirstopen has succeeded, in which case we hold a dk_rawopens reference to the parent that prevents anyone else from closing it. Hence sc->sc_parent->dk_rawopens > 0. On open, sc->sc_parent->dk_rawvp is set to nonnull, and it is only reset to null on close. Hence if the parent is still open, as it must be here, sc->sc_parent->dk_rawvp must be nonnull.
2023-04-13dk(4): Explain why dk_rawopens can't overflow and assert it.riastradh
2022-09-27Remove bogus assertions.mlelstv
2022-08-22dk(4): Assert about dk_openmask under the lock.riastradh
This serves two purposes: 1. Pacifies data race sanitizers. 2. Ensures that we don't spuriously trip over the assertion if dkclose happens concurrently with dkopen due to a revoke call.
2022-08-22Revert "dk(4): Narrow scope of dk_rawlock on close to dklastclose."riastradh
dkfirstopen relies on reading from dk_openmask of _other_ wedges, writes to dk_openmask must be serialized by dk_rawlock in addition to dk_openlock. (However, reads from dk_openlock only require one or the other).