| Message ID | 20230320205617.GA1434@sol.localdomain (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [GIT,PULL] fscrypt fix for v6.3-rc4 | expand |
On Mon, Mar 20, 2023 at 1:56 PM Eric Biggers <ebiggers@kernel.org> wrote: > > fscrypt: check for NULL keyring in fscrypt_put_master_key_activeref() Side note: please just use WARN_ON_ONCE() for things like this, not WARN_ON. If it's triggerable, it should be triggered only once rather than flood the logs and possibly cause a DoS. And if it's not triggerable, the "ONCE" doesn't matter. I note that fscypt in general seems to be *way* too happy with WARN_ON() as some kind of debugging aid. It's not good in general (printf for debugging is wonderful, but shouldn't be left in the sources to rot for all eternity), but it's particularly not good in that form. Linus
The pull request you sent on Mon, 20 Mar 2023 13:56:17 -0700:
> https://git.kernel.org/pub/scm/fs/fscrypt/linux.git tags/fscrypt-for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4f1e308df88ad25c88ab4240161cbac45ba2d78e
Thank you!
On Mon, Mar 20, 2023 at 03:16:48PM -0700, Linus Torvalds wrote: > On Mon, Mar 20, 2023 at 1:56 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > fscrypt: check for NULL keyring in fscrypt_put_master_key_activeref() > > Side note: please just use WARN_ON_ONCE() for things like this, not WARN_ON. > > If it's triggerable, it should be triggered only once rather than > flood the logs and possibly cause a DoS. > > And if it's not triggerable, the "ONCE" doesn't matter. > > I note that fscypt in general seems to be *way* too happy with > WARN_ON() as some kind of debugging aid. > > It's not good in general (printf for debugging is wonderful, but > shouldn't be left in the sources to rot for all eternity), but it's > particularly not good in that form. > Yes, I agree that most of the WARN_ONs should be WARN_ON_ONCEs. I think I've been assuming that WARN_ON is significantly more lightweight than WARN_ON_ONCE. But that doesn't seem to be the case, especially since commit 19d436268dde ("debug: Add _ONCE() logic to report_bug()"). But besides that, I believe WARN* is generally being used appropriately in fs/crypto/. It's used when assumptions made by the code are violated, but where the hard crash of a BUG() is not necessary. I think this is a good thing to have, versus the alternative of doing nothing and making it much harder to track down bugs... Some particularly bad crypto bugs that we can easily WARN about, such as IVs being truncated, may not even be detectable by users otherwise. There are probably a few that should be removed, though. I'm also considering whether the refcounting-related ones should use CHECK_DATA_CORRUPTION, though that may run afoul of the "don't use BUG() unless absolutely needed" rule... - Eric
On Mon, Mar 20, 2023 at 03:59:34PM -0700, Eric Biggers wrote: > > Yes, I agree that most of the WARN_ONs should be WARN_ON_ONCEs. I think I've > been assuming that WARN_ON is significantly more lightweight than WARN_ON_ONCE. > But that doesn't seem to be the case, especially since commit 19d436268dde > ("debug: Add _ONCE() logic to report_bug()"). Another option is WARN_RATELIMITED. As an unrelated side-note, one of the things I've been working on in some of the ext4 code paths when I've been moving BUG_ON's to WARN_RATELIMITED is to think about what might be needed to debug a problem, and sometimes it can be helpful to use a printf string to provide more context than just a WARN_ON. Cheers, - Ted
On Mon, Mar 20, 2023 at 7:03 PM Theodore Ts'o <tytso@mit.edu> wrote: > > Another option is WARN_RATELIMITED. I don't think that exists. There's 'pr_warn_ratelimited()', but honestly, the rate limiting is a joke. It's fine for things that never happen, but if you can flood things without the rate limiting, you can still flood things with the rate limiting. The default rate limiting is "max five reports every five seconds". For some "this should never happen", a reasonable rate limit might be "once every 24 hours" or something like that. Just make sure that if the machine stays up for months or years at a time, it doesn't get hidden in all the *other* noise. Our rate limiting sucks. The only thing that really saves it is that rate limiting is used for things that never happen in the first place, and the default values are basically picked for "this is a network DoS attempt, let's make sure it stands out in the logs without completely bogging down the machine". So no. Please don't use "ratelimited" for "this shouldn't happen". It's still going to suck. We had that *exact* thing just a couple of weeks ago: https://lore.kernel.org/all/CAHk-=wjTMgB0=PQt8synf1MRTfetVXAWWLOibnMKvv1ETn_1uw@mail.gmail.com/ where the networking people thought that ratelimiting would be a good idea. It's not a good idea. Linus
The following changes since commit fe15c26ee26efa11741a7b632e9f23b01aca4cc6: Linux 6.3-rc1 (2023-03-05 14:52:03 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/fs/fscrypt/linux.git tags/fscrypt-for-linus for you to fetch changes up to 4bcf6f827a79c59806c695dc280e763c5b6a6813: fscrypt: check for NULL keyring in fscrypt_put_master_key_activeref() (2023-03-18 21:08:03 -0700) ---------------------------------------------------------------- Fix a bug where when a filesystem was being unmounted, the fscrypt keyring was destroyed before inodes have been released by the Landlock LSM. This bug was found by syzbot. ---------------------------------------------------------------- Eric Biggers (3): fscrypt: destroy keyring after security_sb_delete() fscrypt: improve fscrypt_destroy_keyring() documentation fscrypt: check for NULL keyring in fscrypt_put_master_key_activeref() fs/crypto/keyring.c | 23 +++++++++++++---------- fs/super.c | 15 ++++++++++++--- 2 files changed, 25 insertions(+), 13 deletions(-)