From: Petr Mladek <pmladek@suse.com>
To: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] printk: ringbuffer: avoid memcpy() on state_var
Date: Mon, 14 Sep 2020 14:44:05 +0200 [thread overview]
Message-ID: <20200914124404.GD11154@alley> (raw)
In-Reply-To: <20200914094803.27365-2-john.ogness@linutronix.de>
On Mon 2020-09-14 11:54:03, John Ogness wrote:
> @state_var is copied as part of the descriptor copying via
> memcpy(). This is not allowed because @state_var is an atomic type,
> which in some implementations may contain a spinlock.
Great catch!
> Avoid using memcpy() with @state_var by explicitly copying the other
> fields of the descriptor. @state_var is set using atomic set
> operator before returning.
Just thinking loudly.
Strictly speaking, memcpy() might be used when the atomic variable
is later corrected by an atomic operation. And it is done by the first
patch.
I think that it is a matter of taste what solution is more error
prone. Either this function must be updated when a new field
is added into struct prb_desc or the atomic_long_set() must not
get removed.
Hmm, missing memcpy() should be rather easy to debug. While missing
atomic_long_set() is really subtle problem problem. So, the new
code is better from my POV.
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
next prev parent reply other threads:[~2020-09-14 12:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-14 9:48 [PATCH 1/2] printk: ringbuffer: fix setting state in desc_read() John Ogness
2020-09-14 9:48 ` [PATCH 2/2] printk: ringbuffer: avoid memcpy() on state_var John Ogness
2020-09-14 12:44 ` Petr Mladek [this message]
2020-09-14 12:29 ` [PATCH 1/2] printk: ringbuffer: fix setting state in desc_read() Petr Mladek
2020-09-15 15:24 ` Petr Mladek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200914124404.GD11154@alley \
--to=pmladek@suse.com \
--cc=john.ogness@linutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky.work@gmail.com \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).