linux-kernel - [PATCH 3.16 038/136] bcache: only permit to recovery read error when cache device is clean

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: <lsq.1518323471.119885799@decadent.org.uk>
Date:   Sun, 11 Feb 2018 04:31:11 +0000
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "Kent Overstreet" <kent.overstreet@...il.com>,
        "Junhui Tang" <tang.junhui@....com.cn>,
        "Eric Wheeler" <bcache@...ts.ewheeler.net>,
        "Michael Lyle" <mlyle@...e.org>, "Jens Axboe" <axboe@...nel.dk>,
        "Nix" <nix@...eri.org.uk>, "Kai Krakow" <hurikhan77@...il.com>,
        "Arne Wolf" <awolf@...ovo.com>, "Coly Li" <colyli@...e.de>
Subject: [PATCH 3.16 038/136] bcache: only permit to recovery read error
 when cache device is clean

3.16.54-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Coly Li <colyli@...e.de>

commit d59b23795933678c9638fd20c942d2b4f3cd6185 upstream.

When bcache does read I/Os, for example in writeback or writethrough mode,
if a read request on cache device is failed, bcache will try to recovery
the request by reading from cached device. If the data on cached device is
not synced with cache device, then requester will get a stale data.

For critical storage system like database, providing stale data from
recovery may result an application level data corruption, which is
unacceptible.

With this patch, for a failed read request in writeback or writethrough
mode, recovery a recoverable read request only happens when cache device
is clean. That is to say, all data on cached device is up to update.

For other cache modes in bcache, read request will never hit
cached_dev_read_error(), they don't need this patch.

Please note, because cache mode can be switched arbitrarily in run time, a
writethrough mode might be switched from a writeback mode. Therefore
checking dc->has_data in writethrough mode still makes sense.

Changelog:
V4: Fix parens error pointed by Michael Lyle.
v3: By response from Kent Oversteet, he thinks recovering stale data is a
    bug to fix, and option to permit it is unnecessary. So this version
    the sysfs file is removed.
v2: rename sysfs entry from allow_stale_data_on_failure  to
    allow_stale_data_on_failure, and fix the confusing commit log.
v1: initial patch posted.

[small change to patch comment spelling by mlyle]

Signed-off-by: Coly Li <colyli@...e.de>
Signed-off-by: Michael Lyle <mlyle@...e.org>
Reported-by: Arne Wolf <awolf@...ovo.com>
Reviewed-by: Michael Lyle <mlyle@...e.org>
Cc: Kent Overstreet <kent.overstreet@...il.com>
Cc: Nix <nix@...eri.org.uk>
Cc: Kai Krakow <hurikhan77@...il.com>
Cc: Eric Wheeler <bcache@...ts.ewheeler.net>
Cc: Junhui Tang <tang.junhui@....com.cn>
Signed-off-by: Jens Axboe <axboe@...nel.dk>
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 drivers/md/bcache/request.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -698,8 +698,16 @@ static void cached_dev_read_error(struct
 {
 	struct search *s = container_of(cl, struct search, cl);
 	struct bio *bio = &s->bio.bio;
+	struct cached_dev *dc = container_of(s->d, struct cached_dev, disk);
 
-	if (s->recoverable) {
+	/*
+	 * If cache device is dirty (dc->has_dirty is non-zero), then
+	 * recovery a failed read request from cached device may get a
+	 * stale data back. So read failure recovery is only permitted
+	 * when cache device is clean.
+	 */
+	if (s->recoverable &&
+	    (dc && !atomic_read(&dc->has_dirty))) {
 		/* Retry from the backing device: */
 		trace_bcache_read_retry(s->orig_bio);