[dm-devel] Re: [PATCH] User-configurable HDIO_GETGEO for dm volumes

[Darrick J. Wong]

Andrew Morton wrote:

> Normally kernel functions return zero on success.  So that they can return
> negative errno on failure.  Is that appropriate here?  Just propagate the
> dm_table_get_geometry() return value?

Fair enough, I've changed the functions to return 0 on success.  See the 
patches attached to the end.

> That's brave - we take the hd_geometry straight from userspace without
> checking anything?

My original approach didn't work anyway; libdevmapper thinks that a 
target message is a string and would stop copying at the first null it 
saw.  Since you're also concerned about being locked into a particular 
hd_geometry structure layout, I respun the patch so that dmsetup passes 
a string to the dm configuration code; now dm performs some basic 
range/sanity checks.  However, the patch doesn't check that the CHS 
values make sense or are even close to the real disk size; if somebody 
in userspace wants to configure a 150G dm device to have the same 
geometry as a 360K floppy disk, so be it.  Geometries seem to be rather 
inaccurate anyway.

Or, were you worried that I'm dereferencing a userspace pointer in the 
kernel?  The code that calls _setgeo handles that properly.

> Will this code dtrt if userspace is 32-bit and the kernel is 64-bit?

There shouldn't be any 32/64 mis-match issues with passing a string.  If 
one tries to pass too-large values, -EINVAL is returned.

> > struct hd_geometry looks like something which different compilers could lay
> out differently, perhaps even different gcc versions.  We're relying upon
> the userspace representation being identical to the kernel's
> representation.
> 
> It means that struct hd_geometry becomes part of the kernel ABI.  We can
> never again change it and neither we (nor the compiler) can ever change its
> layout.  That's dangerous.  I'd suggest that you not use hd_geometry in
> this way (unless we're already using it that way, which might be the case).

hd_geometry is already part of the kernel ABI because the HDIO_GETGEO 
ioctl takes a pointer to a struct hd_geometry in userspace and fills it 
out.  Though, now that I've changed the setgeo half to pass a string 
around, I suppose the point is partially moot.

> It'd be better to use some carefully laid-out and documented structure
> which is private to DM and which is designed for future-compatibility and
> for user<->kernel communication.

--D

Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: device-mapper-geometry_2.patch
Type: text/x-patch
Size: 3645 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20060216/ac38e82a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dm-getgeo_2.patch
Type: text/x-patch
Size: 6343 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20060216/ac38e82a/attachment-0001.bin>