Re: [PATCH] blkgetsize64 ioctl

Re: [PATCH] blkgetsize64 ioctl

[Michael E Brown]

Ben LaHaise wrote:
> Here's the modified patch (incompatible with e2fsprogs 1.23, but not
> conflicting with ia64: ioctls that write to disk are b0rken).

In reference to the ia64 ioctls:  I'm sorry, but disk access APIs that
don't allow access to the whole disk are what is broken. These ioctls
would not be necessary if you could actually write to the last sector of
an odd-sized disk. Have you read the comments surrounding this ioctl?

-- 
Michael Brown
Linux OS Development
Dell Computer Corp

  If each of us have one object, and we exchange them,
     then each of us still has one object.
  If each of us have one idea,   and we exchange them,
     then each of us now has two ideas.

Re: [PATCH] blkgetsize64 ioctl

[Ben LaHaise]

On Thu, 30 Aug 2001, Michael E Brown wrote:

> In reference to the ia64 ioctls:  I'm sorry, but disk access APIs that
> don't allow access to the whole disk are what is broken. These ioctls
> would not be necessary if you could actually write to the last sector of
> an odd-sized disk. Have you read the comments surrounding this ioctl?

Were people's heads on backwards when they wrote the ioctl?  Quite simply:
if an ugly hack has to be put in place, put it in the right place.  In
this case, it would have been *trivial* to put the UGLY hack in
fs/block_dev.c and just make read/write transparently able to access the
end of the disk.  No adding crap to the API, and certainly not risking
truely unexpected disk io due to an incorrect ioctl.

		-ben

-- 
"The world would be a better place if Larry Wall had been born in
Iceland, or any other country where the native language actually
has syntax" -- Peter da Silva

Re: [PATCH] blkgetsize64 ioctl

[Michael E Brown]

I'm sorry, but I am the one who wrote the ioctl. And I have to admit my
shortcomings. I looked at fs/block_dev.c and it is a big, ugly, quagmire
of code to somebody who isn't a deep kernel-guru. If I were to have tried
that and made some kind of mistake, it would have affected more people
(everybody). As it is, my code is limited to a limited-use ioctl. If there
is a bug in my code there, it will A) be easier for people to find, and B)
affect fewer people.

The next point I would like to make is this: I was adding code to patch
over a hole in the existing API, not trying to create a whole new API, or
fix the exising API in a way that made it incompatible with previous
versions. As for your *trivial* UGLY hack for fs/block_dev.c, how likely
do you think it would be for that to get accepted by Linux, Alan, et al.
At least with the IOCTL, the code stays separate and can be easily
maintained separately from the main kernel until it either gets integrated
or a better API comes along. A *trivial* UGLY hack for fs/block_dev.c
would be harder to maintain for a longer term as a separate entity.

The great thing about the IOCTL is that I have written all of my code that
uses the IOCTL in such a manner that it tries to access the last block
of the disk and only falls back to the IOCTL if there is an error trying
to go through the normal API. So, feel free to fix the block_dev.c API,
and then all of the programs that I wrote which use that IOCTL will no
longer use it. You can then drop the IOCTL entirely.

And your last point about risking unexpected disk-io due to an incorrect
IOCTL, I would say that is a pretty unlikely in practice. First, I do
parameter checking on what was passed to the IOCTL, and if things don't
match, no io is done. Second, how likely is it that you a) call ioctl with
a (disk) block device, b) pass the wrong ioctl, c) pass along enough data
to pass the checks in the ioctl, and d) pass along a valid pointer to 512
bytes of data to overwrite something?

--
Michael

On Thu, 30 Aug 2001, Ben LaHaise wrote:

> On Thu, 30 Aug 2001, Michael E Brown wrote:
>
> > In reference to the ia64 ioctls:  I'm sorry, but disk access APIs that
> > don't allow access to the whole disk are what is broken. These ioctls
> > would not be necessary if you could actually write to the last sector of
> > an odd-sized disk. Have you read the comments surrounding this ioctl?
>
> Were people's heads on backwards when they wrote the ioctl?  Quite simply:
> if an ugly hack has to be put in place, put it in the right place.  In
> this case, it would have been *trivial* to put the UGLY hack in
> fs/block_dev.c and just make read/write transparently able to access the
> end of the disk.  No adding crap to the API, and certainly not risking
> truely unexpected disk io due to an incorrect ioctl.
>
> 		-ben
>
>

