Re: [RFC] [PATCH] DRM TTM Memory Manager patch

From: Thomas Hellström
Date: Mon Apr 30 2007 - 20:31:15 EST


Dave Airlie wrote:


Most likely in doxygen as that is what Mesa uses and the intersection
of developers is higher in that area, I'll take it as a task to try
and kerneldoc the drm at some stage..

- what's with the /proc interface? Don't add new proc code for
non-process related things. This should all go into sysfs
somewhere. And yes, I know /proc/dri/ is there today, but don't add
new stuff please.


Well we should move all that stuff to sysfs, but we have all the
infrastructure for publishing this stuff under /proc/dri and adding
new files doesn't take a major amount, as much as I appreciate sysfs,
it isn't suitable for this sort of information dump, the whole one
value per file is quite useless to provide this sort of information
which is uni-directional for users to send to us for debugging without
have to install some special tool to join all the values into one
place.. and I don't think drmfs is the answer either... or maybe it
is....

- struct drm_bo_arg can't use an int or unsigned, as it crosses the
userspace/kernelspace boundry, use the proper types for all values
in those types of structures (__u32, etc.)


int is defined, unsigned I'm not so sure about, the drm user space
interface is usually specified in non-system specific types so the
drm.h file is consistent across systems, so we would probably have to
use uint32_t which other people have objected to, but I'd rather use
uint32_t than unspecified types..

- there doesn't seem to be any validity checking for the arguments
passed into this new ioctl. Possibly that's just the way the rest
of the dri interface is, which is scary, but with the memory stuff,
you really should check things properly...


Okay this needs fixing, we do check most ioctls args, the main thing
passed in are handles and these are all looked up in the hash table,
it may not be so obvious, also most of the ioctls are probably going
to end up root or DRM master only, I'd like do an ioctl fuzzer at some
stage, I'd suspect a lot more then the dri would be oopsable with
permissions...

Thanks,
Dave.

I agree with Dave for most if not all of the above.
Typing, for example unsigned / uint32 vs u32, __u32 is very easily fixable once we decide on a clear way to go to keep (if we want to keep) compatibility with *bsd.

For the IOCTL checking, as Dave states, most invalid data will be trapped in hash lookups and
checks in the buffer validation system, but probably far from all. A fuzzer would be a nice tool to trap the exceptions.

/Thomas

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/