lkml.org 
[lkml]   [2007]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC]selinux: Improving SELinux read/write performance

    On Thu, 13 Sep 2007 08:58:32 -0400
    Stephen Smalley wrote:
    > On Wed, 2007-09-12 at 17:51 +0900, Yuichi Nakamura wrote:
    <snip>
    > Thanks, a few comments below.
    Thanks for comments!

    > >
    > > * Description of patch
    > > This patch improves performance of read/write in SELinux.
    > > It improves performance by skipping permission check in
    > > selinux_file_permission. Permission is only checked when
    > > sid change or policy load is detected after file open.
    > > To detect sid change, new LSM hook securiy_dentry_open is added.
    >
    > I think I'd reword this a little, e.g.
    >
    > It reduces the selinux overhead on read/write by only revalidating
    > permissions in selinux_file_permission if the task or inode labels have
    > changed or the policy has changed since the open-time check. A new LSM
    > hook, security_dentry_open, is added to capture the necessary state at
    > open time to allow this optimization.
    I see, I will use that.

    > >
    > > Signed-off-by: Yuichi Nakamura<ynakam@hitachisoft.jp>
    > > ---
    > > fs/open.c | 5 ++++
    > > include/linux/security.h | 16 ++++++++++++++
    > > security/dummy.c | 6 +++++
    > > security/selinux/avc.c | 5 ++++
    > > security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++++++++++-
    > > security/selinux/include/avc.h | 2 +
    > > security/selinux/include/objsec.h | 2 +
    > > 7 files changed, 78 insertions(+), 1 deletion(-)
    > <snip>
    > > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/security/selinux/hooks.c linux-2.6.22/security/selinux/hooks.c
    > > --- linux-2.6.22.orig/security/selinux/hooks.c 2007-07-09 08:32:17.000000000 +0900
    > > +++ linux-2.6.22/security/selinux/hooks.c 2007-09-12 08:42:49.000000000 +0900
    > > @@ -80,6 +82,7 @@
    > >
    > > #define XATTR_SELINUX_SUFFIX "selinux"
    > > #define XATTR_NAME_SELINUX XATTR_SECURITY_PREFIX XATTR_SELINUX_SUFFIX
    > > +#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
    >
    > Leftover from prior version of the patch, no longer needed.
    Fixed.

    >
    > <snip>
    > > @@ -2715,6 +2737,23 @@ static int selinux_file_receive(struct f
    > > return file_has_perm(current, file, file_to_av(file));
    > > }
    > >
    > > +static int selinux_dentry_open(struct file *file)
    > > +{
    > > + struct file_security_struct *fsec;
    > > + struct inode *inode;
    > > + struct inode_security_struct *isec;
    > > + inode = file->f_path.dentry->d_inode;
    > > + fsec = file->f_security;
    > > + isec = inode->i_security;
    >
    > I'd add a comment here, e.g.
    > /*
    > * Save inode label and policy sequence number
    > * at open-time so that selinux_file_permission
    > * can determine whether revalidation is necessary.
    > * Task label is already saved in the file security
    > * struct as its SID.
    > */
    Fixed.

    >
    > > + fsec->isid = isec->sid;
    > > + fsec->pseqno = avc_policy_seqno();
    > > +
    > > + /*Permission has to be rechecked here.
    > > + Policy load of inode sid can happen between
    > > + may_open and selinux_dentry_open.*/
    >
    > Typo in the comment (s/of/or/), coding style isn't right for a
    > multi-line comment, and likely needs clarification, e.g.
    > /*
    > * Since the inode label or policy seqno may have changed
    > * between the selinux_inode_permission check and the saving
    > * of state above, recheck that access is still permitted.
    > * Otherwise, access might never be revalidated against the
    > * new inode label or new policy.
    > * This check is not redundant - do not remove.
    > */
    Fixed.


    >
    > > + return inode_has_perm(current, inode, file_to_av(file), NULL);
    > > +}
    > > +
    > > /* task security operations */
    > >
    > > static int selinux_task_create(unsigned long clone_flags)
    >
    > > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/fs/open.c linux-2.6.22/fs/open.c
    > > --- linux-2.6.22.orig/fs/open.c 2007-07-09 08:32:17.000000000 +0900
    > > +++ linux-2.6.22/fs/open.c 2007-09-12 08:31:24.000000000 +0900
    > > @@ -696,8 +696,13 @@ static struct file *__dentry_open(struct
    > > f->f_op = fops_get(inode->i_fop);
    > > file_move(f, &inode->i_sb->s_files);
    > >
    > > + error = security_dentry_open(f);
    > > + if (error)
    > > + goto cleanup_all;
    > > +
    > > if (!open && f->f_op)
    > > open = f->f_op->open;
    > > +
    >
    > Extraneous whitespace leftover from prior version of the patch.
    Fixed.

    >
    > > if (open) {
    > > error = open(inode, f);
    > > if (error)
    > > diff -purN -X linux-2.6.22/Documentation/dontdiff linux-2.6.22.orig/include/linux/security.h linux-2.6.22/include/linux/security.h
    > > --- linux-2.6.22.orig/include/linux/security.h 2007-07-09 08:32:17.000000000 +0900
    > > +++ linux-2.6.22/include/linux/security.h 2007-09-12 08:30:16.000000000 +0900
    > > @@ -503,6 +503,11 @@ struct request_sock;
    > > * @file contains the file structure being received.
    > > * Return 0 if permission is granted.
    > > *
    > > + * Security hook for dentry
    > > + *
    > > + * @dentry_open
    > > + * Check permission or get additional information before opening dentry.
    > > + *
    >
    > More precisely, "Save open-time permission checking state for later use
    > upon file_permission, and recheck access if anything has changed since
    > inode_permission."
    Fixed.

    > --
    > Stephen Smalley
    > National Security Agency

    I would like to send patch in next e-mail in new thread.

    Regards,
    --
    Yuichi Nakamura
    Hitachi Software Engineering Co., Ltd.
    Japan SELinux Users Group(JSELUG): http://www.selinux.gr.jp/
    SELinux Policy Editor: http://seedit.sourceforge.net/

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

    \
     
     \ /
      Last update: 2007-09-14 02:13    [W:0.030 / U:122.956 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site