Messages in this thread | | | Subject | Re: [PATCH 3/4] integrity: IMA as an integrity service provider | From | Mimi Zohar <> | Date | Thu, 20 Nov 2008 20:42:42 -0500 |
| |
On Thu, 2008-11-20 at 13:15 -0500, Christoph Hellwig wrote: > Ok, after the API is sorted out I had a quick looks at this patch. > > The first very odd thing is the data strucutures: > > > +struct ima_args_data { > > + const char *filename; > > + struct file *file; > > + struct path *path; > > + struct dentry *dentry; > > + struct inode *inode; > > + enum lim_hooks function; > > + u32 osid; > > + int mask; > > +}; > > You can always get from a file to a path, from a path to a dentry, > from a dentry to and inode and from a path to some defintion of a > filename. So a lot of things here seems very redundant. > > When looking at how it's used it's acually even worse. AFAICS the > code would be a lot cleaner if you'd just kill struct ima_args_data > and the odd pass arguments as void pointers obsfucations and just > pass the file/path + mask directly to the lower level functions. > > That's also help killing things like ima_store_measurement which > do entirely different things depending on idata->type.
hm, looking into it.
> > +static int skip_measurement(struct inode *inode, int mask) > > +{ > > + if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) > > + return 1; /* can't measure */ > > + > > + if (special_file(inode->i_mode) || S_ISLNK(inode->i_mode)) > > + return 1; /* don't measure */ > > + > > + if (S_ISREG(inode->i_mode)) > > + return 0; /* measure */ > > + return 1; /* don't measure */ > > +} > > This could just be an > > if (!S_ISREG(inode->i_mode)) > > in the caller..
Done.
> > +static int update_file_hash(struct file *f, struct path *path, > > + struct hash_desc *desc) > > Please split this into a update_file_hash that always operates on a > struct file, and a wrapper around it that creates the struct file > for the cases that needs it.
Ok.
> > +void ima_fixup_argsdata(struct ima_args_data *data, struct file *file, > > + struct path *path, int mask, int function) > > +{ > > + struct dentry *dentry = NULL; > > + > > + data->file = file; > > + data->path = path; > > + data->mask = mask; > > + data->function = function; > > + > > + if (file) > > + data->dentry = dentry = file->f_dentry; > > + > > + if (path) { > > + if (!dentry) > > + data->dentry = dentry = path->dentry; > > + } > > + if (dentry) > > + data->inode = dentry->d_inode; > > + > > + return; > > +} > > You have two different callers for this, either file NULL or path NULL > but never neither or both. So just do the setup in the callers and do > the right thing there. (and please kill the inode member, it's entirely > superflous) > > > +static void ima_file_free(struct file *file) > > +{ > > + struct inode *inode = NULL; > > + struct ima_iint_cache *iint; > > + > > + if (!file->f_dentry) /* can be NULL */ > > + return; > > No, it can't. > > > + > > + inode = file->f_dentry->d_inode; > > + if (S_ISDIR(inode->i_mode)) > > + return; > > + if ((file->f_mode & FMODE_WRITE) && > > + (atomic_read(&inode->i_writecount) == 1)) { > > > + * Returns 0 on success, -ENOMEM on failure > > + */ > > +static int ima_inode_alloc_integrity(struct inode *inode) > > +{ > > + return ima_iint_insert(inode); > > +} > > + > > +/** > > + * ima_inode_free_integrity - free the integrity structure > > + * @inode: the inode structure > > + */ > > +static void ima_inode_free_integrity(struct inode *inode) > > +{ > > + ima_iint_delete(inode); > > +} > > Why these wrappers?
You're right. I'll remove these routines and assign ima_iint_insert()/delete() directly in ima_integrity_ops.
> > + /* The file name is only a hint. */ > > + dentry = path->dentry; > > + data->filename = (!dentry->d_name.name) ? (char *)dentry->d_iname : > > + (char *)dentry->d_name.name; > > d_iname is the internal storage for d_name, always use d_name only.
ok
> > > + if (!file || !file->f_dentry) > > + return rc; > > Can't happen.
Will remove test.
> > +#define audit_type(type) AUDIT_ ##type > > +#define lsm_type(type) LSM_ ##type > > Just spelling out the constants would be a lot more readable..
ok
Mimi
| |