[PATCH] nbd in 2.5.3 does not work, and can cause severe damage when read-write

[PATCH] nbd in 2.5.3 does not work, and can cause severe damage when read-write

[Petr Vandrovec]

Hi Linus,
    I've got strange idea and tried to build diskless machine around
2.5.3... Besides problem with segfaulting crc32 (it is initialized after 
net/ipv4/ipconfig.c due to lib/lib.a being a library... I had to hardcode
lib/crc32.o before --start-group in main Makefile, but it is another
story) there is bad problem with NBD caused by BIO changes:

(1) request flags were immediately put into on-wire request format.
    In the past, we had 0=READ, !0=WRITE. Now only REQ_RW bit determines
    direction. As nbd-server from nbd distribution package treats any
    non-zero value as write, it performs writes instead of read. Fortunately
    it will die due to other consistency checks on incoming request, but...

(2) nbd servers handle only up to 10240 byte requests. So setting max_sectors
    to 20 is needed, as otherwise nbd server commits suicide. Maximum request size
    should be handshaked during nbd initialization, but currently just use
    hardwired 20 sectors, so it will behave like it did in the past.

						Thanks,
							Petr Vandrovec
							vandrove@vc.cvut.cz


diff -urdN linux/drivers/block/nbd.c linux/drivers/block/nbd.c
--- linux/drivers/block/nbd.c	Thu Jan 10 18:15:38 2002
+++ linux/drivers/block/nbd.c	Thu Jan 31 00:24:50 2002
@@ -155,14 +155,15 @@
 	unsigned long size = req->nr_sectors << 9;
 
 	DEBUG("NBD: sending control, ");
+	
+	rw = rq_data_dir(req);
+	
 	request.magic = htonl(NBD_REQUEST_MAGIC);
-	request.type = htonl(req->flags);
+	request.type = htonl((rw & WRITE) ? 1 : 0);
 	request.from = cpu_to_be64( (u64) req->sector << 9);
 	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
-	rw = rq_data_dir(req);
-
 	result = nbd_xmit(1, sock, (char *) &request, sizeof(request), rw & WRITE ? MSG_MORE : 0);
 	if (result <= 0)
 		FAIL("Sendmsg failed for control.");
@@ -517,6 +518,7 @@
 	blksize_size[MAJOR_NR] = nbd_blksizes;
 	blk_size[MAJOR_NR] = nbd_sizes;
 	blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), do_nbd_request, &nbd_lock);
