Re: [PATCH v3 01/17] notifier: Fix broken error handling pattern

[Peter Zijlstra <peterz@infradead.org>]

On Sat, Mar 21, 2020 at 01:24:19PM +0100, Borislav Petkov wrote:
> On Fri, Mar 20, 2020 at 10:38:45PM +0100, Peter Zijlstra wrote:
> > The current notifiers have the following error handling pattern all
> > over the place:
> > 
> > 	int err, nr;
> > 
> > 	err = __foo_notifier_call_chain(&chain, val_up, v, -1, &nr);
> > 	if (err & NOTIFIER_STOP_MASK)
> > 		__foo_notifier_call_chain(&chain, val_down, v, nr-1, NULL)
> > 
> > And aside from the endless repetition thereof, it is broken. Consider
> > blocking notifiers; both calls take and drop the rwsem, this means
> > that the notifier list can change in between the two calls, making @nr
> > meaningless.
> > 
> > Fix this by replacing all the __foo_notifier_call_chain() functions
> > with foo_notifier_call_chain_robust() that embeds the above pattern,
> > but ensures it is inside a single lock region.
> 
> "robust" huh? Sure reads funny :)

This has been around the bike-shed a few times already.

> So if the "normal" notifier_call_chain() usage is buggy, how about we
> prepend its name with "__" and call the new one with the rollback,
> notifier_call_chain() ?
> 
> Instead of adding the "robust" set of interfaces?

Well, it depends on the usecase. The robust one can deal with failure,
the other ones are fine (and preferred) if failure is not an option.

This robust variant ensures that all the notifiers that succeeded prior
to the one that failed get a second callback with another state. Some
notifier chains don't care, but a few clearly did and did it utterly
broken.

> Btw, the indentation in that notifier.* files is yuck.

Yeha, wasn't going to fix that.