All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] block: Restore tape support
@ 2018-12-10  3:08 Bart Van Assche
  2018-12-10 11:17 ` Christoph Hellwig
  2018-12-10 15:32 ` Keith Busch
  0 siblings, 2 replies; 9+ messages in thread
From: Bart Van Assche @ 2018-12-10  3:08 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Christoph Hellwig, Bart Van Assche, Todd Aiken,
	Keith Busch, Laurence Oberman, stable

According to what I found in
https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block: Clear
kernel memory before copying to user" broke tape access. Hence revert
that patch.

Reported-by: Todd Aiken <taiken@mvtech.ca>
Fixes: f3587d76da05 ("block: Clear kernel memory before copying to user") # v4.20-rc2
Cc: Todd Aiken <taiken@mvtech.ca>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Laurence Oberman <loberman@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/bio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/bio.c b/block/bio.c
index 06760543ec81..90c32cd3e0c7 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1262,7 +1262,6 @@ struct bio *bio_copy_user_iov(struct request_queue *q,
 		if (ret)
 			goto cleanup;
 	} else {
-		zero_fill_bio(bio);
 		iov_iter_advance(iter, bio->bi_iter.bi_size);
 	}
 
-- 
2.19.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10  3:08 [PATCH] block: Restore tape support Bart Van Assche
@ 2018-12-10 11:17 ` Christoph Hellwig
  2018-12-10 15:11   ` Laurence Oberman
  2018-12-10 15:32 ` Keith Busch
  1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2018-12-10 11:17 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Todd Aiken,
	Keith Busch, Laurence Oberman, stable

On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
> According to what I found in
> https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block: Clear
> kernel memory before copying to user" broke tape access. Hence revert
> that patch.

This looks wrong.  The patch from Keith is obviously correct for
normal usage of bio_copy_user_iov.  The SCSI tape drivers use the
somewhat odd null_mapped case, so we probably need to refine the checks
for that a bit.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 11:17 ` Christoph Hellwig
@ 2018-12-10 15:11   ` Laurence Oberman
  0 siblings, 0 replies; 9+ messages in thread
From: Laurence Oberman @ 2018-12-10 15:11 UTC (permalink / raw)
  To: Christoph Hellwig, Bart Van Assche
  Cc: Jens Axboe, linux-block, Todd Aiken, Keith Busch, stable

On Mon, 2018-12-10 at 12:17 +0100, Christoph Hellwig wrote:
> On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
> > According to what I found in
> > https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block:
> > Clear
> > kernel memory before copying to user" broke tape access. Hence
> > revert
> > that patch.
> 
> This looks wrong.  The patch from Keith is obviously correct for
> normal usage of bio_copy_user_iov.  The SCSI tape drivers use the
> somewhat odd null_mapped case, so we probably need to refine the
> checks
> for that a bit.

I am going to look into that, just connected a tape device so I can
reproduce and then see what we need to make it more specific in the st
driver.
I may need help from folks who understand the bio stuff better than I
do.

Back when I have more.

Regards
Laurence

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10  3:08 [PATCH] block: Restore tape support Bart Van Assche
  2018-12-10 11:17 ` Christoph Hellwig
