[GIT,PULL] fscrypt fix for v6.3-rc4

  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(-)
  

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