Correct parameter size for BLKSSZGET ioctl.

Correct parameter size for BLKSSZGET ioctl.

[Jason Cipriani]

In blkdiscard in util-linux, at least since version 2.23, the
following code is used to retrieve a device's physical sector size:

  uint64_t secsize;
  ioctl(fd, BLKSSZGET, &secsize);

On my machine (Ubuntu 12.04 -- 3.2.0-55-generic-pae #85-Ubuntu SMP Wed
Oct 2 14:03:15 UTC 2013 i686 i686 i386 GNU/Linux) this yields
incorrect results as it seems a 32-bit int is expected, this causes
subsequent sector alignment calculations in blkdiscard to be
incorrect, which in turn causes blkdiscards trim ioctl's to fail in
certain situations (or even worse, to trim the wrong blocks).

I have seen BLKSSZGET implemented in two places. In block/ioctl.c it
is implemented using put_int
(http://lxr.free-electrons.com/source/block/ioctl.c#L365) which works
on an "int" (which doesn't actually have a fixed size -- it is defined
by the particular C compiler). In block/compat_ioctl.c it is
implemented using compat_put_int, which ultimately operates on a
compat_int_t, which, for all platforms I looked at, is explicitly
defined to be a 32-bit type (e.g. "typedef s32 compat_int_t").

My goal is to determine if blkdiscard is incorrectly using a uint64_t,
or if the Ubuntu kernel is incorrectly using some 32-bit type.
Therefore I would like to know precisely what parameter ioctl 0x1268
(BLKSSZGET) is specified to take.

My question, then, is where is this specified? Currently I have only
found scattered implementations (many using the vague "int") of
ioctl.c and compat_ioctl.c, uncommented header declarations (e.g.
fs.h), and anecdotal evidence and claims.

In other words: What is the parameter size for BLKSSZGET and, more
importantly, *how do you know that*? Is blkdiscard broken, or is my
kernel's implementation broken? If I wrote a device driver that
supported this ioctl, what data type would I use? Surely at some point
in the past somebody decided that 0x1268 would retrieve the physical
sector size of a device, and documented that somewhere.

Please CC me on replies; I am not subscribed to this list.

Thanks,
Jason

Re: Correct parameter size for BLKSSZGET ioctl.

[Theodore Ts'o]

On Fri, Nov 01, 2013 at 08:29:26PM -0400, Jason Cipriani wrote:
> In blkdiscard in util-linux, at least since version 2.23, the
> following code is used to retrieve a device's physical sector size:
> 
>   uint64_t secsize;
>   ioctl(fd, BLKSSZGET, &secsize);
> 
> On my machine (Ubuntu 12.04 -- 3.2.0-55-generic-pae #85-Ubuntu SMP Wed
> Oct 2 14:03:15 UTC 2013 i686 i686 i386 GNU/Linux) this yields
> incorrect results as it seems a 32-bit int is expected, this causes
> subsequent sector alignment calculations in blkdiscard to be
> incorrect, which in turn causes blkdiscards trim ioctl's to fail in
> certain situations (or even worse, to trim the wrong blocks).

BLKSSZGET returns an int.  If you look at the sources of util-linux
v2.23, you'll see it passes an int to BLKSSZGET in 

	sys-utils/blkdiscard.c
	lib/blkdev.c

E2fsprogs also expects BLKSSZGET to return an int, and if you look at
the kernel sources, it very clearly returns an int.

The one place it doesn't is in sys-utils/blkdiscard.c, where as you
have noted, it is passing in a uint64 to BLKSSZGET.  This looks like
it's a bug in sys-util/blkdiscard.c.

I'll send a proposed patch in the next e-mail message.

     	    	     	      	  - Ted

Re: Correct parameter size for BLKSSZGET ioctl.

[Jason Cipriani]

On Sat, Nov 2, 2013 at 7:44 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Nov 01, 2013 at 08:29:26PM -0400, Jason Cipriani wrote:
> > In blkdiscard in util-linux, at least since version 2.23, the
> > following code is used to retrieve a device's physical sector size:
> >
> >   uint64_t secsize;
> >   ioctl(fd, BLKSSZGET, &secsize);
> >
> > On my machine (Ubuntu 12.04 -- 3.2.0-55-generic-pae #85-Ubuntu SMP Wed
> > Oct 2 14:03:15 UTC 2013 i686 i686 i386 GNU/Linux) this yields
> > incorrect results as it seems a 32-bit int is expected, this causes
> > subsequent sector alignment calculations in blkdiscard to be
> > incorrect, which in turn causes blkdiscards trim ioctl's to fail in
> > certain situations (or even worse, to trim the wrong blocks).
>
> BLKSSZGET returns an int.  If you look at the sources of util-linux
> v2.23, you'll see it passes an int to BLKSSZGET in
>
>         sys-utils/blkdiscard.c
>         lib/blkdev.c
>
> E2fsprogs also expects BLKSSZGET to return an int, and if you look at
> the kernel sources, it very clearly returns an int.
>
> The one place it doesn't is in sys-utils/blkdiscard.c, where as you
> have noted, it is passing in a uint64 to BLKSSZGET.  This looks like
> it's a bug in sys-util/blkdiscard.c.
>
> I'll send a proposed patch in the next e-mail message.

Thank you for submitting that patch.

There was a bigger question hidden behind the context there that I'm
still wondering about: Are these ioctl interfaces specified and
documented somewhere? From what I've seen, and from your response, the
implication is that the kernel source *is* the specification, and not
document exists that the kernel is expected to comply with; is this
the case?

Secondly, would it not make sense to change all ints in all public
kernel interfaces to data types with a well-defined, machine- and
(mostly) compiler-independent size, e.g. int32_t (or whatever)? On one
hand, nothing seems particularly broken, per se, but on the other,
"int" is vaguely defined and it is arguably only by chance (albeit a
strong chance) that everything works (e.g. compilers used to build
applications agree with compilers used to build the kernel).

Please CC me on replies, I am not subscribed to this list.

Thanks again,
Jason

Re: Correct parameter size for BLKSSZGET ioctl.

[Theodore Ts'o]

On Sun, Nov 03, 2013 at 02:07:56PM -0500, Jason Cipriani wrote:
> 
> There was a bigger question hidden behind the context there that I'm
> still wondering about: Are these ioctl interfaces specified and
> documented somewhere? From what I've seen, and from your response, the
> implication is that the kernel source *is* the specification, and not
> document exists that the kernel is expected to comply with; is this
> the case?

The kernel source is the specification.  Some of these ioctl are
documented as part of the linux man pages, for which the project home
page is here:

     https://www.kernel.org/doc/man-pages/

However, these document existing practice; if there is a discrepancy
between what is in the kernel has implemented and the Linux man pages,
it is the Linux man pages which are buggy and which will be changed.
That is man pages are descriptive, not perscriptive.

> Secondly, would it not make sense to change all ints in all public
> kernel interfaces to data types with a well-defined, machine- and
> (mostly) compiler-independent size, e.g. int32_t (or whatever)? On one
> hand, nothing seems particularly broken, per se, but on the other,
> "int" is vaguely defined and it is arguably only by chance (albeit a
> strong chance) that everything works (e.g. compilers used to build
> applications agree with compilers used to build the kernel).

Changing the parameters would result in an ABI change, which would
break programs.  So in general, it's generally not worth making these
changes.

For newer ioctls, in general we'll try to use specifically sized
types, mainly so that we don't need to do compat_ioctl handling to
support 32-bit binaries on a 64-bit kernel, but in general, it's
generally not worth changing existing ioctls.
						- Ted

Re: Correct parameter size for BLKSSZGET ioctl.

[Rob Landley]

On 11/03/2013 01:43:14 PM, Theodore Ts'o wrote:
> On Sun, Nov 03, 2013 at 02:07:56PM -0500, Jason Cipriani wrote:
> >
> > There was a bigger question hidden behind the context there that I'm
> > still wondering about: Are these ioctl interfaces specified and
> > documented somewhere? From what I've seen, and from your response,  
> the
> > implication is that the kernel source *is* the specification, and  
> not
> > document exists that the kernel is expected to comply with; is this
> > the case?
> 
> The kernel source is the specification.  Some of these ioctl are
> documented as part of the linux man pages, for which the project home
> page is here:
> 
>      https://www.kernel.org/doc/man-pages/

Specifically "man ioctl_list"

> However, these document existing practice; if there is a discrepancy
> between what is in the kernel has implemented and the Linux man pages,
> it is the Linux man pages which are buggy and which will be changed.
> That is man pages are descriptive, not perscriptive.

Although if something obvious is missing or wrong, it's nice to ping  
Michael Kerrisk so he can update it.

> > Secondly, would it not make sense to change all ints in all public
> > kernel interfaces to data types with a well-defined, machine- and
> > (mostly) compiler-independent size, e.g. int32_t (or whatever)?

Allow me to introduce you to the LP64 specification, which Linux, BSD,  
and MacOS X all adhere to:

http://www.unix.org/whitepapers/64bit.html
http://www.unix.org/version2/whatsnew/lp64_wp.html

It says that char is 8 bits, short is 16 bits, int is 32 bits, and long  
is the same size as a pointer (32 bits on 32 bit platforms, 64 bits on  
64 bit platforms). There's a de-facto "long long is 64 bits" assumption  
in fairly widespread use, but it's not explicit in the standard.

(Welcome to the 21st century, where we know what sizes our data types  
are even _without_ c99.)

Rob

P.S. The insane legacy reasons that Windows does _not_ adhere to this  
standard are documented at:

http://blogs.msdn.com/oldnewthing/archive/2005/01/31/363790.aspx

(So if you're wondering why qemu has "pointer sized int" typedefs in  
its' source, it's because they care about running on windows. And even  
if you did care about that, you can still rely on the sizes of char,  
short, and int.)