In response to @jnewbery's comment

I think I'd prefer log-shedding to be done globally, with a special log at the end of a log suppression period that tallies up which log locations have been suppressed.

I disagree on the global schedding, i think the source location based schedding is over all the better approach.
I agree with your point that it could be confusing to drop some logs in the context of user issues but schedding should only kick in if there is an attack or something is horribly wrong, so i would think that almost all logs provided by users would be complete. We can also always tell if rate limiting is taking place since we log when it starts/stops and in those cases a partial log is better then no log.

(This is obviously mostly an opinion and I am totally willing to be convinced of global log-schedding if enough reviewers favor it)

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25306 (logging: add LogPrintfCategory to log unconditionally with category by jonatack)
• #25287 (logging: threshold log level by klementtan)
• #25203 (logging: update to severity-based logging by jonatack)
• #24950 (Add config option to set max debug log size by tehelsper)
• #23443 (p2p: Erlay support signaling by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

schedding should only kick in if there is an attack or something is horribly wrong, so i would think that almost all logs provided by users would be complete.

Right, and those cases are exactly where we need the best possible logs.

We can also always tell if rate limiting is taking place since we log when it starts/stops and in those cases a partial log is better then no log.

I guarantee you that not everyone will know this and most people will miss a single log line saying "rate-limiting log x".

I can only speak from my own experience of supporting network equipment for several years - partial logs are often worse than no logs at all since they're so misleading.

Strongest possible Concept ACK: this mitigation will kill an entire bug class (the "disk fill via logging" bug class).

Thanks for taking up this work @dergoegge.

Will test and review.

@jnewbery

I think I'd prefer log-shedding to be done globally, with a special log at the end of a log suppression period that tallies up which log locations have been suppressed.

Clarifying questions to fully understand what is suggested:

Let's assume that log location A is a misplaced LogPrintf in a code path that the attacker can make us take.

In the case of an attacker using log location A to fill our disk by making us log from there repeatedly, is the suggestion then that all log locations should be suppressed during the suppression period (instead of suppressing only the "attacker controlled" log location A)?

To make your suggestion clear: could you exemplify what the special log entry at the end of the log suppression period would look like? (A patch or even a separate PR would be even better, but an example log entry would probably clarify enough!)

@dergoegge

Thanks for your work on this PR!

Would you be willing to implement also @jnewbery's suggestions as a separate PR?

I tried to summarise his suggestion in #21603 (comment). I'm not certain I got it right though - @jnewbery, feel free to chime in :)

I think it makes sense to do it as separate PR since it deviates in important ways from the original suggestion. By having two separate PRs we would see which of the two approaches to address this type of attack that have consensus support.

Personally I think I could live with both approaches: as long as we kill the disk-fill-via-logging bug class sooner rather than later I'm happy :)

Would you be willing to implement also jnewbery's suggestions as a separate PR?

@practicalswift Sure, thats a good idea :)

concept ACK (not that I'm sure if my opinion is relevant) and I think this makes more sense than the global alternative.

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

@dergoegge Thanks for working on this. Would you mind rebasing? I would like to review the updated version :)

It looks like many of the LogPrintf messages will be going away with the update to severity-based logging and the added complexity here may be worth reevaluating (and potentially avoiding) after that.

@jonatack IMO removing some of the LogPrintf locations (or assigning them a severity level which is not logged by default) does not really address the issue at its root like this PR is trying to do. Any remaining default logging could still cause issues as well as any logging that is introduced in the future.

What I am working on is that no logging on by default would be externally provokable (and potentially removing LogPrintf). That along with reducing default logging in general has appeared to be the desired direction for some time now AFAICS. Of course, this could still be merged in the interim and then removed if no longer needed, but I'm targeting the next release.

reACK c86d5ff

What I am working on is that no logging on by default would be externally provokable (and potentially removing LogPrintf). That along with reducing default logging in general has appeared to be the desired direction for some time now AFAICS. Of course, this could still be merged in the interim and then removed if no longer needed, but I'm targeting the next release.

With bd971bf, category-based logging with a severity >= BCLog::Level::Warning is equivalent to LogPrintf from the viewpoint of disk filling attacks.
I think that it would be really good to have this general countermeasure in place, especially with probable future PRs that convert more existing log messages to the new format, because we'll now have to be careful about not adding category-based warnings or errors that could be triggered externally. The old rule, which is no longer valid but probably still in many contributors' heads was that one only needs to be careful about LogPrintf, while category-base logging is not so critical.

convert more existing log messages to the new format, because we'll now have to be careful about not adding category-based warnings or errors that could be triggered externally

