linux-kernel - RE: [RFC][PATCH v3 2/3] Skip spin_locks in panic case and add WARN

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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 12 Dec 2011 13:32:16 -0500
From:	Seiji Aguchi <seiji.aguchi@....com>
To:	Don Zickus <dzickus@...hat.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Luck, Tony" <tony.luck@...el.com>,
	Matthew Garrett <mjg@...hat.com>,
	Vivek Goyal <vgoyal@...hat.com>,
	"Chen, Gong" <gong.chen@...el.com>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"len.brown@...el.com" <len.brown@...el.com>,
	"'ying.huang@...el.com'" <'ying.huang@...el.com'>,
	"'ak@...ux.intel.com'" <'ak@...ux.intel.com'>,
	"'hughd@...omium.org'" <'hughd@...omium.org'>,
	"'mingo@...e.hu'" <'mingo@...e.hu'>,
	"jmorris@...ei.org" <jmorris@...ei.org>,
	"a.p.zijlstra@...llo.nl" <a.p.zijlstra@...llo.nl>,
	"namhyung@...il.com" <namhyung@...il.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Satoru Moriya <satoru.moriya@....com>
Subject: RE: [RFC][PATCH v3 2/3] Skip spin_locks in panic case and add
 WARN_ON()

Hi,

>> -	if (in_nmi()) {
>> -		is_locked = spin_trylock(&psinfo->buf_lock);
>> -		if (!is_locked)
>> -			pr_err("pstore dump routine blocked in NMI, may corrupt error record\n");
>> -	} else
>> +	/*
>> +	 * pstore_dump() is serialized in panic path.
>> +	 * So, we don't need to take any locks.
>> +	 */
>> +	if (reason != KMSG_DUMP_PANIC)
>>  		spin_lock_irqsave(&psinfo->buf_lock, flags);
>
>This probably won't work because the original check wasn't necessarily for
>locking purposes but to see if a lock was already taken in the serialized
>path.  If buf_lock is already held, the code may have trouble executing
>the backend, hence the pr_err.

If locking of backend drivers ,such as erst_lock/efivars_lock, was held in serialized path,
the backend drivers may have some troubles.

On the other hand, pstore will not have any trouble with my code because psinfo->buf_lock is protecting 
the memory ,psinfo->buf, which backend driver use to save data and the content of psinfo->buf is
simply overwritten in pstore_dump().

So, pstore can just blindly continue even if psinfo->buf_lock is held in the serialized path.

If there are some troubles in backend drivers, they should be fixed.

I guess you suggest me to add some code checking lock status so that users/developers know 
the reason why pstore failed.
So, I have a comment about that.

If you would like to just check if a lock was already in the serialized path,
"is_spin_locked()" macro is better.
In addition to that, if you would like to let users know the locking status, pr_err() is needed. 
So, the code will be as follows.

 if (reason == KMSG_DUMP_PANIC) {
 	if(is_spin_locked(&psinfo->buf_lock))
 		pr_err("lock is taken.\n");
 else {
 	spin_lock_irqsave(&psinfo->buf_lock, flags);
 }

However, this won't work for this reason.
  - printk() must not be called in serialized path because deadlock of logbuf_lock may cause.

Of course, we can avoid deadlock if spin_lock_init(logbuf_lock) is called. But spin_lock_init() 
is not accepted in previous discussion.

>> +	WARN_ON(in_nmi() && reason != KMSG_DUMP_PANIC);
>> +
>
>You will need a comment to explain why the above is there.

I agree. The comment is needed.

Seiji

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/