linux-kernel - Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)

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]

Date:	Tue, 28 Sep 2010 22:12:42 +0200
From:	Janjaap Bos <janjaap@....nl>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:	richard -rw- weinberger <richard.weinberger@...il.com>,
	tj@...nel.org, axboe@...nel.dk, mike.miller@...com,
	user-mode-linux-devel@...ts.sourceforge.net,
	adrian@...en.demon.co.uk, jdike@...toit.com,
	LKML <linux-kernel@...r.kernel.org>, grant.likely@...retlab.ca,
	Geert.Uytterhoeven@...ycom.com, Chris Frey <cdfrey@...rsquare.net>
Subject: Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3
 filesystem corruption in user mode linux)

Hi,

On Tue, 2010-09-28 at 21:52 +0200, Geert Uytterhoeven wrote:
> On Tue, Sep 28, 2010 at 19:07, Janjaap Bos <janjaap@....nl> wrote:
> > See attached patch, and earlier message posted in March 2010 on uml user
> > list. We are out of maintainer...
> 
> Thanks for the patch!
> 
> | --- a/arch/um/drivers/ubd_kern.c
> | +++ b/arch/um/drivers/ubd_kern.c
> | @@ -1223,7 +1227,7 @@ static void do_ubd_request(struct request_queue *q)
> |       struct io_thread_req *io_req;
> |       struct request *req;
> |       sector_t sector;
> | -     int n;
> | +     int n, last_sectors;
> |
> |       while(1){
> |               struct ubd *dev = q->queuedata;
> | @@ -1239,9 +1243,12 @@ static void do_ubd_request(struct request_queue *q)
> |
> |               req = dev->request;
> |               sector = blk_rq_pos(req);
> | +             last_sectors = 0;
> |               while(dev->start_sg < dev->end_sg){
> |                       struct scatterlist *sg = &dev->sg[dev->start_sg];
> |
> | +                     sector += last_sectors;
> | +                     last_sectors = 0;
> |                       io_req = kmalloc(sizeof(struct io_thread_req),
> |                                        GFP_ATOMIC);
> |                       if(io_req == NULL){
> | @@ -1253,7 +1260,7 @@ static void do_ubd_request(struct request_queue *q)
> |                                       (unsigned long long)sector << 9,
> |                                       sg->offset, sg->length, sg_page(sg));
> |
> | -                     sector += sg->length >> 9;
> | +                     last_sectors = sg->length >> 9;
> |                       n = os_write_file(thread_fd, &io_req,
> |                                         sizeof(struct io_thread_req *));
> |                       if(n != sizeof(struct io_thread_req *)){
> 
> However, I'm wondering what difference this part makes?
> 


It fixes ubd block handling integrity.
With large block operations errors occurred. Probably due to lost
request pointers as explained below. Need to keep a local count of
sectors and delay the update. Done by reverting commit
f81f2f7c9fee307e371f37424577d46f9eaf8692 using the present
block api. (At least which is what I intend, but perhaps quite likely I
am missing the point, also it may not be needed anymore if only single
512 byte sector blocks are used per request. Anyway... it solves the
problem for me ;-)

Regards,
-Janjaap


See:

reverted: commit f81f2f7c9fee307e371f37424577d46f9eaf8692
          Author: Tejun Heo <tj@...nel.org>
          Date:   Tue Apr 28 13:06:10 2009 +0900
          ubd: drop unnecessary rq->sector manipulation
    ubd curiously updates rq->sector while issuing the request
    in multiple pieces.  Don't do it and simply use local copy
    of sector.
        
See for original reason:
    
commit 0a6d3a2a3813e7b25267366cfbf9a4a4698dd1c2
        Author: Jeff Dike <jdike@...toit.com>
        Date:   Sun Jul 15 23:38:47 2007 -0700
        uml: fix request->sector update
    
    It is theoretically possible for a request to finish and be freed
between writing it to the I/O thread and updating the sector count.  In
this case, the update will dereference a freed pointer.
    To avoid this, I delay the update until processing the next sg
segment, when the request pointer is known to be good.
    
    modified:   arch/um/drivers/ubd_kern.c



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