| 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
| ||
|
Message-ID: <48AE19AD.1020209@lougher.demon.co.uk>
Date: Fri, 22 Aug 2008 02:43:09 +0100
From: Phillip Lougher <phillip@...gher.demon.co.uk>
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, cotte@...ibm.com, nickpiggin@...oo.com.au
Subject: Re: [PATCH 06/10] AXFS: axfs_super.c
Jared Hulbert wrote:
> +static void axfs_free_region(struct axfs_super *sbi,
> + struct axfs_region_desc *region)
> +{
> + if (!region)
> + return;
> +
> + if (AXFS_IS_REGION_XIP(sbi, region))
> + return;
> +
> + if (region->virt_addr)
> + vfree(region->virt_addr);
> +}
> +
No need to do
if(xxx)
vfree(xxx)
vfree/kfree can cope with NULL pointers.
> +static void axfs_put_sbi(struct axfs_super *sbi)
> +{
> + axfs_free_region(sbi, &sbi->uids);
> + axfs_free_region(sbi, &sbi->gids);
> +
> + if (sbi->second_dev)
> + kfree(sbi->second_dev);
> +
> + if (sbi->cblock_buffer[0])
> + vfree(sbi->cblock_buffer[0]);
> + if (sbi->cblock_buffer[1])
> + vfree(sbi->cblock_buffer[1]);
> +
Again, just kfree(xxx)/vfree(xxx)
> +
> +static int axfs_copy_mem(struct super_block *sb, void *buf, u64 fsoffset,
> + u64 len)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + unsigned long addr;
> +
> + addr = sbi->virt_start_addr + (unsigned long)fsoffset;
> + memcpy(buf, (void *)addr, (size_t) len);
> + return 0;
Always returns 0, consider changing to static void
> +}
> +
> +static int axfs_copy_metadata(struct super_block *sb, void *buf, u64 fsoffset,
> + u64 len)
> +{
> + struct axfs_super *sbi = AXFS_SB(sb);
> + u64 end = fsoffset + len;
> + u64 a = sbi->mmap_size - fsoffset;
> + u64 b = end - sbi->mmap_size;
> + void *bb = (void *)((unsigned long)buf + (unsigned long)a);
> + int err;
> +
> + /* Catches case where sbi is not yet fully initialized. */
> + if ((sbi->magic == 0) && (sbi->virt_start_addr != 0))
> + return axfs_copy_mem(sb, buf, fsoffset, len);
> +
> + if (fsoffset < sbi->mmap_size) {
> + if (end > sbi->mmap_size) {
> + err = axfs_copy_metadata(sb, buf, fsoffset, a);
> + if (err)
> + return err;
> + err = axfs_copy_metadata(sb, bb, sbi->mmap_size, b);
> + } else {
> + if (AXFS_IS_OFFSET_MMAPABLE(sbi, fsoffset)) {
> + err = axfs_copy_mem(sb, buf, fsoffset, len);
> + } else if (AXFS_HAS_MTD(sb)) {
> + err = axfs_copy_mtd(sb, buf, fsoffset, len);
> + } else if (AXFS_HAS_BDEV(sb)) {
> + err = axfs_copy_block(sb, buf, fsoffset, len);
> + } else {
> + err = -EINVAL;
Consider initialising err to -EINVAL at declaration time, and get rid of
this else,
> + }
> + }
> + } else {
> + if (AXFS_NODEV(sb)) {
> + err = axfs_copy_mem(sb, buf, fsoffset, len);
> + } else if (AXFS_HAS_BDEV(sb)) {
> + err = axfs_copy_block(sb, buf, fsoffset, len);
> + } else if (AXFS_HAS_MTD(sb)) {
> + err = axfs_copy_mtd(sb, buf, fsoffset, len);
> + } else {
> + err = -EINVAL;
and this one.
> + }
> + }
> + return err;
> +}
> +
> +static int axfs_fill_region_data(struct super_block *sb,
> + struct axfs_region_desc *region, int force)
> +{
> +
> + if (AXFS_IS_REGION_INCORE(region))
> + goto incore;
> +
> + if (AXFS_IS_REGION_COMPRESSED(region))
> + goto incore;
> +
> + if (AXFS_IS_REGION_XIP(sbi, region)) {
> + if ((end > sbi->mmap_size) && (force))
> + goto incore;
> + addr = sbi->virt_start_addr;
> + addr += (unsigned long)fsoffset;
> + region->virt_addr = (void *)addr;
> + return 0;
> + }
> +
> +incore:
> + region->virt_addr = vmalloc(size);
> + if (!region->virt_addr)
> + goto out;
> + vaddr = region->virt_addr;
> +
> + if (AXFS_IS_REGION_COMPRESSED(region)) {
> + buff = vmalloc(c_size);
> + if (!buff)
> + goto out;
> + axfs_copy_metadata(sb, buff, fsoffset, c_size);
> + err = axfs_uncompress_block(vaddr, size, buff, c_size);
> + if (!err)
> + goto out;
> + vfree(buff);
> + } else {
> + axfs_copy_metadata(sb, vaddr, fsoffset, size);
> + }
> +
> + return 0;
From this it would appear that if the region data can't be mapped XIP
(i.e. it is compressed or on a block device), it is cached in its
entirety in RAM?
This implies for block devices that the entire filesystem metadata has
to be cached in RAM. This severely limits the size of AXFS filesystems
when using block devices, or the else memory usage will be excessive.
> +
> +out:
> + if (buff)
> + vfree(buff);
> + if (region->virt_addr)
> + vfree(region->virt_addr);
> + return err;
> +}
> +
Just do kfree(xxx)
> +
> +static int axfs_get_onmedia_super(struct super_block *sb)
> +{
> + int err;
> + struct axfs_super *sbi = AXFS_SB(sb);
> + struct axfs_super_onmedia *sbo;
> +
> + sbo = kmalloc(sizeof(*sbo), GFP_KERNEL);
> + if (!sbo)
> + return -ENOMEM;
> +
> + axfs_copy_metadata(sb, (void *)sbo, 0, sizeof(*sbo));
> +
> + /* Do sanity checks on the superblock */
> + if (be32_to_cpu(sbo->magic) != AXFS_MAGIC) {
> + printk(KERN_ERR "axfs: wrong magic\n");
> + err = -EINVAL;
> + goto out;
> + }
> +
> + /* verify the signiture is correct */
> + if (strncmp(sbo->signature, AXFS_SIGNATURE, sizeof(AXFS_SIGNATURE))) {
> + printk(KERN_ERR "axfs: wrong axfs signature,"
> + " read %s, expected %s\n",
> + sbo->signature, AXFS_SIGNATURE);
> + err = -EINVAL;
> + goto out;
> + }
> +
> + sbi->magic = be32_to_cpu(sbo->magic);
> + sbi->version_major = sbo->version_major;
> + sbi->version_minor = sbo->version_minor;
> + sbi->version_sub = sbo->version_sub;
> + sbi->files = be64_to_cpu(sbo->files);
> + sbi->size = be64_to_cpu(sbo->size);
> + sbi->blocks = be64_to_cpu(sbo->blocks);
> + sbi->mmap_size = be64_to_cpu(sbo->mmap_size);
> + sbi->cblock_size = be32_to_cpu(sbo->cblock_size);
> + sbi->timestamp.tv_sec = be64_to_cpu(sbo->timestamp);
> + sbi->timestamp.tv_nsec = 0;
> + sbi->compression_type = sbo->compression_type;
> +
> + err = axfs_set_compression_type(sbi);
> + if (err)
> + goto out;
> +
> + /* If no block or MTD device, adjust mmapable to cover all image */
> + if (AXFS_NODEV(sb))
> + sbi->mmap_size = sbi->size;
> +
> + err = axfs_fill_region_descriptors(sb, sbo);
> + if (err)
> + goto out;
> +
> + err = 0;
Redundant code
> +out:
> + kfree(sbo);
> + return err;
> +}
> +
Phillip
--
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