| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Aug 2008 10:35:41 +0200
From: Carsten Otte <cotte@...ibm.com>
To: jaredeh@...il.com
CC: Linux-kernel@...r.kernel.org, linux-embedded@...r.kernel.org,
linux-mtd <linux-mtd@...ts.infradead.org>,
Jörn Engel <joern@...fs.org>,
tim.bird@...SONY.COM, nickpiggin@...oo.com.au
Subject: Re: [PATCH 04/10] AXFS: axfs_inode.c
Jared Hulbert wrote:
> +/***************** functions in other axfs files ******************************/
> +int axfs_get_sb(struct file_system_type *, int, const char *, void *,
> + struct vfsmount *);
This is neither implemented nor used in axfs_inode.c - why define it here?
> +void axfs_kill_super(struct super_block *);
Same for this one.
> +void axfs_profiling_add(struct axfs_super *, unsigned long, unsigned int);
> +int axfs_copy_mtd(struct super_block *, void *, u64, u64);
> +int axfs_copy_block(struct super_block *, void *, u64, u64);
These are used, but not implemented here. Please consider putting them
in a header file
> +static int axfs_copy_data(struct super_block *sb, void *dst,
> + struct axfs_region_desc *region, u64 offset, u64 len)
> +{
> + u64 mmapped = 0;
> + u64 end = region->fsoffset + offset + len;
> + u64 begin = region->fsoffset + offset;
> + u64 left;
> + void *addr;
> + void *newdst;
> + struct axfs_super *sbi = AXFS_SB(sb);
> +
> + if (len == 0)
> + return 0;
> +
> + if (region->virt_addr) {
> + if (sbi->mmap_size >= end) {
> + mmapped = len;
> + } else if (sbi->mmap_size > begin) {
> + mmapped = sbi->mmap_size - begin;
> + }
> + }
You can save braces and make the code more readable here:
=> if (sbi->mmap_size >= end)
mmapped = len;
else if (sbi->mmap_size > begin)
mmapped = si->mmap_size - begin;
> +struct inode *axfs_create_vfs_inode(struct super_block *sb, int ino)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + struct inode *inode;
> + u64 size;
[SNIP]
> + size = AXFS_GET_INODE_FILE_SIZE(sbi, ino);
> + inode->i_size = size;
The variable size is not needed. Do
inode->i_size = AXFS_GET_INODE_FILE_SIZE(sbi, ino);
> +static struct dentry *axfs_lookup(struct inode *dir, struct dentry *dentry,
> + struct nameidata *nd)
> +{
> + struct super_block *sb = dir->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = dir->i_ino;
> + u64 dir_index = 0;
> + u64 entry;
> + char *name;
> + int namelen, err;
> +
> + while (dir_index < AXFS_GET_INODE_NUM_ENTRIES(sbi, ino_number)) {
> + entry = AXFS_GET_INODE_ARRAY_INDEX(sbi, ino_number);
> + entry += dir_index;
> +
> + name = AXFS_GET_INODE_NAME(sbi, entry);
> + namelen = strlen(name);
> +
> + /* fast test, the entries are sorted alphabetically and the
> + * first letter is smaller than the first letter in the search
> + * name then it isn't in this directory. Keeps this loop from
> + * needing to scan through always.
> + */
> + if (dentry->d_name.name[0] < name[0])
> + break;
> +
> + dir_index++;
> +
> + /* Quick check that the name is roughly the right length */
> + if (dentry->d_name.len != namelen)
> + continue;
> +
> + err = memcmp(dentry->d_name.name, name, namelen);
> + if (err > 0)
> + continue;
> +
> + /* The file name isn't present in the directory. */
> + if (err < 0)
> + break;
Very ingenious way to compare strings. strncmp also stops after the
first character if it does'nt fit. I doubt this has a measurable
performance advantage over using strncmp, please consider to replace
this logic to make the code smaller and more readable. See lib/string.c.
> +static int axfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
> +{
> + struct inode *inode = filp->f_dentry->d_inode;
> + struct super_block *sb = inode->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = inode->i_ino;
> + u64 entry;
> + loff_t dir_index;
> + char *name;
> + int namelen, mode;
> + int err = 0;
> +
> + /* Get the current index into the directory and verify it is not beyond
> + the end of the list */
> + dir_index = filp->f_pos;
> + if (dir_index >= AXFS_GET_INODE_NUM_ENTRIES(sbi, ino_number))
> + goto out;
> +
> + /* Verify the inode is for a directory */
> + if (!(S_ISDIR(inode->i_mode))) {
> + err = -EINVAL;
> + goto out;
> + }
Well, -ENOTDIR would be the correct return code. You can remove that
sanity check alltogether, vfs_readdir makes sure this is the right
file type. If you really want to check, make it
BUG_ON(!S_ISDIR(inode->i_mode));
> +static int axfs_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> + struct file *file = vma->vm_file;
> + struct inode *inode = file->f_dentry->d_inode;
> + struct super_block *sb = inode->i_sb;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 ino_number = inode->i_ino;
> + u64 array_index;
> +
> + array_index = AXFS_GET_INODE_ARRAY_INDEX(sbi, ino_number) + vmf->pgoff;
> +
> + /* if that pages are marked for write they will probably end up in RAM
> + therefore we don't want their counts for being XIP'd */
> + if (!(vma->vm_flags & VM_WRITE))
> + axfs_profiling_add(sbi, array_index, ino_number);
Thats very inacurate profiling, it does never count MAP_PRIVATE
mappings which is the regular case for executables and libraries. When
booting an enterprise distro, my sniff test shows that only about 5%
of the MAP_PRIVATE mappings get COW'ed. To get correct statistics, it
might be a good idea to find a way to add here and substract during
cow. Or to scan these mappings when the profiling information is being
retrieved - the readonly bit in the pte gives the right indication for
MIXEDMAP mappings.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists