linux-kernel - Re: [PATCH V2] notifier/panic: Introduce panic_notifier

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e416d20-13f2-25ea-6ff7-b8c0c1818eac@igalia.com>
Date:   Thu, 6 Jan 2022 18:05:09 -0300
From:   "Guilherme G. Piccoli" <gpiccoli@...lia.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
        dyoung@...hat.com, linux-doc@...r.kernel.org, bhe@...hat.com,
        vgoyal@...hat.com, akpm@...ux-foundation.org,
        andriy.shevchenko@...ux.intel.com, corbet@....net,
        halves@...onical.com, kernel@...ccoli.net
Subject: Re: [PATCH V2] notifier/panic: Introduce panic_notifier_filter

On 06/01/2022 17:25, Alan Stern wrote:
> [...]
>> +	if (unlikely(panic_nf_count) && nh == &panic_notifier_list)
>> +		if (!is_panic_notifier_filtered(n))
>> +			goto panic_filtered_out;
> 
> Forget the unlikely(); this is not a hot path.
> 

Thanks for the review Alan! Sure, I can remove that in the V3 - good point.


>> +
>>  	ret = notifier_chain_register(&nh->head, n);
>> +
>> +panic_filtered_out:
>>  	spin_unlock_irqrestore(&nh->lock, flags);
>>  	return ret;
>>  }
> 
> It would be simpler to do:
> 
> 	if (!(nh == &panic_notifier_list && panic_nf_count > 0 &&
> 			is_panic_notifier_filtered(n)))
> 		ret = notifier_chain_register(&nh->head, n);
> 
> If there were special-purpose functions just for registering and 
> unregistering callbacks on the panic_notifier_list, the design would be 
> cleaner (no need to modify core notifier code).  But making that change 
> would mean altering a lot of call sites.  :-(
> 

Good suggestion, I'll change the code with your suggestion in the V3.

And I agree, I considered something like that when writing this "hijack"
into core code just for the sake of panic notifier filtering...
I'm not sure though it worth the effort and code addition, to change all
the call sites and add a special register mechanism to the panic
notifier only. Let's see what the others think...

Cheers,


Guilherme


>> @@ -162,10 +194,16 @@ int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
>>  		struct notifier_block *n)
>>  {
>>  	unsigned long flags;
>> -	int ret;
>> +	int ret = 0;
>>  
>>  	spin_lock_irqsave(&nh->lock, flags);
>> +	if (unlikely(panic_nf_count) && nh == &panic_notifier_list)
>> +		if (!is_panic_notifier_filtered(n))
>> +			goto panic_filtered_out;
>> +
>>  	ret = notifier_chain_unregister(&nh->head, n);
>> +
>> +panic_filtered_out:
> 
> Same idea here.
> 
> Alan Stern
>