-- 
Michael Brown
Linux OS Development
Dell Computer Corp

  If each of us have one object, and we exchange them,
     then each of us still has one object.
  If each of us have one idea,   and we exchange them,
     then each of us now has two ideas.

Re: [PATCH] blkgetsize64 ioctl

[Ben LaHaise]

On Thu, 30 Aug 2001, Michael E Brown wrote:

> And your last point about risking unexpected disk-io due to an incorrect
> IOCTL, I would say that is a pretty unlikely in practice. First, I do
> parameter checking on what was passed to the IOCTL, and if things don't
> match, no io is done. Second, how likely is it that you a) call ioctl with
> a (disk) block device, b) pass the wrong ioctl, c) pass along enough data
> to pass the checks in the ioctl, and d) pass along a valid pointer to 512
> bytes of data to overwrite something?

e2fsprogs-1.23 on x86 does this.

		-ben

Re: [PATCH] blkgetsize64 ioctl

[Michael E Brown]

And your response to the rest of the points I raised would be?

I'm sorry about e2fsprogs. If I had known a bit better (this was my first
kernel patch), I would have added a magic number to the struct you pass
in, and that would have prevented this little bit of braindamage.
--
Michael

On Thu, 30 Aug 2001, Ben LaHaise wrote:

> On Thu, 30 Aug 2001, Michael E Brown wrote:
>
> > And your last point about risking unexpected disk-io due to an incorrect
> > IOCTL, I would say that is a pretty unlikely in practice. First, I do
> > parameter checking on what was passed to the IOCTL, and if things don't
> > match, no io is done. Second, how likely is it that you a) call ioctl with
> > a (disk) block device, b) pass the wrong ioctl, c) pass along enough data
> > to pass the checks in the ioctl, and d) pass along a valid pointer to 512
> > bytes of data to overwrite something?
>
> e2fsprogs-1.23 on x86 does this.
>
> 		-ben
>
>

-- 
Michael Brown
Linux OS Development
Dell Computer Corp

  If each of us have one object, and we exchange them,
     then each of us still has one object.
  If each of us have one idea,   and we exchange them,
     then each of us now has two ideas.

Re: [PATCH] blkgetsize64 ioctl

[Ben LaHaise]

On Thu, 30 Aug 2001, Michael E Brown wrote:

> And your response to the rest of the points I raised would be?

fs/block_dev.c is not a hot spot in the kernel.  It's been patched maybe 3
or 4 times over the last year, so I don't buy the argument that it would
have been difficult to maintain.  (Think if (offset >=
last_block_start(dev) return read_last_block(offset, buf, len);)

> I'm sorry about e2fsprogs. If I had known a bit better (this was my first
> kernel patch), I would have added a magic number to the struct you pass
> in, and that would have prevented this little bit of braindamage.

No, that's not what's got me miffed.  What is a problem here is that an
obvious next to use ioctl number in a *CORE* kernel api was used without
reserving it.  AND PEOPLE SHIPPED IT!  I for one don't go about shipping
new ABIs without reserving at least a placeholder for it in the main
kernel (or stating that the code is not bearing a fixed ABI).  If the
ioctl # was in the main kernel, this mess would never have happened even
with the accidental shipping of the patch in e2fsprogs.  I just hope
people will remember this in the future.  ABI compatibility is not that
hard if it's thought about.

		-ben

Re: [PATCH] blkgetsize64 ioctl

[Michael E Brown]

On Thu, 30 Aug 2001, Ben LaHaise wrote:

> No, that's not what's got me miffed.  What is a problem here is that an
> obvious next to use ioctl number in a *CORE* kernel api was used without
> reserving it.  AND PEOPLE SHIPPED IT!  I for one don't go about shipping
> new ABIs without reserving at least a placeholder for it in the main
> kernel (or stating that the code is not bearing a fixed ABI).  If the
> ioctl # was in the main kernel, this mess would never have happened even
> with the accidental shipping of the patch in e2fsprogs.  I just hope
> people will remember this in the future.  ABI compatibility is not that
> hard if it's thought about.