@ 2018-12-10 15:32 ` Keith Busch
  2018-12-10 15:36   ` Jens Axboe
  2018-12-10 20:06   ` Laurence Oberman
  1 sibling, 2 replies; 9+ messages in thread
From: Keith Busch @ 2018-12-10 15:32 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Todd Aiken,
	Laurence Oberman, stable

On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
> According to what I found in
> https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block: Clear
> kernel memory before copying to user" broke tape access. Hence revert
> that patch.

Instead of reverting back to the leaking arbitrary kernel memory, why
not just make a fix on top of it? This should do it:

---
diff --git a/block/bio.c b/block/bio.c
index c4ef8aa46452..55a5386fd431 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1262,7 +1262,8 @@ struct bio *bio_copy_user_iov(struct request_queue *q,
 		if (ret)
 			goto cleanup;
 	} else {
-		zero_fill_bio(bio);
+		if (bmd->is_our_pages)
+			zero_fill_bio(bio);
 		iov_iter_advance(iter, bio->bi_iter.bi_size);
 	}
 
--

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 15:32 ` Keith Busch
@ 2018-12-10 15:36   ` Jens Axboe
  2018-12-10 16:17     ` Laurence Oberman
  2018-12-10 20:06   ` Laurence Oberman
  1 sibling, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2018-12-10 15:36 UTC (permalink / raw)
  To: Keith Busch, Bart Van Assche
  Cc: linux-block, Christoph Hellwig, Todd Aiken, Laurence Oberman, stable

On 12/10/18 8:32 AM, Keith Busch wrote:
> On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
>> According to what I found in
>> https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block: Clear
>> kernel memory before copying to user" broke tape access. Hence revert
>> that patch.
> 
> Instead of reverting back to the leaking arbitrary kernel memory, why
> not just make a fix on top of it? This should do it:
> 
> ---
> diff --git a/block/bio.c b/block/bio.c
> index c4ef8aa46452..55a5386fd431 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1262,7 +1262,8 @@ struct bio *bio_copy_user_iov(struct request_queue *q,
>  		if (ret)
>  			goto cleanup;
>  	} else {
> -		zero_fill_bio(bio);
> +		if (bmd->is_our_pages)
> +			zero_fill_bio(bio);
>  		iov_iter_advance(iter, bio->bi_iter.bi_size);
>  	}
>  

That should be fine, the other case is user mapped memory anyway.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 15:36   ` Jens Axboe
@ 2018-12-10 16:17     ` Laurence Oberman
  0 siblings, 0 replies; 9+ messages in thread
From: Laurence Oberman @ 2018-12-10 16:17 UTC (permalink / raw)
  To: Jens Axboe, Keith Busch, Bart Van Assche
  Cc: linux-block, Christoph Hellwig, Todd Aiken, stable

On Mon, 2018-12-10 at 08:36 -0700, Jens Axboe wrote:
> On 12/10/18 8:32 AM, Keith Busch wrote:
> > On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
> > > According to what I found in
> > > https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block:
> > > Clear
> > > kernel memory before copying to user" broke tape access. Hence
> > > revert
> > > that patch.
> > 
> > Instead of reverting back to the leaking arbitrary kernel memory,
> > why
> > not just make a fix on top of it? This should do it:
> > 
> > ---
> > diff --git a/block/bio.c b/block/bio.c
> > index c4ef8aa46452..55a5386fd431 100644
> > --- a/block/bio.c
> > +++ b/block/bio.c
> > @@ -1262,7 +1262,8 @@ struct bio *bio_copy_user_iov(struct
> > request_queue *q,
> >  		if (ret)
> >  			goto cleanup;
> >  	} else {
> > -		zero_fill_bio(bio);
> > +		if (bmd->is_our_pages)
> > +			zero_fill_bio(bio);
> >  		iov_iter_advance(iter, bio->bi_iter.bi_size);
> >  	}
> >  
> 
> That should be fine, the other case is user mapped memory anyway.
> 

OK, I will test this today
Thanks folks!!
Laurence

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 15:32 ` Keith Busch
  2018-12-10 15:36   ` Jens Axboe
@ 2018-12-10 20:06   ` Laurence Oberman
  2018-12-10 20:26     ` Keith Busch
  1 sibling, 1 reply; 9+ messages in thread
From: Laurence Oberman @ 2018-12-10 20:06 UTC (permalink / raw)
  To: Keith Busch, Bart Van Assche
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Todd Aiken, stable

On Mon, 2018-12-10 at 08:32 -0700, Keith Busch wrote:
> On Sun, Dec 09, 2018 at 07:08:14PM -0800, Bart Van Assche wrote:
> > According to what I found in
> > https://bugzilla.kernel.org/show_bug.cgi?id=201935 patch "block:
> > Clear
> > kernel memory before copying to user" broke tape access. Hence
> > revert
> > that patch.
> 
> Instead of reverting back to the leaking arbitrary kernel memory, why
> not just make a fix on top of it? This should do it:
> 
> ---
> diff --git a/block/bio.c b/block/bio.c
> index c4ef8aa46452..55a5386fd431 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1262,7 +1262,8 @@ struct bio *bio_copy_user_iov(struct
> request_queue *q,
>  		if (ret)
>  			goto cleanup;
>  	} else {
> -		zero_fill_bio(bio);
> +		if (bmd->is_our_pages)
> +			zero_fill_bio(bio);
>  		iov_iter_advance(iter, bio->bi_iter.bi_size);
>  	}
>  
> --
Reproduced on 4.20-rc6

# tar cvf /dev/st0 /etc
...
Showed data was written in tar
However listing is empty, data not actually written

# tar tvf /dev/st0 
#

Then:

Applied Keith Patch

# tar cvf /dev/st0 /etc
...

# tar tvf /dev/st0

Data was written and can be read

..drwxr-xr-x root/root         0 2018-10-31 08:41 etc/fwupd/
-rw-r--r-- root/root       292 2018-06-07 08:27 etc/fwupd/daemon.conf
drwxr-xr-x root/root         0 2018-10-31 08:41 etc/fwupd/remotes.d/
-rw-r--r-- root/root       181 2018-09-05 06:00
etc/fwupd/remotes.d/fwupd.conf
-rw-r--r-- root/root       327 2018-06-07 08:27
etc/fwupd/remotes.d/lvfs-testing.conf
-rw-r--r-- root/root       283 2018-09-05 06:00
etc/fwupd/remotes.d/lvfs.conf
-rw-r--r-- root/root       283 2018-09-05 06:00
etc/fwupd/remotes.d/vendor.conf
-rw-r--r-- root/root       118 2018-09-05 06:00 etc/fwupd/uefi.conf
-r--r----- root/root      4328 2018-09-25 01:57 etc/sudoers.rpmnew
-rw-r--r-- root/root      2331 2018-11-02 14:50 etc/hercules.cnf
-rw-r--r-- root/root       147 2018-11-06 17:08 etc/minirc.p2000


[   96.272548] st 2:0:0:0: [st0] Block limits 1 - 16777215 bytes.
[   96.300782] st 2:0:0:0: [st0] Mode sense. Length 11, medium 0, WBS
10, BLL 8
[   96.334985] st 2:0:0:0: [st0] Density 58, tape length: 0, drv
buffer: 1
[   96.367366] st 2:0:0:0: [st0] Block size: 0, buffer size: 4096 (1
blocks).
[   97.197209] st 2:0:0:0: [st0] Rewinding tape.
[  103.104429] st 2:0:0:0: [st0] Block limits 1 - 16777215 bytes.
[  103.133142] st 2:0:0:0: [st0] Mode sense. Length 11, medium 0, WBS
10, BLL 8
[  103.167421] st 2:0:0:0: [st0] Density 58, tape length: 0, drv
buffer: 1
[  103.199711] st 2:0:0:0: [st0] Block size: 0, buffer size: 4096 (1
blocks).
[  104.109081] st 2:0:0:0: [st0] Rewinding tape.

Tested and works fine. 
Thanks All

Tested-by: Laurence Oberman <loberman@redhat.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 20:06   ` Laurence Oberman
@ 2018-12-10 20:26     ` Keith Busch
  2018-12-10 20:38       ` Todd Aiken
  0 siblings, 1 reply; 9+ messages in thread
