linux-kernel - Re: [block] allow blk_flush_policy to return REQ_FSEQ

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

Message-ID: <eaf37464-ae9d-4192-9044-2b2d90906a17@googlegroups.com>
Date:	Sat, 12 Jan 2013 23:01:02 -0800 (PST)
From:	Ajith Kumar <ajithb.kumar@...il.com>
To:	fa.linux.kernel@...glegroups.com
Cc:	ajithb.kumar@...il.com, linux-kernel@...r.kernel.org,
	Jens Axboe <jaxboe@...ionio.com>
Subject: Re: [block] allow blk_flush_policy to return REQ_FSEQ_DATA
 independent of *FLUSH

Hello,
Re-reading the code, I see that I was wrong with the previous comment of mine.  So, in summary

i) If block driver does not support FLUSH and FUA, then __generic_make_request() checks will clear BIO FLUSH and FUA flags and hence blk_insert_flush() will not be invoked.

ii) If block driver clears FLUSH & FUA flags while IO is in flight, then there is possibility of IO missing __generic_make_request() checks and hitting issue with blk_insert_flush() being discussed here.

iii) If block driver sets only REQ_FUA without REQ_FLUSH then __generic_make_request() checks will not clear BIO flags and hence blk_insert_flush() will be invoked which will hit the blk_insert_flush() issue being discussed here.  However, setting REQ_FUA without REQ_FLUSH is not as per documentation and it is invalid for block driver to do so.

Thanks,
Ajith

On Wednesday, 9 January 2013 09:44:55 UTC+5:30, Ajith Kumar  wrote:
> Hello,
> 
> Thanks for the response.
> 
> A block device driver during initialization would decide if it is capable of supporting FLUSH/FUA or not.  Suppose driver claims FLUSH/FUA support then any bio targeted at this driver with FLUSH bit set(which is commonly set by file system like XFS for doing internal logging) has a data corruption vulnerability in case of an abrupt shutdown.  So, IMO the vulnerability is not open to rare window where driver changes q->flush_flags while IO is in flight, but for a much larger window from time driver comes up and throughout it's life.
> 
> 
> 
> Thanks,
> 
> Ajith
> 
> 
> 
> On Wednesday, 9 January 2013 00:15:31 UTC+5:30, Tejun Heo  wrote:
> 
> > Hello,
> 
> > 
> 
> > 
> 
> > 
> 
> > On Tue, Jan 08, 2013 at 10:04:23AM -0800, ajithb.kumar@...il.com wrote:
> 
> > 
> 
> > > Hi,
> 
> > 
> 
> > > Could you please provide clarity on the following.
> 
> > 
> 
> > > ">   Hmmm... yes, this can become a correctness issue if (and only if)
> 
> > 
> 
> > > >   blk_queue_flush() is called to change q->flush_flags while requests
> 
> > 
> 
> > > >   are in-flight;"
> 
> > 
> 
> > >
> 
> > 
> 
> > > Could you please clarify as to why is it a correctness issue only if
> 
> > 
> 
> > > blk_queue_flush() is used to change flush_flags when requests are in
> 
> > 
> 
> > > flight ?  As I understand, XFS does set WRITE_FLUSH_FUA flag in
> 
> > 
> 
> > > _xfs_buf_ioapply() function irrespective of whether the underlying
> 
> > 
> 
> > > device supports flush capabilities or not which will flow into
> 
> > 
> 
> > > blk_insert_flush().  Is my reading of the code correct and is there
> 
> > 
> 
> > > a general correctness issue here which potentially results in XFS
> 
> > 
> 
> > > file system corruption in case of an abrupt shutdown independent of
> 
> > 
> 
> > > q->flush_flags getting changed while request is in flight.
> 
> > 
> 
> > 
> 
> > 
> 
> > My memory is kinda fuzzy at this point but if a queue doesn't support
> 
> > 
> 
> > flush, its flush_flags should be zero and
> 
> > 
> 
> > generic_make_request_checks() will clear REQ_FLUSH|REQ_FUA from
> 
> > 
> 
> > bio->bi_rw so we never hit blk_insert_flush() and the request will be
> 
> > 
> 
> > processed as a normal IO one; however, if REQ_FLUSH goes off after a
> 
> > 
> 
> > request passed generic_make_request_checks() but before
> 
> > 
> 
> > blk_flush_policy(), it'll become null op and its data payload won't
> 
> > 
> 
> > get written out to the underlying device, which is data corruption.
> 
> > 
> 
> > 
> 
> > 
> 
> > Thanks.
> 
> > 
> 
> > 
> 
> > 
> 
> > -- 
> 
> > 
> 
> > tejun
> 
> > 
> 
> > --
> 
> > 
> 
> > 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/

--
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/