Ok. I can agree with that. As a junior kernel hacker, I did not know about
this issue, and I agree that somebody with a bit more experience should
have reserved the next number. Sorry :-(

But, since I've gone through this ioctl reservation conflict twice now
(e2fsprogs and something having to do with XFS), I think this is a more
general problem.

As a side note, a copy of the second edition of "Linux Device Drivers" I
am using doesn't mention this idea of reserving ioctl numbers.

-- 
Michael Brown
Linux OS Development
Dell Computer Corp

  If each of us have one object, and we exchange them,
     then each of us still has one object.
  If each of us have one idea,   and we exchange them,
     then each of us now has two ideas.

Re: [PATCH] blkgetsize64 ioctl

[Theodore Tso]

On Thu, Aug 30, 2001 at 01:12:07PM -0400, Ben LaHaise wrote:
> 
> No, that's not what's got me miffed.  What is a problem here is that an
> obvious next to use ioctl number in a *CORE* kernel api was used without
> reserving it.  AND PEOPLE SHIPPED IT!  I for one don't go about shipping
> new ABIs without reserving at least a placeholder for it in the main
> kernel (or stating that the code is not bearing a fixed ABI).  If the
> ioctl # was in the main kernel, this mess would never have happened even
> with the accidental shipping of the patch in e2fsprogs.  

... and for my part, I included the patch in e2fsprogs because Ben
sent me the patch, saying that people would want to test it, and I
assumed he had already reserved the ioctl in the kernel.  I should
have checked first....

							- Ted

Re: [PATCH] blkgetsize64 ioctl

[Alan Cox]

> On Thu, Aug 30, 2001 at 01:12:07PM -0400, Ben LaHaise wrote:
> > No, that's not what's got me miffed.  What is a problem here is that an
> > obvious next to use ioctl number in a *CORE* kernel api was used without
> > reserving it.  AND PEOPLE SHIPPED IT!  I for one don't go about shipping
> > new ABIs without reserving at least a placeholder for it in the main
> > kernel (or stating that the code is not bearing a fixed ABI).  If the
> > ioctl # was in the main kernel, this mess would never have happened even
> > with the accidental shipping of the patch in e2fsprogs.  
> 
> ... and for my part, I included the patch in e2fsprogs because Ben
> sent me the patch, saying that people would want to test it, and I
> assumed he had already reserved the ioctl in the kernel.  I should
> have checked first....

Follow the rule I use with Linus - never send proposed changes you dont
mean to be merged in a compilable form

On this subject I think it would be good to get the security() syscall
allocated now that folks are using the LSM framework for real stuff - even
the NSA stuff

Alan

Re: [PATCH] blkgetsize64 ioctl

[Michael E Brown]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1902 bytes --]

Alan,
	Here is a patch that reserves the 108 and 109 ioctl numbers for
get/set last sector. This patch has already been merged into the ia64
tree, and is currently necessary in order to support the EFI GPT
partitioning scheme on ia64. It is also in the Red Hat kernel tree. I had
assumed that somebody at Red Hat would have forwarded it to you.

	Please apply.

	If you would consider the rest of the patch that implements the
ioctls, I can send you that as well.
--
Michael


On Thu, 30 Aug 2001, Alan Cox wrote:

> > On Thu, Aug 30, 2001 at 01:12:07PM -0400, Ben LaHaise wrote:
> > > No, that's not what's got me miffed.  What is a problem here is that an
> > > obvious next to use ioctl number in a *CORE* kernel api was used without
> > > reserving it.  AND PEOPLE SHIPPED IT!  I for one don't go about shipping
> > > new ABIs without reserving at least a placeholder for it in the main
> > > kernel (or stating that the code is not bearing a fixed ABI).  If the
> > > ioctl # was in the main kernel, this mess would never have happened even
> > > with the accidental shipping of the patch in e2fsprogs.
> >
> > ... and for my part, I included the patch in e2fsprogs because Ben
> > sent me the patch, saying that people would want to test it, and I
> > assumed he had already reserved the ioctl in the kernel.  I should
> > have checked first....
>
> Follow the rule I use with Linus - never send proposed changes you dont
> mean to be merged in a compilable form
>
> On this subject I think it would be good to get the security() syscall
> allocated now that folks are using the LSM framework for real stuff - even
> the NSA stuff
>
> Alan
>

-- 
Michael Brown
Linux OS Development
Dell Computer Corp

  If each of us have one object, and we exchange them,
     then each of us still has one object.
  If each of us have one idea,   and we exchange them,
     then each of us now has two ideas.