From: Keith Busch @ 2018-12-10 20:26 UTC (permalink / raw)
  To: Laurence Oberman
  Cc: Bart Van Assche, Jens Axboe, linux-block, Christoph Hellwig,
	Todd Aiken, stable

On Mon, Dec 10, 2018 at 03:06:52PM -0500, Laurence Oberman wrote:
> Tested and works fine. 
> Thanks All
> 
> Tested-by: Laurence Oberman <loberman@redhat.com>

Cool, thank you for confirming.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] block: Restore tape support
  2018-12-10 20:26     ` Keith Busch
@ 2018-12-10 20:38       ` Todd Aiken
  0 siblings, 0 replies; 9+ messages in thread
From: Todd Aiken @ 2018-12-10 20:38 UTC (permalink / raw)
  To: Keith Busch
  Cc: Laurence Oberman, Bart Van Assche, Jens Axboe, linux-block,
	Christoph Hellwig, stable

----- Message from Keith Busch <keith.busch@intel.com> ---------
    Date: Mon, 10 Dec 2018 13:26:47 -0700
    From: Keith Busch <keith.busch@intel.com>
Subject: Re: [PATCH] block: Restore tape support
      To: Laurence Oberman <loberman@redhat.com>
      Cc: Bart Van Assche <bvanassche@acm.org>, Jens Axboe  
<axboe@kernel.dk>, linux-block@vger.kernel.org, Christoph Hellwig  
<hch@lst.de>, Todd Aiken <taiken@mvtech.ca>, stable@vger.kernel.org


> On Mon, Dec 10, 2018 at 03:06:52PM -0500, Laurence Oberman wrote:
>> Tested and works fine.
>> Thanks All
>>
>> Tested-by: Laurence Oberman <loberman@redhat.com>
>
> Cool, thank you for confirming.

I can also confirm that this fix works fine on my system.


----- End message from Keith Busch <keith.busch@intel.com> -----




^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-12-10 20:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10  3:08 [PATCH] block: Restore tape support Bart Van Assche
2018-12-10 11:17 ` Christoph Hellwig
2018-12-10 15:11   ` Laurence Oberman
2018-12-10 15:32 ` Keith Busch
2018-12-10 15:36   ` Jens Axboe
2018-12-10 16:17     ` Laurence Oberman
2018-12-10 20:06   ` Laurence Oberman
2018-12-10 20:26     ` Keith Busch
2018-12-10 20:38       ` Todd Aiken

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.