Re: LVM / General IOCTL question

On Fri, 14 Nov 2003, Peter Jones wrote:

> Of course, it assumes that they are.  I'd like to see an example of where
> they're not -- they're certainly supposed to be, and sparc64 isn't the
> only arch that uses this mechanism.

By definition, one can use whatever 'identifying code' (the 'f' in your
example below, that one wants. There is no registration authority to
assign these numbers.

> > So for example 0x4004fe98 (used by LVM) might be used by another device.
> > In the normal case, there is no problem since the file handle allows the
> > device/file/socket driver to handle the ioctl processing.  But the
> > translation table can't properly handle the case were there are two
> > different devices that use the same ioctl.
>
> Two different IOCTLs shouldn't have the same number.  That's why things
> like all the _IOW()/_IOR()/_IO() macros exist, which take an ID number for
> the code you're looking at, a number (which needs to be unique within the
> namespace to which that first ID refers), and a type.
>
> You'll find that many IOCTL names are defined something along the lines
> of:
>
> #define EXT2_IOC_GETFLAGS               _IOR('f', 1, long)
> #define EXT2_IOC_SETFLAGS               _IOW('f', 2, long)
> #define EXT2_IOC_GETVERSION             _IOR('v', 1, long)
> #define EXT2_IOC_SETVERSION             _IOW('v', 2, long)

Yep. But they don't have to be defined this way. They could be defined
however it is convenient to the driver that is going to interpret them.

> So in effect, each IOCTL is defined with a different combination of
> type+number, direction, and arguments taken.

I know.  But this is for the purpose of preventing collisions when ioctls
are made on the wrong device. It is not for the purpose of making all
ioctls globably unique.  That goal can't be achieved except in cases where
there are less than 256 drivers.

> > If the IOCTL number was unique, we would not need to pass a file
> > descriptor to the ioctl system call.
>
> No, that's just plain wrong.  If I do two opens:
>
> fd0 = open("/dev/hda",O_RDWR);
> fd1 = open("/dev/hdb",O_RDWR);
>
> and I want to do ioctls to change device parameters, I need the IOCTL
> number to tell it what function I'm doing, but I need the fd to tell it
> which device to do it to.

You are missing the point. Here is a better example:

fd0 = open("/dev/hda",O_RDWR);
fd1 = open("/dev/sdb",O_RDWR);

The same IOCTLs may be implemented for both devices, but the drivers may
do different things to implement those identical IOCTLs.  The global
translation table scheme cannot handle that when they are not
'compatible'.  It can only handle this case if both ioctls are
'compatible', because in this case it is just passed to the correct
driver.  There are certainly examples in the scsi and ide drivers of the
same ioctls implemented differently.

If either IOCTL is not "compatible", then the translation table scheme has
a problem. You have to modify the driver code, and then call it
"compatible". Or course, it would be better to do the same thing in every
case, rather than spread bits and pieces of (say) the LVM code around the
kernel.

> > The unqifying components of IOCTL are meant to minimize the damage done
> > when the wrong device file is used.  In some hardware devices, sending
> > random IOCTLs results in relays being altered, and can result in damage
> > and even fire.
>
> So, uh, if some piece of an ioctl handler fails to check if the ioctls
> issued actually applicable to the device, then it's completely and totally
> broken and won't ever work anyway.  Don't do that.

No. Its not a matter of "failure to check".  If the wrong device file is
used, chances are that the uniqified ioctls called won't be implemented on
that wrong device, and then nothing bad will happen.

Consider the case if all IOCTLs were numbered from, say, 1,2,3,4. (as they
were originally)  Then who knows what would happen when you called ioctl 1
on the wrong device, followed by ioctl 3. Something bad probably.

Solution: We have a whole int, lets try to get different devices to use
different parts of the range.  The macros were constructed to do this,
making it unlikely that a wrong device will implement the same ioctl
numbers for some other device.  But, as there are only 8 bits of 'id', and
more than 256 devices/files/sockets, it cannot be unique except on small
systems.

> > Kernel pointers shouldn't be passed to userland.
>
> No, the _other_ way.  When userland passes a structure in, this layer
> translates the structure such that the pointers reflect addresses the
> kernel understands, rather than the user's 32-bit pointers.

Umm. This is literally correct, but not what I was referring to.  Look at
the code for VG_STATUS in do_lvm_handler and in lvm.c.  The userland
called the ioctl to obtain information about volume groups from the
kernel. The kernel structure which is typically sent back contains several
ints, followed by an array of pointers (kernel memory addresses), followed
by vg_uuid.  These are the pointers that I'm talking about.  The problem
in this case is that the kernel structure has 64 bit pointers, while the
userland structure is expecting 32 bit pointers. The code in do_lvm_ioctl
allocates memory, makes a copy of the kernel structure, and copies the
vg_uuid to the place it would be if the pointers were 32bits, and then
copies this out to userland.  Then it frees up the memory it allocated.

*These* pointers shouldn't be passed back to userland in the first place.
Indeed, the do_lvm_ioctl code for VG_STATUS just clobbers them.
Apparently, the userland is only interested in the ints and the vg_uuid.
If so, that's all that should be sent back.

> Convince davem on [email protected] or on lkml .  I'm really not
> interested in rewriting big chunks of infrastructure because you don't
> like something, especially when it's not in code anybody here maintains,
> and it works.

Ok. Sorry to bother you with this.

> > The other alternative is to fix the driver code directly.  There is no
> > reason that, for example, the lvm module/driver (lvm.c) ioctl handler
> > can't do what's in do_lvm_ioctl.  This would be far more portable.
>
> Portable?  You scare me.

I would hope I don't scare you. Putting this code, which isn't machine
specific, and is (or will be)  applicable on other architectures for which
sizeof (void *) != sizeof (int), into the sparc64 specific code certainly
isn't portable.

		--Dean

_______________________________________________
Aurora-sparc-devel mailing list
[email protected]
http://lists.auroralinux.org/mailman/listinfo/aurora-sparc-devel
Aurora FAQ: http://www.ecs.soton.ac.uk/~mas01r/aurorafaq.html