[-- Attachment #2: patch-getlastsector --]
[-- Type: TEXT/PLAIN, Size: 572 bytes --]

diff -ruP linux/include/linux/fs.h linux-ioctl/include/linux/fs.h
--- linux/include/linux/fs.h	Tue Jan 30 01:24:56 2001
+++ linux-ioctl/include/linux/fs.h	Tue Feb 13 11:08:43 2001
@@ -180,6 +180,8 @@
 /* This was here just to show that the number is taken -
    probably all these _IO(0x12,*) ioctls should be moved to blkpg.h. */
 #endif
+#define BLKGETLASTSECT  _IO(0x12,108) /* get last sector of block device */
+#define BLKSETLASTSECT  _IO(0x12,109) /* get last sector of block device */
 
 
 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */

Re: [PATCH] blkgetsize64 ioctl

[Alan Cox]

> Alan,
> 	Here is a patch that reserves the 108 and 109 ioctl numbers for
> get/set last sector. This patch has already been merged into the ia64
> tree, and is currently necessary in order to support the EFI GPT
> partitioning scheme on ia64. It is also in the Red Hat kernel tree. I had
> assumed that somebody at Red Hat would have forwarded it to you.

Rejected. I still think this is an ugly evil hack and want no part in it.
Its definitely not a 2.4 core tree thing.

Alan

Re: [PATCH] blkgetsize64 ioctl

[Andries.Brouwer]

    From: Ben LaHaise
    ...

An interesting conversation, this.
A is blamed for making the terrible mistake of shipping an unreserved ioctl,
very stupid, because by accident B has made the minor mistake of shipping
an unreserved ioctl.
Maybe I misread something.

    From: Michael E Brown <michael_e_brown@dell.com>

    Alan,
        Here is a patch that reserves the 108 and 109 ioctl numbers for
    get/set last sector. This patch has already been merged into the ia64
    tree, and is currently necessary in order to support the EFI GPT
    partitioning scheme on ia64. It is also in the Red Hat kernel tree. I had
    assumed that somebody at Red Hat would have forwarded it to you.

Marking the numbers not-to-be-used is certainly a good idea.

Concerning the need for this particular nonsense - we have had this
discussion earlier this year, and also without any patches
one can read the last sector.  (Using some bad kludge, but still..)

One of the patches I have at ftp.kernel.org removes the entire problem -
one needs (1) a test in ll_rw_blk.c that uses not the size in 1024-byte blocks
but in 512-byte sectors, and (2) a set-blocksize ioctl.
And this latter is needed for other reasons as well.

Maybe I should resubmit some such patch fragments?

Andries


>   Content-Disposition: attachment; filename=patch-getlastsector-20010213
>
>   ZGlmZiAtcnVQIGxpbnV4L2luY2x1ZGUvbGludXgvZnMuaCBsaW51eC1pb2N0

Yes, indeed.

Re: [PATCH] blkgetsize64 ioctl

[Alan Cox]

> one needs (1) a test in ll_rw_blk.c that uses not the size in 1024-byte blocks
> but in 512-byte sectors, and (2) a set-blocksize ioctl.
> And this latter is needed for other reasons as well.
> 
> Maybe I should resubmit some such patch fragments?

If you can doit cleanly - yes

Re: [PATCH] blkgetsize64 ioctl

[Andries.Brouwer]

    From: Ben LaHaise <bcrl@redhat.com>

    The patch below reserves an ioctl for getting the size in blocks of a
    device as a long long instead of long as the old ioctl did.  The patch for
    this to e2fsprogs sneaked in a bit too early.  There is a conflict with
    the ia64 get/set sector ioctls, but I that's less common than e2fsprogs.


    +#define BLKGETSIZE64 _IO(0x12,109)    /* return device size (long long *arg) */

There are more conflicts with the numbering.

But more interestingly, I don't think we need the size in blocks.
We need the size in bytes.

(In 32-bit space we need the 8 or 9 or 10 bits extra provided by
counting blocks instead of bytes, but it causes quite a few problems.
In 64-bit space we can avoid all problems and work in bytes.
(Or even in bits.))

Andries

[PATCH] blkgetsize64 ioctl

[Ben LaHaise]

Hello,

The patch below reserves an ioctl for getting the size in blocks of a
device as a long long instead of long as the old ioctl did.  The patch for
this to e2fsprogs sneaked in a bit too early.  There is a conflict with
the ia64 get/set sector ioctls, but I that's less common than e2fsprogs.

		-ben

diff -urN /md0/kernels/2.4/v2.4.10-pre2/include/linux/fs.h work-v2.4.10-pre2/include/linux/fs.h
--- /md0/kernels/2.4/v2.4.10-pre2/include/linux/fs.h	Wed Aug 29 18:28:50 2001
+++ work-v2.4.10-pre2/include/linux/fs.h	Wed Aug 29 18:39:48 2001
@@ -166,7 +166,7 @@
 #define BLKROSET   _IO(0x12,93)	/* set device read-only (0 = read-write) */
 #define BLKROGET   _IO(0x12,94)	/* get read-only status (0 = read_write) */
 #define BLKRRPART  _IO(0x12,95)	/* re-read partition table */
-#define BLKGETSIZE _IO(0x12,96)	/* return device size */
+#define BLKGETSIZE _IO(0x12,96)	/* return device size (long *arg) */
 #define BLKFLSBUF  _IO(0x12,97)	/* flush buffer cache */
 #define BLKRASET   _IO(0x12,98)	/* Set read ahead for block device */
 #define BLKRAGET   _IO(0x12,99)	/* get current read ahead setting */
@@ -182,6 +182,7 @@
 /* This was here just to show that the number is taken -
    probably all these _IO(0x12,*) ioctls should be moved to blkpg.h. */
 #endif
+#define BLKGETSIZE64 _IO(0x12,109)	/* return device size (long long *arg) */


 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */

Re: [PATCH] blkgetsize64 ioctl

[David S. Miller]

   From: Ben LaHaise <bcrl@redhat.com>
   Date: Wed, 29 Aug 2001 18:45:20 -0400 (EDT)

   The patch below reserves an ioctl for getting the size in blocks of a
   device as a long long instead of long as the old ioctl did.  The patch for
   this to e2fsprogs sneaked in a bit too early.  There is a conflict with
   the ia64 get/set sector ioctls, but I that's less common than e2fsprogs.

Any problems with using "u64" or some other more strictly portable
type?  "long long" and other non-fixed sized types cause grief for
many dual-API platforms.

Later,
David S. Miller
davem@redhat.com

Re: [PATCH] blkgetsize64 ioctl

[Ben LaHaise]

On Wed, 29 Aug 2001, David S. Miller wrote:

> Any problems with using "u64" or some other more strictly portable
> type?  "long long" and other non-fixed sized types cause grief for
> many dual-API platforms.

That's entirely doable; I used long long because the existing API used
long.  In fact, thinking about it a bit more, it would be better to not
add the ioctl as _IO(0x12,109), but instead use 110 and reserve 108 and
109 as braindamage ioctl writeoff (except documented, unlike the current
silent usage of ioctls by ia64).  I hate ioctls and binary
incompatibilities.  Here's the modified patch (incompatible with
e2fsprogs 1.23, but not conflicting with ia64: ioctls that write to disk
are b0rken).

		-ben