Yes, doing precisely this right now for all of them. Though, assuming people are in favor of that, maybe reviewers might prefer extending this rate limiting to non-default logging or otherwise it can always still be removed.

Is it possible to convert all of them? I think currently we don't have a way to express a log category but unconditional log severity. Unless you want to use Warning/Error for "unconditional".

@MarcoFalke added that in #25203.

Though, assuming people are in favor of that, maybe reviewers might prefer extending this rate limiting to non-default logging.

Good point, I think that this would definitely be useful (here or as a follow-up) - having a rate limit only for LogPrintf but not for unconditionally logged category-based warnings and errors would be confusing.

Good point, I think that this would definitely be useful (here or as a follow-up) - having a rate limit only for LogPrintf but not for unconditionally logged category-based warnings and errors would be confusing.

I agree; though as an example just updated the (copious) net processing logging in #25203 and there are a only a few remaining unconditional messages (user-controlled or unusual):

src/net_processing.cpp:2305:                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Large reorg, won't direct fetch to %s (%d)\n",
src/net_processing.cpp:2702:            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Connected to self at %s, disconnecting\n", pfrom.addr.ToString());
src/net_processing.cpp:3425:                    LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId());
src/net_processing.cpp:4190:        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Not punishing misbehaving peer %d because it has noban permission set!\n", peer.m_id);
src/net_processing.cpp:4196:        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Not punishing misbehaving peer %d because it was manually connected!\n", peer.m_id);
src/net_processing.cpp:4873:                        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Announcing block %s not on main chain (tip=%s)\n",

having a rate limit only for LogPrintf but not for unconditionally logged category-based warnings and errors would be confusing.

Also agree with this and i want to add that to this PR.

The two new categories I introduce here (UNCONDITIONAL_ALWAYS and UNCONDITIONAL_RATE_LIMITED) don't really make sense anymore, given that we now also have category based unconditional logging. Whether a log location should be rate limited could purely be based on the severity level.
I was thinking of the following: Adding an extra severity Always which is logged unconditionally without rate limiting (e.g. for this location) and rate limiting logging to disk if severity == None || (severity >= Warning && severity < Always). Additionally, categories specified by -debug=cat should still be logged unconditionally without rate limiting. @mzumsande @jonatack @MarcoFalke what do y'all think of that?

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

I was thinking of the following: Adding an extra severity Always which is logged unconditionally without rate limiting (e.g. for this location) and rate limiting logging to disk if severity == None || (severity >= Warning && severity < Always). Additionally, categories specified by -debug=cat should still be logged unconditionally without rate limiting. @mzumsande @jonatack @MarcoFalke what do y'all think of that?

So, it seems to me that with the recent changes, we currently have

1. Unconditional logging with no level and no category (LogPrintf)
2. Conditional logging with category but no level (LogPrint) - not relevant to this PR
3. Unconditional logging with category but no level (LogPrintfCategory)
4. Logging with category and level, which may be unconditional depending on the level (LogPrintLevel)

The "missing" combination of logging by level but without a category is currently not possible as far as I can see.

If there would be a use case for having "Always" logged messages also to have a severity like "Warning" or "Error" we'd need another dimension - but I think that at least for the current uses this is not the case, so your suggestion makes sense to me.

However, I'm really not sure how stable the current state of the changes to the logging framework is, it seems very much in flow and I'm a little bit confused about what direction it is heading:

• The severity threshold mentioned in 4) was added in log: Use severity-based logging for leveldb/libevent messages, reverse LogPrintLevel order #25202, but after reading this comment by @laanwj I'm not sure it will stay.
• Is the final goal for each logging message to have both a category and a level so that 1) and 2) and 3) would vanish over time? That would probably reduce the complexity of the rate-limiting considerably.

If there is a log category with Info, which is also the default level, you wouldn't need an Always level?

If there is a log category with Info, which is also the default level, you wouldn't need an Always level?

I don't see how - the "Always" level is meant for noisy unconditional logs that can hit the rate limit, but are so important that they should be exempt from it - basically only UpdateTipLog during IBD.

I'm about to push an update to #25203 to incorporate #25287 and the changes discussed in #25306, and the current approach is that the None severity level is only used internally, i.e. in src/logging.{h.cpp}, and moved to the end of the enum to permit iterating through it similar to GetNetworkNames() / NET_MAX.

The logging you might be worried about limiting would be, with the current direction, IIRC:

• LogPrintf
• LogPrintfWithCategory
• LogPrintLevel with level Info/Warning/Error (these levels are planned to be logged unconditionally, while Debug and Trace would not)

Haven't had the time to maintain this. I still think some general mitigation for disk-filling would be nice, so I might pick this up again later unless someone else beats me to it. Closing for now.