Re: [GIT PULL] block bits

Re: [GIT PULL] block bits

[Boaz Harrosh]

Jens Axboe wrote:
> Hi Linus,
> 
> One critical bit in here, which fixes a reported oops from the iostat
> patch. The others are either trivial/minor or documentation. Please
> pull.
> 
>   git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> Alberto Bertogli (2):
>       Fix misleading comment in bio.h
>       bio.h: If they MUST be inlined, then use __always_inline
> 
> Jens Axboe (3):
>       block: fix oops in blk_queue_io_stat()
>       block: fix inconsistent parenthesisation of QUEUE_FLAG_DEFAULT
>       block: add text file detailing queue/ sysfs files
> 

Hi Jens.

The bsg sense handling bug fix, is I think, a security breach in that
Kernel copies dangling Kernel stack back to user-mode. It might even
need a stable release.

At the time I was thinking that since bsg is marked EXPERIMENTAL in
Kconfig then it must not be included in distro's Kernels by default,
but Benny noted that it is included in Fedora since FC8 or FC9.

The patch I sent still applies I think?

Thanks
Boaz

Re: [GIT PULL] block bits

[Jens Axboe]

On Mon, Feb 02 2009, Boaz Harrosh wrote:
> Jens Axboe wrote:
> > Hi Linus,
> > 
> > One critical bit in here, which fixes a reported oops from the iostat
> > patch. The others are either trivial/minor or documentation. Please
> > pull.
> > 
> >   git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > Alberto Bertogli (2):
> >       Fix misleading comment in bio.h
> >       bio.h: If they MUST be inlined, then use __always_inline
> > 
> > Jens Axboe (3):
> >       block: fix oops in blk_queue_io_stat()
> >       block: fix inconsistent parenthesisation of QUEUE_FLAG_DEFAULT
> >       block: add text file detailing queue/ sysfs files
> > 
> 
> Hi Jens.
> 
> The bsg sense handling bug fix, is I think, a security breach in that
> Kernel copies dangling Kernel stack back to user-mode. It might even
> need a stable release.
> 
> At the time I was thinking that since bsg is marked EXPERIMENTAL in
> Kconfig then it must not be included in distro's Kernels by default,
> but Benny noted that it is included in Fedora since FC8 or FC9.
> 
> The patch I sent still applies I think?

I'm sure it does, I didn't know if Tomo had swoopt it up himself. I'll
add it to this branch.

-- 
Jens Axboe

[PATCH resend] bsg: Fix sense buffer bug in SG_IO

[Boaz Harrosh]

When submitting requests via SG_IO, which does a sync io, a
bsg_command is not allocated. So an in-Kernel sense_buffer was not
set. However when calling blk_execute_rq() with no sense buffer
one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq()
would check if rq->sense_len and a sense was requested by sg_io_v4
the rq->sense was copy_user() back, but by now it is already mangled
stack memory.

I have fixed that by forcing a sense_buffer when calling bsg_map_hdr().
The bsg_command->sense is provided in the write/read path like before,
and on-the-stack buffer is provided when doing SG_IO.

I have also fixed a dprintk message to print rq->errors in hex because
of the scsi bit-field use of this member. For other block devices it
does not matter anyway.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/bsg.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index d414bb5..0ce8806 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -244,7 +244,8 @@ bsg_validate_sgv4_hdr(struct request_queue *q, struct sg_io_v4 *hdr, int *rw)
  * map sg_io_v4 to a request.
  */
 static struct request *
-bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm)
+bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
+	    u8 *sense)
 {
 	struct request_queue *q = bd->queue;
 	struct request *rq, *next_rq = NULL;
@@ -306,6 +307,10 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm)
 		if (ret)
 			goto out;
 	}
+
+	rq->sense = sense;
+	rq->sense_len = 0;
+
 	return rq;
 out:
 	if (rq->cmd != rq->__cmd)