diff -urN /md0/kernels/2.4/v2.4.10-pre2/include/linux/fs.h work-v2.4.10-pre2/include/linux/fs.h
--- /md0/kernels/2.4/v2.4.10-pre2/include/linux/fs.h	Wed Aug 29 18:28:50 2001
+++ work-v2.4.10-pre2/include/linux/fs.h	Wed Aug 29 20:14:58 2001
@@ -166,7 +166,7 @@
 #define BLKROSET   _IO(0x12,93)	/* set device read-only (0 = read-write) */
 #define BLKROGET   _IO(0x12,94)	/* get read-only status (0 = read_write) */
 #define BLKRRPART  _IO(0x12,95)	/* re-read partition table */
-#define BLKGETSIZE _IO(0x12,96)	/* return device size */
+#define BLKGETSIZE _IO(0x12,96)	/* return device size (long *arg) */
 #define BLKFLSBUF  _IO(0x12,97)	/* flush buffer cache */
 #define BLKRASET   _IO(0x12,98)	/* Set read ahead for block device */
 #define BLKRAGET   _IO(0x12,99)	/* get current read ahead setting */
@@ -182,6 +182,8 @@
 /* This was here just to show that the number is taken -
    probably all these _IO(0x12,*) ioctls should be moved to blkpg.h. */
 #endif
+/* _IO(0x12,108) and _IO(0x12,109) are reserved for binary compatibility */
+#define BLKGETSIZE64 _IO(0x12,110)	/* return device size (u64 *arg) */


 #define BMAP_IOCTL 1		/* obsolete - kept for compatibility */