Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

[Mark Peloquin]

On 10/03/2002 at 10:00 AM, Christoph Hellwig wrote:

> > +#ifdef __KERNEL__

> Nuke this.  kernel he3aders aren't for userspace anyway.

At one point, userspace did share this header file.
That hasn't been the case for a while, so this has
been removed.

> > +#define EVMS_CHECK_MEDIA_CHANGE         _IO(EVMS_MAJOR, >
EVMS_CHECK_MEDIA_CHANGE_NUMBER)
> > +
> > +#define EVMS_REVALIDATE_DISK_STRING     "EVMS_REVALIDATE_DISK"
> > +#define EVMS_REVALIDATE_DISK            _IO(EVMS_MAJOR,
EVMS_REVALIDATE_DISK_NUMBER)

> Can't you use normal revalidate/media change operations?

For most plugins, except the device manager, this operation
was nothing more than an exercise in routing the operation
requests to the underlying devices. Since this routing code
already exists in the ioctl interface, it seemed reasonable
to just reuse it for this purpose as well. As a result,
each plugin had to add 0 lines of code to support this.

> > +
> > +#define EVMS_OPEN_VOLUME_STRING         "EVMS_OPEN_VOLUME"
> > +#define EVMS_OPEN_VOLUME                _IO(EVMS_MAJOR,
EVMS_OPEN_VOLUME_NUMBER)
> > +
> > +#define EVMS_CLOSE_VOLUME_STRING        "EVMS_CLOSE_VOLUME"
> > +#define EVMS_CLOSE_VOLUME               _IO(EVMS_MAJOR,
EVMS_CLOSE_VOLUME_NUMBER)

> if you need open/close ioctl you got some abstraction wrong..

In EVMS, because of removable media devices, we chose to
open/close the underlying devices only when an actual
open/close is received for a volume residing on the
devices. This allows removable media devices to remain
unlocked until they are in use.

> > +/**
> > + * struct evms_sector_io_pkt - sector io ioctl packet definition
> > + * @disk_handle:   disk handle of target device
> > + * @io_flag:       0 = read, 1 = write
> > + * @starting_sector:     disk relative starting sector
> > + * @sector_count:  count of sectors
> > + * @buffer_address:      user buffer address
> > + * @status:        return operation status
> > + *
> > + * ioctl packet definition for EVMS_SECTOR_IO ioctl
> > + **/
> > +struct evms_sector_io_pkt {
> > + u64 disk_handle;
> > + s32 io_flag;
> > + u64 starting_sector;
> > + u64 sector_count;
> > + u8 *buffer_address;
> > + s32 status;
> > +};

> You don't use an ioctl to read into a user supplied buffer??

This ioctl is used by our userspace tools. EVMS stores
metadata at the very end of devices. This ioctl was
initially done to avoid the problem of "short writes"
to those last sectors. It also allows the core to
control which devices userspace can address.

> > +/**
> > + * struct evms_compute_csum_pkt - compute checksum ioctl packet
definition
> > + * @buffer_address:
> > + * @buffer_size:
> > + * @insum:
> > + * @outsum:
> > + * @status:
> > + *
> > + * ioctl packet definition for EVMS_COMPUTE_CSUM ioctl
> > + **/
> > +struct evms_compute_csum_pkt {
> > + u8 *buffer_address;
> > + s32 buffer_size;
> > + u32 insum;
> > + u32 outsum;
> > + s32 status;
> > +};

> An ioctl to compute a checksum??

MD uses checksum routines. Rather than duplicating
this code, we provided a method, for our usertools,
of using the kernel routines, especially considering
there is multiple methods based on various
characteristics.

Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

[Christoph Hellwig]

On Mon, Oct 07, 2002 at 11:43:30AM -0500, Mark Peloquin wrote:
> > Can't you use normal revalidate/media change operations?
> 
> For most plugins, except the device manager, this operation
> was nothing more than an exercise in routing the operation
> requests to the underlying devices. Since this routing code
> already exists in the ioctl interface, it seemed reasonable
> to just reuse it for this purpose as well. As a result,
> each plugin had to add 0 lines of code to support this.