@@ -348,9 +353,6 @@ static void bsg_rq_end_io(struct request *rq, int uptodate)
 static void bsg_add_command(struct bsg_device *bd, struct request_queue *q,
 			    struct bsg_command *bc, struct request *rq)
 {
-	rq->sense = bc->sense;
-	rq->sense_len = 0;
-
 	/*
 	 * add bc command to busy queue and submit rq for io
 	 */
@@ -419,7 +421,7 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 {
 	int ret = 0;
 
-	dprintk("rq %p bio %p %u\n", rq, bio, rq->errors);
+	dprintk("rq %p bio %p 0x%x\n", rq, bio, rq->errors);
 	/*
 	 * fill in all the output members
 	 */
@@ -635,7 +637,7 @@ static int __bsg_write(struct bsg_device *bd, const char __user *buf,
 		/*
 		 * get a request, fill in the blanks, and add to request queue
 		 */
-		rq = bsg_map_hdr(bd, &bc->hdr, has_write_perm);
+		rq = bsg_map_hdr(bd, &bc->hdr, has_write_perm, bc->sense);
 		if (IS_ERR(rq)) {
 			ret = PTR_ERR(rq);
 			rq = NULL;
@@ -922,11 +924,12 @@ static long bsg_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		struct request *rq;
 		struct bio *bio, *bidi_bio = NULL;
 		struct sg_io_v4 hdr;
+		u8 sense[SCSI_SENSE_BUFFERSIZE];
 
 		if (copy_from_user(&hdr, uarg, sizeof(hdr)))
 			return -EFAULT;
 
-		rq = bsg_map_hdr(bd, &hdr, file->f_mode & FMODE_WRITE);
+		rq = bsg_map_hdr(bd, &hdr, file->f_mode & FMODE_WRITE, sense);
 		if (IS_ERR(rq))
 			return PTR_ERR(rq);
 
-- 
1.6.0.1

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

[Jens Axboe]

On Tue, Feb 03 2009, Boaz Harrosh wrote:
> 
> When submitting requests via SG_IO, which does a sync io, a
> bsg_command is not allocated. So an in-Kernel sense_buffer was not
> set. However when calling blk_execute_rq() with no sense buffer
> one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq()
> would check if rq->sense_len and a sense was requested by sg_io_v4
> the rq->sense was copy_user() back, but by now it is already mangled
> stack memory.
> 
> I have fixed that by forcing a sense_buffer when calling bsg_map_hdr().
> The bsg_command->sense is provided in the write/read path like before,
> and on-the-stack buffer is provided when doing SG_IO.
> 
> I have also fixed a dprintk message to print rq->errors in hex because
> of the scsi bit-field use of this member. For other block devices it
> does not matter anyway.

Thanks for the resend Boaz, I'll make sure it gets upstream for 2.6.29.

-- 
Jens Axboe

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

[FUJITA Tomonori]

On Tue, 03 Feb 2009 08:34:07 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 
> When submitting requests via SG_IO, which does a sync io, a
> bsg_command is not allocated. So an in-Kernel sense_buffer was not
> set. However when calling blk_execute_rq() with no sense buffer
> one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq()
> would check if rq->sense_len and a sense was requested by sg_io_v4
> the rq->sense was copy_user() back, but by now it is already mangled
> stack memory.
> 
> I have fixed that by forcing a sense_buffer when calling bsg_map_hdr().
> The bsg_command->sense is provided in the write/read path like before,
> and on-the-stack buffer is provided when doing SG_IO.
> 
> I have also fixed a dprintk message to print rq->errors in hex because
> of the scsi bit-field use of this member. For other block devices it
> does not matter anyway.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/bsg.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)

Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>


I think I did this in the previous submission though.

Re: [PATCH resend] bsg: Fix sense buffer bug in SG_IO

[Jens Axboe]

On Tue, Feb 03 2009, FUJITA Tomonori wrote:
> On Tue, 03 Feb 2009 08:34:07 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > 
> > When submitting requests via SG_IO, which does a sync io, a
> > bsg_command is not allocated. So an in-Kernel sense_buffer was not
> > set. However when calling blk_execute_rq() with no sense buffer
> > one is provided from the stack. Now bsg at blk_complete_sgv4_hdr_rq()
> > would check if rq->sense_len and a sense was requested by sg_io_v4
> > the rq->sense was copy_user() back, but by now it is already mangled
> > stack memory.
> > 
> > I have fixed that by forcing a sense_buffer when calling bsg_map_hdr().
> > The bsg_command->sense is provided in the write/read path like before,
> > and on-the-stack buffer is provided when doing SG_IO.
> > 
> > I have also fixed a dprintk message to print rq->errors in hex because
> > of the scsi bit-field use of this member. For other block devices it
> > does not matter anyway.
> > 
> > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > ---
> >  block/bsg.c |   17 ++++++++++-------
> >  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> 
> I think I did this in the previous submission though.

Yeah, I also added it.

-- 
Jens Axboe

Re: [osd-dev] Kernel crashes on login

[Boaz Harrosh]

Boaz Harrosh wrote:
> Fuerst Lars wrote:
>> Hello Boaz,
>>
>> I try to use your kernel initiator in combination with the OSC osd2
>> target. The target (FC 9, 2.6.26.5-45.fc9.x86_64) works fine with the
>> user space initiator from osc, but when i use your initiator (in tree
>> build 2.6.29-rc1-9) i get a failure after login:
>>
>> # iscsiadm -m node -p 141.11.96.100 -T render01crh --login
>> Logging in to [iface: default, target: render01crh, portal:
>> 141.11.96.100,3260]
>> iscsiadm: got read error (0/0), daemon died?
>>
>> iscsiadm: Could not login to [iface: default, target: render01crh,
>> portal: 141.11.96.100,3260]: 
>> iscsiadm: initiator reported error (18 - could not communicate to
>> iscsid)  
>>
>> kernel log says:
>>
>> [..]
>> Feb  5 14:29:16 render02crh kernel: osd:
>> OSD_ATTR_RI_VENDOR_IDENTIFICATION [<NULL>]
>>
>> Feb  5 14:29:16 render02crh kernel: osd:
>> OSD_ATTR_RI_PRODUCT_IDENTIFICATION [<NULL>]
>>
>> Feb  5 14:29:16 render02crh kernel: osd: OSD_ATTR_RI_PRODUCT_MODEL
>> [<NULL>]                                                             
>> Feb  5 14:29:16 render02crh kernel: BUG: unable to handle kernel NULL
>> pointer dereference at (null)                                     
>> Feb  5 14:29:16 render02crh kernel: IP: [<ffffffffa3114aa3>]
>> _osd_print_system_info+0x177/0x29b [libosd]
>>
>> Feb  5 14:29:16 render02crh kernel: PGD 6ad4d067 PUD 785d5067 PMD 0
>>
>> Feb  5 14:29:16 render02crh kernel: Oops: 0000 [#1] SMP
>>
>> Feb  5 14:29:16 render02crh kernel: last sysfs file:
>> /sys/devices/platform/host5/scsi_host/host5/scan
>> [..]
>>
>> Complete log and kernel config is attached. It seems to be a problem
>> during the osd auto detection. Do you have an idea?
>>
>> Thanks in advance
>>
>> Regards,
>>
>> Lars
> 
> Thanks Lars, It should not crash, I'll fix that ASAP
> 
> But please use the OSC's target from the open-osd.org
> git trees. Please follow the instructions in this page:
> http://www.open-osd.org/bin/view/Main/OscOsdProject
> Specifically the "How to get started" section.
> This will also automatically fetch the tgt module with all
> the right patches applied. So it should be easier to use.
> 
> The original version is missing some mandatory attributes
> and other incompatibilities with OSD2 been crafted on draft
> 01, when currently we're on draft 04d.
> 
> But sorry I should have checked that. Code should not crash
> ever.
> 
> I'll send a fix today.
> 
> Boaz
> 

Nice things about having some users is that they get to find
your bugs ;) . Thanks Lars

Some project History:

  At git://git.open-osd.org/osc-osd, our OSC's osd-target git tree,
there are a couple of fixes and enhancements over the latest source
code from OSC's internal SVN.

  Prior to that when we first started to use OSC's target we sent to
Pete and Anath fixes regarding none conforming behavior with regard
to attributes. The tar balls that where public on the net at the time
had these short comings. 

Lars are you using the old tar balls, I thought they where removed
from the net around the time Pete left OSC. (By accident)

The latest SVN code included our fixes, (and did not fully compile).
Are only publicly available on above git in the "osc" branch.
I have tested with that version and it does not crash so it might
be that you are using the old tar balls. In that case even with my
patch to fix the crash, attributes will not work at all and you will
not be able to work with open-osd initiator. :-(

It is strongly recommended that you upgrade to the open-osd version of
OSC's osd-target. (It also includes a matching OSC's osd-initiator)

I'm sending a patch as it's own email thread to James for inclusion
into 2.6.30.

Thank you for trying out and helping with the open-osd initiator.

Boaz