+	blk_queue_max_sectors(BLK_DEFAULT_QUEUE(MAJOR_NR), 20);
 	for (i = 0; i < MAX_NBD; i++) {
 		nbd_dev[i].refcnt = 0;
 		nbd_dev[i].file = NULL;

crc32 and lib.a (was Re: [PATCH] nbd in 2.5.3 does not work, and can cause severe damage when read-write)

[Jeff Garzik]

On Thu, Jan 31, 2002 at 02:24:46PM +0100, Petr Vandrovec wrote:
>     I've got strange idea and tried to build diskless machine around
> 2.5.3... Besides problem with segfaulting crc32 (it is initialized after 
> net/ipv4/ipconfig.c due to lib/lib.a being a library... I had to hardcode
> lib/crc32.o before --start-group in main Makefile, but it is another
> story)

Would you be willing to cook up a patch for this problem?

I ran into this too.  It was solved by setting CONFIG_CRC32=n and
letting the Makefile rules pull it in...  but lib/lib.a needs to be
lib/lib.o really.

	Jeff

Re: [PATCH] nbd in 2.5.3 does not work, and can cause severe damage when read-write

[Pavel Machek]

Hi!

>     I've got strange idea and tried to build diskless machine around
> 2.5.3... Besides problem with segfaulting crc32 (it is initialized after 
> net/ipv4/ipconfig.c due to lib/lib.a being a library... I had to hardcode
> lib/crc32.o before --start-group in main Makefile, but it is another
> story) there is bad problem with NBD caused by BIO changes:
> 
> (1) request flags were immediately put into on-wire request format.
>     In the past, we had 0=READ, !0=WRITE. Now only REQ_RW bit determines
>     direction. As nbd-server from nbd distribution package treats any
>     non-zero value as write, it performs writes instead of read. Fortunately
>     it will die due to other consistency checks on incoming request,
>     but...

I have no problem with this.

> (2) nbd servers handle only up to 10240 byte requests. So setting max_sectors
>     to 20 is needed, as otherwise nbd server commits suicide. Maximum request size
>     should be handshaked during nbd initialization, but currently just use
>     hardwired 20 sectors, so it will behave like it did in the past.

But please do not apply this one. Nbd servers should be fixed, and I
already have fix in cvs. (Besides, its trivial). Just make buffer in
server 1MB big.

I do not like idea of handshake.

So, first hunk is fine, but I do not like second one.
									Pavel

> diff -urdN linux/drivers/block/nbd.c linux/drivers/block/nbd.c
> --- linux/drivers/block/nbd.c	Thu Jan 10 18:15:38 2002
> +++ linux/drivers/block/nbd.c	Thu Jan 31 00:24:50 2002
> @@ -155,14 +155,15 @@
>  	unsigned long size = req->nr_sectors << 9;
>  
>  	DEBUG("NBD: sending control, ");
> +	
> +	rw = rq_data_dir(req);
> +	
>  	request.magic = htonl(NBD_REQUEST_MAGIC);
> -	request.type = htonl(req->flags);
> +	request.type = htonl((rw & WRITE) ? 1 : 0);
>  	request.from = cpu_to_be64( (u64) req->sector << 9);
>  	request.len = htonl(size);
>  	memcpy(request.handle, &req, sizeof(req));
>  
> -	rw = rq_data_dir(req);
> -
>  	result = nbd_xmit(1, sock, (char *) &request, sizeof(request), rw & WRITE ? MSG_MORE : 0);
>  	if (result <= 0)
>  		FAIL("Sendmsg failed for control.");
> @@ -517,6 +518,7 @@
>  	blksize_size[MAJOR_NR] = nbd_blksizes;
>  	blk_size[MAJOR_NR] = nbd_sizes;
>  	blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), do_nbd_request, &nbd_lock);
> +	blk_queue_max_sectors(BLK_DEFAULT_QUEUE(MAJOR_NR), 20);
>  	for (i = 0; i < MAX_NBD; i++) {
>  		nbd_dev[i].refcnt = 0;
>  		nbd_dev[i].file = NULL;

-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: [PATCH] nbd in 2.5.3 does not work, and can cause severe damage when read-write

[Petr Vandrovec]

On Thu, Jan 31, 2002 at 10:21:57PM +0100, Pavel Machek wrote:
> >     to 20 is needed, as otherwise nbd server commits suicide. Maximum request size
> >     should be handshaked during nbd initialization, but currently just use
> >     hardwired 20 sectors, so it will behave like it did in the past.
> 
> But please do not apply this one. Nbd servers should be fixed, and I
> already have fix in cvs. (Besides, its trivial). Just make buffer in
> server 1MB big.
> 
> I do not like idea of handshake.

I do not like breaking backward compatibility when there is no
need for that, but as you will be the target of the complaints...

Linus, this reverts limit for request size from 10KB to unlimited.
Although no released nbd version supports it, it is certainly better to
add support to servers than cripple clients if incompatibility does
not matter.
					Best regards,
						Petr Vandrovec
						vandrove@vc.cvut.cz

 
diff -urdN linux/drivers/block/nbd.c linux/drivers/block/nbd.c
--- linux/drivers/block/nbd.c	Thu Jan 31 19:00:00 2002
+++ linux/drivers/block/nbd.c	Thu Jan 10 18:15:38 2002
@@ -518,7 +518,6 @@
 	blksize_size[MAJOR_NR] = nbd_blksizes;
 	blk_size[MAJOR_NR] = nbd_sizes;
 	blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), do_nbd_request, &nbd_lock);
-	blk_queue_max_sectors(BLK_DEFAULT_QUEUE(MAJOR_NR), 20);
 	for (i = 0; i < MAX_NBD; i++) {
 		nbd_dev[i].refcnt = 0;
 		nbd_dev[i].file = NULL;