lkml.org 
[lkml]   [2010]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] p9auth: add p9auth driver
On Tue, Apr 20, 2010 at 08:29:08PM -0500, Serge E. Hallyn wrote:
> This is a driver that adds Plan 9 style capability device
> implementation. See Documentation/p9auth.txt for a description
> of how to use this.

Hm, you didn't originally write this driver, so it would be good to get
some original authorship information in here to keep everything correct,
right?

> Documentation/p9auth.txt | 47 ++++
> drivers/char/Kconfig | 2 +
> drivers/char/Makefile | 2 +
> drivers/char/p9auth/Kconfig | 9 +
> drivers/char/p9auth/Makefile | 1 +
> drivers/char/p9auth/p9auth.c | 517 ++++++++++++++++++++++++++++++++++++++++++

Is this code really ready for drivers/char/? What has changed in it
that makes it ok to move out of the staging tree?

And who is going to maintain it? You? Or someone else?

> 6 files changed, 578 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/p9auth.txt
> create mode 100644 drivers/char/p9auth/Kconfig
> create mode 100644 drivers/char/p9auth/Makefile
> create mode 100644 drivers/char/p9auth/p9auth.c
>
> diff --git a/Documentation/p9auth.txt b/Documentation/p9auth.txt
> new file mode 100644
> index 0000000..14a69d8
> --- /dev/null
> +++ b/Documentation/p9auth.txt
> @@ -0,0 +1,47 @@
> +The p9auth device driver implements a plan-9 factotum-like
> +capability API. Tasks which are privileged (authorized by
> +possession of the CAP_GRANT_ID privilege (POSIX capability))
> +can write new capabilities to /dev/caphash. The kernel then
> +stores these until a task uses them by writing to the
> +/dev/capuse device. Each capability represents the ability
> +for a task running as userid X to switch to userid Y and
> +some set of groups. Each capability may be used only once,
> +and unused capabilities are cleared after two minutes.
> +
> +The following examples shows how to use the API. Shell 1
> +contains a privileged root shell. Shell 2 contains an
> +unprivileged shell as user 501 in the same user namespace. If
> +not already done, the privileged shell should create the p9auth
> +devices:
> +
> + majfile=/sys/module/p9auth/parameters/cap_major
> + minfile=/sys/module/p9auth/parameters/cap_minor
> + maj=`cat $majfile`
> + mknod /dev/caphash c $maj 0
> + min=`cat $minfile`
> + mknod /dev/capuse c $maj 1
> + chmod ugo+w /dev/capuse

That is incorrect, you don't need the cap_major/minor files at all, the
device node should be automatically created for you, right?

And do you really want to do all of this control through a device node?
Why?

> +DEFINE_MUTEX(cap_mutex); /* TODO fix up the locking one day */

One might hope that today would be that day...

Also, please run this through sparse. This is a variable that doesn't
need to be global.

> +struct cap_dev {
> + struct cdev cdev;
> +};

Do you really need to do it this way? A cdev for a single device node?
That's major overkill.

> +static int cap_major = CAP_MAJOR;
> +static int cap_minor;

Just always use a dynamic misc device, you never need a whole major for
this.

> +module_param(cap_major, int, S_IRUGO);
> +module_param(cap_minor, int, S_IRUGO);

Can be removed.

> +MODULE_AUTHOR("Ashwin Ganti");

Hm, who is going to maintain this, you, or Ashwin?

> +static void hexdump(unsigned char *buf, unsigned int len)
> +{
> + while (len--)
> + printk(KERN_DEBUG "%02x", *buf++);
> + printk(KERN_DEBUG "\n");
> +}

We have a built-in function for this already.

Oh, this function is also incorrect, which is a good reason to use the
built-in ones.

I'm going to stop now, this doesn't belong in drivers/char/ yet, it
needs work...

thanks,

greg k-h

> +/*
> + * read an entry. For now it is
> + * source_user@target_user@rand
> + * Next it will become
> + * source_user@target_user@target_group@numgroups@grp1..@grpn@rand
> + */

Hm, wait, one more.... The kernel/user api is going to change some time
in the future? Please fix this now before it gets merged.

\
 
 \ /
  Last update: 2010-04-21 05:07    [from the cache]
©2003-2011 Jasper Spaans