On Tue, Sep 14, 2021 at 12:11:34PM -0700, Boris Burkov wrote: > > > > The mode 0 is the one I like the least, as it makes some ad-hoc changes like > > making the fs-verity ioctls fail with -EOPNOTSUPP. If userspace doesn't want to > > use those ioctls, shouldn't it just not use those ioctls? > > > > It might help if you elaborated on what sort of problems you are trying to plan > > for. One concern that was raised on Android was that on low-end flash storage, > > files can have bit-flips that would normally be "benign" but would cause errors > > if fs-verity detects them. Falling back to your mode 1 (logging-only) would be > > sufficient if that happened and caused problems. So I am wondering more what > > the purpose of mode 0 would be; it seems it might be overkill, and an > > "enforcing" boolean equivalent to your modes 1 and 2 might be sufficient? > > In our situation, I think we are less worried about these sorts of > bit-flips as we already use btrfs checksums and verity would only catch > the cases where the checksum also changed (presumably this is only the > malicious case, in practice) > > Mode 0 is actually probably more interesting to us, as it would be > insurance against the case where there is either a serious bug in the > btrfs implementation or if there is a performance regression on some > unforeseen workload. Without being able to shut it off entirely, we > would be in a tough spot of having to replace the affected files. > > The most important part of this mode to me is the skip and return 0 in > fsverity_verify_page. I agree that failing the enables is sort of lame > because userspace would need to be ignoring errors or falling back to > not-verity for that to even "help". > > Maybe I could make them a no-op? That could be too surprising, but is > in line with verify being a no-op and could actually have useful > semantics in an emergency shutoff scenario. In that case I guess it's reasonable to have all three modes, but they need to have clearly defined semantics and have an intuitive interface, and be documented. Setting "enabled" to 1 to disable something is unintuitive; it probably should be fs.verity.mode with string values, e.g. "enforcing", "log-only" (or "audit"?), and "disabled". For the log-only mode, you also need to consider which types of errors it applies to, specifically. In your patch, it appears that only data verification errors would be log-only, whereas other errors such as bad signatures and fsverity_descriptor corruption would still be fatal. It probably would make sense to have these other errors be log-only as well, so that log-only applies to all fs-verity errors. I don't think the "disabled" mode making the fs-verity ioctls be no-ops is a good idea. I think you should just make them return an error code, preferably a distinct error code rather than overloading EOPNOTSUPP. You can always make userspace aware of whether fs-verity is disabled or not, if needed. Trying to make userspace think that it's using fs-verity when it's actually not isn't going to work well, especially if it's using the FS_IOC_MEASURE_VERITY ioctl, as there is no way to return a meaningful value from that if the prior call to FS_IOC_ENABLE_VERITY was ignored. - Eric
|