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

[Eric Biggers <ebiggers@kernel.org>]

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