Messages in this thread | | | Subject | Re: [RFC][PATCH 3/6] SLIM main patch | From | Kylene Jo Hall <> | Date | Fri, 14 Jul 2006 12:25:57 -0700 |
| |
Comments inline below.
On Fri, 2006-07-14 at 11:27 -0700, Dave Hansen wrote: > On Fri, 2006-07-14 at 10:24 -0700, Kylene Jo Hall wrote: > > +static int is_guard_integrity(struct slm_file_xattr *level) > > +{ > > + if ((level->guard.iac_r != SLM_IAC_NOTDEFINED) > > + && (level->guard.iac_wx != SLM_IAC_NOTDEFINED)) > > + return 1; > > + return 0; > > +} > > + > > +static int is_guard_secrecy(struct slm_file_xattr *level) > > +{ > > + if ((level->guard.sac_rx != SLM_SAC_NOTDEFINED) > > + && (level->guard.sac_w != SLM_SAC_NOTDEFINED)) > > + return 1; > > + return 0; > > +} > > This is a nice helper function. I think there are a couple of other > places where nice helpers like this could really clean things up. > I'll try to clean this up better in the next version.
> > + > > +#define do_demote_thread_list(head, member) { \ > > + struct task_struct *thread_tsk; \ > > + list_for_each_entry(thread_tsk, head, member) \ > > + do_demote_thread_entry(thread_tsk); \ > > +} > > Can this be an inline function instead? > I wanted to make it a static inline but how would I pass the member field name that list_for_each_entry needs. I presume this is why the list_for_each_* functions are #defines themselves.
> > +static void demote_threads(void) > > +{ > > + do_demote_thread_list(¤t->sibling, sibling); > > + do_demote_thread_list(¤t->children, children); > > +} > > + > > +/* > > + * Revoke write permissions and demote threads using shared memory > > + */ > > +static void revoke_permissions(struct slm_file_xattr *cur_level) > > +{ > > + if ((!is_kernel_thread(current)) && (current->pid != 1)) { > > + if (using_shmem()) > > + demote_threads(); > > + > > + revoke_mmap_wperm(cur_level); > > + revoke_file_wperm(cur_level); > > + } > > +} > > Is that using_shmem() check really necessary? IF you're not a threaded > process and you get asked to demote your threads, I would imagine that > the code would fall out of the loop immediately. What does this protect > against?
I'll test it out. > > > +static enum slm_iac_level set_iac(char *token) > > +{ > > + int iac; > > + > > + if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0) > > + return SLM_IAC_EXEMPT; > > + else { > > Might as well add brackets here. Or, just kill the else{} block and > pull the code back to the lower indenting level. The else is really > unnecessary because of the return;
I'll fix next revision. > > > + for (iac = 0; iac < sizeof(slm_iac_str) / sizeof(char *); iac++) { > > + if (memcmp(token, slm_iac_str[iac], > > + strlen(slm_iac_str[iac])) == 0) > > + return iac; > > Why not use strcmp? > > > +static enum slm_sac_level set_sac(char *token) > > +{ > > + int sac; > > + > > + if (memcmp(token, EXEMPT_STR, strlen(EXEMPT_STR)) == 0) > > + return SLM_SAC_EXEMPT; > > + else { > > + for (sac = 0; sac < sizeof(slm_sac_str) / sizeof(char *); sac++) { > > + if (memcmp(token, slm_sac_str[sac], > > + strlen(slm_sac_str[sac])) == 0) > > + return sac; > > + } > > + } > > + return SLM_SAC_ERROR; > > +} > > This function looks awfully similar :). Can you just pass that array in > as an argument, and get rid of one of the functions?
Sure that shouldn't be a problem.
> > > +static inline int set_bounds(char *token) > > +{ > > + if (memcmp(token, UNLIMITED_STR, strlen(UNLIMITED_STR)) == 0) > > + return 1; > > + return 0; > > +} > > strcmp? > > > +/* > > + * Get the 7 access class levels from the extended attribute > > + * Format: TIMESTAMP INTEGRITY SECRECY [INTEGRITY_GUARD INTEGRITY_GUARD] [SECRECY_GUARD SECRECY_GUARD] [GUARD_ TYPE] > > + */ > > +static int slm_parse_xattr(char *xattr_value, int xattr_len, > > + struct slm_file_xattr *level) > > +{ > > + char *token; > > + int token_len; > > + char *buf, *buf_end; > > + int fieldno = 0; > > + int rc = -1; > > + > > + buf = xattr_value + sizeof(time_t); > > + if (*buf == 0x20) > > + buf++; /* skip blank after timestamp */ > > + buf_end = xattr_value + xattr_len; > > + > > + while ((token = get_token(buf, buf_end, ' ', &token_len)) != NULL) { > > + buf = token + token_len; > > + switch (++fieldno) { > > + case 1: > > + if ((level->iac_level = > > + set_iac(token)) != SLM_IAC_ERROR) > > + rc = 0; > > + break; > > How about: > > level->iac_level = set_iac(token); > if (level->iac_level != SLM_IAC_ERROR) > rc = 0; > break;
ok
> > + isec->lock = SPIN_LOCK_UNLOCKED; > > + return isec; > > +} > > Is that safe, or is will the spin_lock_init() version make the lock > debugging code happier?
Ok.
> > +/* > > + * Exempt objects without extended attribute support > > + */ > > +static int is_exempt(struct inode *inode) > > +{ > > + if ((inode->i_sb->s_magic == PROC_SUPER_MAGIC) > > + || S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) > > + return 1; > > + return 0; > > +} > > This could probably be a much more generic function, no? > > inode_supports_xaddr()? Seems like something that should check a > superblock flag or something.
I don't know of any such flags.
Thanks, Kylie
- 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/
| |