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
| ||
|
Date: Wed, 26 Sep 2012 16:35:56 -0700 From: Anton Vorontsov <anton.vorontsov@...aro.org> To: "Luck, Tony" <tony.luck@...el.com> Cc: "Liu, Chuansheng" <chuansheng.liu@...el.com>, "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, "keescook@...omium.org" <keescook@...omium.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Colin Cross <ccross@...roid.com> Subject: Re: [PATCH] pstore: avoid recursive spinlocks in the oops_in_progress case On Mon, Sep 24, 2012 at 03:02:35PM +0000, Luck, Tony wrote: > > And my plan was to get rid of the fact that backends touch pstore->buf > > directly. Backends would always receive anonymous 'buf' pointer (we > > already have write_buf callback that does exactly this), and thus it > > It feels like we are just shuffling the lock problem from one place > to another. In the panic case we have to use a pre-allocated buffer > (hoping that we can allocate one seems to be a foolish plan). Yes, we definitely need some buffer pre-allocated for panics, so I have no plans to get rid of the 'buf' -- both 'buf' and 'buf_lock' are going to stay. But it will not be multi-purpose anymore (which I see as an improvement). The thing is, what I dislike is the whole console_write routine: static void pstore_console_write(struct console *con, const char *s, unsigned c) { const char *e = s + c; while (s < e) { unsigned long flags; if (c > psinfo->bufsize) c = psinfo->bufsize; spin_lock_irqsave(&psinfo->buf_lock, flags); memcpy(psinfo->buf, s, c); psinfo->write(PSTORE_TYPE_CONSOLE, 0, NULL, 0, c, psinfo); spin_unlock_irqrestore(&psinfo->buf_lock, flags); s += c; c = e - s; } } It's bad not because of the locks, but because we unnecessary copy things around -- and that's the only reason why we need the lock in the first place. With write_buf, the above would turn into just: static void pstore_console_write(struct console *con, const char *s, unsigned c) { psinfo->write_buf(PSTORE_TYPE_CONSOLE, 0, NULL, 0, s, c, psinfo); } Of course, this assumes that write_buf does its own hw/driver-specific protection, but only if necessary: for ram backend no locks would be necessary at all. And as it appears, both erst and efivars drivers do their own locks in the ->write callback anyways. (Btw, efi pstore backend just grabs its lock w/o trying it first, is it in trouble?) But for panic case, we will use buf and buf_lock: pstore_dump() { lock(buf_lock); ... psinfo->write_buf(PSTORE_TYPE_DMESG, ..., psinfo->buf, ...); ... unlock(buf_lock); } So, we will use them, but only when necessary (for dumping). We can think of them as dump_buf and dump_buf_lock, because that's the only when this stuff will be needed, actually. Thanks, Anton. -- 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/
Powered by blists - more mailing lists