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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20200321131715.GH20696@hirez.programming.kicks-ass.net>
Date:   Sat, 21 Mar 2020 14:17:15 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        mhiramat@...nel.org, bristot@...hat.com, jbaron@...mai.com,
        torvalds@...ux-foundation.org, tglx@...utronix.de,
        mingo@...nel.org, namit@...are.com, hpa@...or.com, luto@...nel.org,
        ard.biesheuvel@...aro.org, jpoimboe@...hat.com,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v3 01/17] notifier: Fix broken error handling pattern

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.