Re: LVM / General IOCTL question

On Fri, 14 Nov 2003, Dean Anderson wrote:

> On Fri, 14 Nov 2003, Peter Jones wrote:
> 
> > > My real question is, after searching this out, I find a whole bunch of
> > > ioctl translation stuff in the module independent parts of the kernel.  I
> > > see some wrong assumptions in this (mainly that IOCTL numbers are not
> > > globally unique)
> >
> > Where does it make that assumption?
> 
> In the translation table, which has an entry for each IOCTL, keyed only
> by IOCTL number. If the IOCTL numbers are reused by different devices
> (which they can be), then there is no way to determine which device to
> do the translation for. A unique mapping is (fd[ eg device/file/socket],
> IOCTL). The IOCTL numbers may not be unique.

Oh, I understood you as saying that it assumes IOCTL numbers are not 
globally unique.

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.

> 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)

And then (in asm-sparc64/ioctl.h):

#define _IOC_NRBITS      8
#define _IOC_TYPEBITS    8
#define _IOC_SIZEBITS   13      /* Actually 14, see below. */
#define _IOC_DIRBITS     3
                                                                                
#define _IOC_NRMASK      ((1 << _IOC_NRBITS)-1)
#define _IOC_TYPEMASK    ((1 << _IOC_TYPEBITS)-1)
#define _IOC_SIZEMASK    ((1 << _IOC_SIZEBITS)-1)
#define _IOC_XSIZEMASK   ((1 << (_IOC_SIZEBITS+1))-1)
#define _IOC_DIRMASK     ((1 << _IOC_DIRBITS)-1)
                                                                                
#define _IOC_NRSHIFT     0
#define _IOC_TYPESHIFT   (_IOC_NRSHIFT + _IOC_NRBITS)
#define _IOC_SIZESHIFT   (_IOC_TYPESHIFT + _IOC_TYPEBITS)
#define _IOC_DIRSHIFT    (_IOC_SIZESHIFT + _IOC_SIZEBITS)

#define _IOC_READ        2U

#define _IOC(dir,type,nr,size) \
        (((dir)  << _IOC_DIRSHIFT) | \
         ((type) << _IOC_TYPESHIFT) | \
         ((nr)   << _IOC_NRSHIFT) | \
         ((size) << _IOC_SIZESHIFT))

#define _IOR(type,nr,size)  _IOC(_IOC_READ,(type),(nr),sizeof(size))

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

> 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.

> 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.

> > > This seems to be such a terrible clusterf*ck, creating a lot of update
> > > contention on a ioctl32.c. But of course, we'll also ignore/acknowledge
> > > that sending pointers out of the kernel is probably also a clusterf*ck
> > > waiting to happen.
> >
> > Not just a clusterfuck.  You actually _can't_ do it, because the pointers
> > aren't the same as userland's.
> 
> 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.

> > It makes it possible
> 
> It is certainly not the only solution. It is also possible to change the
> kernel/userland interface structures to not contain/pass back pointers
> (they are no good to anyone anyway, as they can't be trustably passed
> back). Then there is no problem if sizeof(void *) changes.  This is the
> cleanest solution.

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.

> 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.

> Both of these alternatives seem to be much better than this translation
> table scheme.

Again, make a patch to do it however you'd like, and convince davem (and 
now since the code is shared with ppc64 in 2.6, the ppc guys and Linus as 
well) that it should be applied.

-- 
        Peter

I hope you know that this will go on your permanent record.

_______________________________________________
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