I don't think that basing kernel internal interfaces on ioctl is
a smart idea.  Just add another function pionter to your operations
vector for every operation you want supported on volumes.

That way the uppermost layer can do the translation from the ugly
ioctl interface (or a future nice fs-based interface) to the operations
once instead of carrying the user-defined syscalls (aka ioctls)
around through your whole architecture.

> > if you need open/close ioctl you got some abstraction wrong..
> 
> In EVMS, because of removable media devices, we chose to
> open/close the underlying devices only when an actual
> open/close is received for a volume residing on the
> devices. This allows removable media devices to remain
> unlocked until they are in use.

Again, why can't you have open/close _methods_ for it instead of abusing
the ioctl interface.  It would be even nicer if you could extend
the block device operations with your new volume-mangment methods
instead of adding another API for blockdevices/disks outside the higher
level kernel code.

> > You don't use an ioctl to read into a user supplied buffer??
> 
> This ioctl is used by our userspace tools. EVMS stores
> metadata at the very end of devices. This ioctl was
> initially done to avoid the problem of "short writes"
> to those last sectors. It also allows the core to
> control which devices userspace can address.

Umm.  I'd suggest you fix the read/write access instead of duplicating those
two core unix syscalls in an messy ioctl API.

> > An ioctl to compute a checksum??
> 
> MD uses checksum routines. Rather than duplicating
> this code, we provided a method, for our usertools,
> of using the kernel routines, especially considering
> there is multiple methods based on various
> characteristics.

Based on that argumentation we should make memcpy a syscall..

Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

[Alexander Viro]

On Mon, 7 Oct 2002, Christoph Hellwig wrote:

> I don't think that basing kernel internal interfaces on ioctl is
> a smart idea.  Just add another function pionter to your operations
> vector for every operation you want supported on volumes.

s/every/& generic/.  Other than that, seconded.  BTW, one of the pending
changes is taking the last more or less generic ioctl (HDIO_GETGEO) into
a separate method...

->ioctl() is for driver-specific crud; stuff that won't be used by
any generic application.  "Make a cuckoo jump out of drive and sing
'1000 bottles of beer'" is a valid ioctl.  "Get drive size" isn't.

Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

[Christoph Hellwig]

On Mon, Oct 07, 2002 at 01:50:00PM -0400, Alexander Viro wrote:
> 
> 
> On Mon, 7 Oct 2002, Christoph Hellwig wrote:
> 
> > I don't think that basing kernel internal interfaces on ioctl is
> > a smart idea.  Just add another function pionter to your operations
> > vector for every operation you want supported on volumes.
> 
> s/every/& generic/.  Other than that, seconded.  BTW, one of the pending
> changes is taking the last more or less generic ioctl (HDIO_GETGEO) into
> a separate method...

Well, I implied generic ioctl as the EVMS "API" (aka collection of random
ioctls) is about that generic interface.

Re: [Evms-devel] Re: [PATCH] EVMS core 3/4: evms_ioctl.h

[Mark Peloquin]

On 10/03/2002 at 6:49 PM, Andi Kleen wrote:
> I think you have some security holes in there:

 > +parms.buffer_address  = (u8 *)uvirt_to_kernel(parms32.buffer_address);
 > [...]
 > +set_fs(KERNEL_DS);
 > +rc = sys_ioctl(fd, kcmd, (unsigned long)karg);
 > +set_fs(old_fs);


> parms32.buffer_address comes from user space. With the set_fs you turn
> off all access checking. Surely when whatever sits at the bottom of
> sys_ioctl accesses it it'll use copy_from/to_user and it will do an
> unchecked reference of a user supplied pointer, allowing it to read/write
> all memory.

> Same bug is present in more functions.

> The rule is: when you do set_fs(KERNEL_DS) you have to copy all user
supplied
> pointers before it.

Yes, we became aware of this while working on sparc64 and have
coded the appropriate copy *before* set_fs(KERNEL_DS).
Unfortunately, that code didn't make it into CVS yet.
This will be fixed ASAP.

Thanks for pointing it out.
Mark