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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sun, 12 Apr 2009 02:35:44 +0200
From:	Sam Ravnborg <sam@...nborg.org>
To:	Adrian McMenamin <adrian@...golddream.dyndns.info>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	viro <viro@...iv.linux.org.uk>,
	linux-sh <linux-sh@...r.kernel.org>
Subject: Re: [RFC][patch] VMUFAT filesystem - v2

Hi Adrian.

Some nitpicks. I do not know filesystems so nothing value on that front.

> 
> I will be writing appropriate userland tools and more documentation in
> due course.
> 
>  fs/Kconfig         |    1 +
>  fs/Makefile        |    1 +
>  fs/vmufat/Kconfig  |   14 +
>  fs/vmufat/Makefile |    7 +
>  fs/vmufat/inode.c  | 1400 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 1423 insertions(+), 0 deletions(-)

That was a lot of code in one file.

> 
> Signed-off by: Adrian McMenamin <adrian@...en.demon.co.uk>

It is:
Signed-off-by: ...


> +++ b/fs/vmufat/Makefile
> @@ -0,0 +1,7 @@
> +#
> +# Makefile for VMUFAT filesystem
> +#
> +
> +obj-$(CONFIG_VMUFAT_FS) += vmufat.o
> +
> +vmufat-objs := inode.o

Please use:
vmufat-y := inode.o

This is preferred syntax these days.

> diff --git a/fs/vmufat/inode.c b/fs/vmufat/inode.c
> new file mode 100644
> index 0000000..8ac0cba
> --- /dev/null
> +++ b/fs/vmufat/inode.c
> @@ -0,0 +1,1400 @@
> +/*
> + * inode operations for the VMU file system
> + *
> + * Copyright (C) 2002 - 2009	Adrian McMenamin
> + * Copyright (C) 2002		Paul Mundt
> + *
> + * Released under the terms of the GNU GPL.
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/blkdev.h>
> +#include <linux/buffer_head.h>
> +#include <linux/statfs.h>
> +#include <linux/mpage.h>
> +#include <linux/bcd.h>

The preference is to either sort includes by length (longest first)
or in alphabetic order.


> +
> +#define VMUFAT_NAMELEN 12
> +
> +/* GNU utils won't list files with inode num 0 */
> +#define VMUFAT_ZEROBLOCK 32768
> +#define VMU_BLK_SZ 512
> +#define VMUFAT_MAGIC 0x55555555
No so magic?
And it belongs in <linux/magic.h>

> +
> +static struct kmem_cache *vmufat_inode_cachep;
> +static struct kmem_cache *vmufat_blist_cachep;
> +static const struct inode_operations vmufat_inode_operations;
> +static const struct file_operations vmufat_file_operations;
> +static const struct address_space_operations vmufat_address_space_operations;
> +static const struct file_operations vmufat_file_dir_operations;
> +static const struct super_operations vmufat_super_operations;
> +
> +static struct inode *vmufat_get_inode(struct super_block *sb, long ino);
> +static int vmufat_list_blocks(struct inode *in);
Can you rearrange things so we do not need these forward decalarations?

> +struct memcard {
> +	long sb_bnum;
> +	long fat_bnum;
> +	long fat_len;
> +	long dir_bnum;
> +	long dir_len;
> +	long numblocks;
> +};
> +
> +struct vmufat_block_list {
> +	struct list_head b_list;
> +	int bno;
> +};
> +
> +struct vmufat_inode {
> +	struct vmufat_block_list blocks;
> +	int nblcks;
> +	struct inode vfs_inode;
> +};
> +
> +static struct vmufat_inode *VMUFAT_I(struct inode *in)
> +{
> +	return container_of(in, struct vmufat_inode, vfs_inode);
> +}
> +
> +struct vmufat_file_info {
> +	__u8 ftype;
> +	__u8 copy_pro;
> +	__u16 fblk;
> +	char fname[12];
> +};
VMUFAT_NAMELEN?

> +/* VMU hardware is flaky, so let's compensate for that
> + * without losing hardare independence -
> + * as it is likely to be where this filesystem is used
> + */
> +static inline struct buffer_head *vmufat_sb_bread(struct super_block *sb,
> +	sector_t block)
> +{
> +	struct buffer_head *bh;
> +	bh = sb_bread(sb, block);
> +	if (bh)
> +		return bh;
> +	return sb_bread(sb, block);
> +}
Looks wrong that we just try the same thing twice.

> +
> +/* Linear day numbers of the respective 1sts in non-leap years. */
> +static int day_n[] =
> +    {0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};
> +
> +static struct dentry *vmufat_inode_lookup(struct inode *in, struct dentry *dent,
> +	struct nameidata *nd)
> +{
> +	struct super_block *sb;
> +	struct memcard *vmudetails;
> +	struct buffer_head *bh;
> +	struct inode *ino;
> +	char name[VMUFAT_NAMELEN];
> +	long blck_read;
> +	int error = 0, fno = 0;
> +
> +	if (dent->d_name.len > VMUFAT_NAMELEN) {
> +		error = -ENAMETOOLONG;
> +		goto out;
> +	}
> +
> +	sb = in->i_sb;
> +	vmudetails = sb->s_fs_info;
> +	blck_read = vmudetails->dir_bnum;
> +
> +	bh = vmufat_sb_bread(sb, blck_read);
> +	if (!bh) {
> +		error = -EIO;
> +		goto out;
> +	}
> +
> +	do {
> +		/* Have we got a file? */
> +		if (bh->b_data[vmufat_index(fno)] == 0)
> +			goto next;
> +
> +		/* get file name */
> +		memcpy(name,
> +		       bh->b_data + 4 + vmufat_index(fno), VMUFAT_NAMELEN);
> +		/* do names match ?*/
> +		if (memcmp(dent->d_name.name, name, dent->d_name.len) == 0) {
> +			/* read the inode number from the directory */
> +			ino = vmufat_get_inode(sb,
> +				le16_to_cpu(((u16 *) bh->b_data)
> +					[1 + vmufat_index_16(fno)]));
> +			if (!ino) {
> +				error = -EACCES;
> +				goto release_bh;
> +			}
> +			if (IS_ERR(ino)) {
> +				error = PTR_ERR(ino);
> +				goto release_bh;
> +			}
> +			/* return the entry */
> +			d_add(dent, ino);
> +			goto release_bh;
> +		}
> +next:
> +		/* did not match, so try the next file */
> +		fno++;
> +		/* do we need to move to the next block in the directory? */
> +		if (fno >= 0x10) {

Can we have a descriptive constant here..


> +			fno = 0;
> +			blck_read--;
> +			if (blck_read <=
> +				vmudetails->dir_bnum - vmudetails->dir_len) {
> +				d_add(dent, NULL);
> +				break;
> +			}
> +			brelse(bh);
> +			bh = vmufat_sb_bread(sb, blck_read);
> +			if (!bh) {
> +				error = -EIO;
> +				goto out;
> +			}
> +		}
> +	} while (1);
> +
> +release_bh:
> +	brelse(bh);
> +out:
> +	return ERR_PTR(error);
> +}
> +

> +/*
> + * Find a free block in the FAT and mark it
> + * as the end of a file
> + */
> +static int vmufat_find_free(struct super_block *sb)
> +{
> +	struct memcard *vmudetails = sb->s_fs_info;
> +	int nextblock, x;
> +	u16 fatdata;
> +	struct buffer_head *bh_fat;
> +	int error = 0;
> +
> +	nextblock = vmudetails->fat_bnum + vmudetails->fat_len - 1;
> +	x = VMU_BLK_SZ;
> +	bh_fat =
> +	    vmufat_sb_bread(sb, nextblock);
> +	if (!bh_fat) {
> +		error = -EIO;
> +		goto fail;
> +	}
> +
> +	do {
> +		fatdata = ((u16 *) bh_fat->b_data)[x];
> +		if (fatdata == 0xfffc)
> +			break;	/*empty block */
Is this endian safe - you use le16_to_cpu before.
Hmmm, you are reading a char * and casting it to a u16 *.
So -4 equals 0xfffc. That should be ok.
But it would be nicer with an accessor function to read this.

Use a constant for 0xfffc (you use it multiple times)

> +		if (--x < 0) {
> +			put_bh(bh_fat);
> +			if (--nextblock >= vmudetails->fat_bnum) {
> +				x = VMU_BLK_SZ;
> +				bh_fat = vmufat_sb_bread(sb, nextblock);
> +				if (!bh_fat) {
> +					error = -EIO;
> +					goto fail;
> +				}
> +			} else
> +				break;
> +		}
> +	} while (1);
> +
> +	if (nextblock < vmudetails->fat_bnum) {
> +		printk(KERN_ERR "VMUFAT: device is full\n");
> +		error = -ENOSPC;
> +		put_bh(bh_fat);
> +		goto fail;
> +	}
> +	put_bh(bh_fat);
> +	return  x + (nextblock - vmudetails->fat_bnum) * VMU_BLK_SZ;
> +
> +fail:
> +	return error;
> +}

> +
> +/* read the FAT for a given block */
> +static u16 vmufat_get_fat(struct super_block *sb, long block)
> +{
> +	struct memcard *vmudetails = sb->s_fs_info;
> +	struct buffer_head *bh;
> +	int offset;
> +	u16 block_content;
> +
> +	offset = block/(VMU_BLK_SZ/2);
Spaces around operators..

> +	if (offset >= vmudetails->fat_len)
> +		return 0xFFFE;

Constant for 0xFFFE

> +
> +	bh = vmufat_sb_bread(sb, offset + 1 +
> +		vmudetails->fat_bnum - vmudetails->fat_len);
> +	if (!bh)
> +		return 0xFFFF;

Constant for 0xFFFF

> +
> +
> +	block_content = ((u16 *)bh->b_data)[block % (VMU_BLK_SZ / 2)];

What is the difference between block % (VMU_BLK_SZ / 2) and block / (VMU_BLK_SZ / 2)?


> +	put_bh(bh);
> +	return block_content;
> +}

> +
> +/* set the FAT for a given block */
> +static int vmufat_set_fat(struct super_block *sb, long block, u16 set)
> +{
> +	struct memcard *vmudetails = sb->s_fs_info;
> +	struct buffer_head *bh;
> +	int offset;
> +
> +	offset = block/(VMU_BLK_SZ/2);
Spaces. And same calculation as before.

> +	if (offset >= vmudetails->fat_len)
> +		return -EINVAL;
> +
> +	bh = vmufat_sb_bread(sb, offset + 1 +
> +		vmudetails->fat_bnum - vmudetails->fat_len);
> +	if (!bh)
> +		return -EIO;
> +
> +	((u16 *) bh->b_data)[block % (VMU_BLK_SZ / 2)] = set;
> +	mark_buffer_dirty(bh);
> +	put_bh(bh);
> +	return 0;
> +}
> +
> +static int vmufat_inode_create(struct inode *dir, struct dentry *de,
> +		int imode, struct nameidata *nd)
> +{
> +	/* Create an inode */
> +	int x = 0, y, z, error = 0, q;
> +	long blck_read;
> +	struct inode *inode;
> +	struct super_block *sb;
> +	struct memcard *vmudetails;
> +	struct buffer_head *bh_fat = NULL, *bh;
> +	unsigned long unix_date;
> +	int year, day, nl_day, month;	/*inspired by FAT driver */
> +	u8 century, u8year;
> +
> +	if (de->d_name.len > VMUFAT_NAMELEN)
> +		return -ENAMETOOLONG;
> +
> +	sb = dir->i_sb;
> +	vmudetails = sb->s_fs_info;
> +
> +	inode = new_inode(sb);
> +	if (!inode) {
> +		error = -ENOSPC;
> +		goto out;
> +	}
> +
> +	/* Walk through blocks looking for place to write
> +	* Is this an executible file? */
> +	if (imode & 73) {	/*Octal 111 */

Then write an octal value?

> +		inode->i_ino = VMUFAT_ZEROBLOCK;
> +		/* But this already allocated? */
> +		if (vmufat_get_fat(sb, 0) != 0xFFFC) {
> +			printk(KERN_ERR
> +				"VMUFAT: cannot write executible file to"
> +				" filesystem - block 0 already allocated.\n");
> +			error = -ENOSPC;
> +			goto clean_inode;
> +		}
> +		q = 0;
> +	} else {
> +		q = vmufat_find_free(sb);
> +		if (q < 0) {
> +			error = q;
> +			goto clean_inode;
> +		}
> +		inode->i_ino = q;
> +	}
> +
> +	error = vmufat_set_fat(sb, q, 0xFFFA);
> +	if (error)
> +		goto clean_inode;
> +
> +	inode->i_uid = 0;
> +	inode->i_gid = 0;
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> +	inode->i_mode = imode;
> +	inode->i_blocks = 1;
> +	inode->i_sb = sb;
> +	insert_inode_hash(inode);
> +	inode->i_op = &vmufat_inode_operations;
> +	inode->i_fop = &vmufat_file_operations;
> +	inode->i_mapping->a_ops = &vmufat_address_space_operations;
> +
> +	/* Write to the directory
> +	* Now search for space for the directory entry */
> +	blck_read = vmudetails->dir_bnum;
> +	bh = vmufat_sb_bread(sb, blck_read);
> +	if (!bh) {
> +		error = -EIO;
> +		goto clean_inode;
> +	}
> +
> +	for (y = 0; y < (vmudetails->dir_len * 0x10); y++) {
> +		if ((y / 0x10) > (vmudetails->dir_bnum - blck_read)) {
> +			brelse(bh);
> +			blck_read--;
> +			bh = vmufat_sb_bread(sb, blck_read);
> +			if (!bh) {
> +				error = -EIO;
> +				goto clean_fat;
> +			}
> +		}
> +		if (((bh->b_data)[vmufat_index(y)]) == 0)
> +			break;
> +	}
> +	/* Have the directory entry
> +	 * so now update it */
> +	z = vmufat_index(y);
> +	if (inode->i_ino != VMUFAT_ZEROBLOCK)
> +		bh->b_data[z] = 0x33;	/* data file */
> +	else
> +		bh->b_data[z] = 0xcc;
> +
> +	if ((bh->b_data[z + 1] != (char) 0x00) &&
> +		(bh->b_data[z + 1] != (char) 0xff))
> +		bh->b_data[z + 1] = (char) 0x00;
> +
> +	if (inode->i_ino != VMUFAT_ZEROBLOCK) {
> +		((u16 *) bh->b_data)[z / 2 + 1] =
> +		    cpu_to_le16(inode->i_ino);
> +		((u16 *) bh->b_data)[z / 2 + 0x0D] = 0;
> +	} else {
> +		((u16 *) bh->b_data)[z / 2 + 1] = 0;
> +		((u16 *) bh->b_data)[z / 2 + 0x0D] = 1;
> +	}
> +
> +	/* Name */
> +	memset((char *) (bh->b_data + z + 0x04), '\0', 0x0C);
> +	memcpy((char *) (bh->b_data + z + 0x04), ((de->d_name).name),
> +		de->d_name.len);
> +
Lots of hardcoded constants above that I think describe the filesystem layout..

> +	/* BCD timestamp it */
> +	unix_date = CURRENT_TIME.tv_sec;
> +	day = unix_date / 86400 - 3652;
> +	year = day / 365;
> +
> +	if ((year + 3) / 4 + 365 * year > day)
> +		year--;
> +
> +	day -= (year + 3) / 4 + 365 * year;
> +	if (day == 59 && !(year & 3)) {
> +		nl_day = day;
> +		month = 2;
> +	} else {
> +		nl_day = (year & 3) || day <= 59 ? day : day - 1;
> +		for (month = 0; month < 12; month++)
> +			if (day_n[month] > nl_day)
> +				break;
> +	}
> +
> +	century = 19;
> +	if (year > 19)
> +		century = 20;
> +
> +	bh->b_data[z + 0x10] = bin2bcd(century);
> +	u8year = year + 80;
> +	if (u8year > 99)
> +		u8year = u8year - 100;
> +
> +	bh->b_data[z + 0x11] = bin2bcd(u8year);
> +	bh->b_data[z + 0x12] = bin2bcd(month);
> +	bh->b_data[z + 0x13] =
> +	    bin2bcd(day - day_n[month - 1] + 1);
> +	bh->b_data[z + 0x14] =
> +	    bin2bcd((unix_date / 3600) % 24);
> +	bh->b_data[z + 0x15] = bin2bcd((unix_date / 60) % 60);
> +	bh->b_data[z + 0x16] = bin2bcd(unix_date % 60);
The above should be in a separate function.

> +
> +	((u16 *) bh->b_data)[z / 2 + 0x0C] =
> +	    cpu_to_le16(inode->i_blocks);
> +
> +	inode->i_mtime.tv_sec = unix_date;
> +	mark_buffer_dirty(bh);
> +	brelse(bh);
> +
> +	error = vmufat_list_blocks(inode);
> +	if (error)
> +		goto clean_fat;
> +
> +	d_instantiate(de, inode); printk("created inode 0x%lX\n", inode->i_ino);
> +	brelse(bh_fat);
> +	return error;
> +
> +clean_fat:
> +	((u16 *)bh_fat->b_data)[x] = 0xfffc;
> +	mark_buffer_dirty(bh_fat);
> +	brelse(bh_fat);
> +clean_inode:
> +	iput(inode);
> +out:
> +	return error;
> +}
> +
> +static int vmufat_inode_rename(struct inode *in_source,
> +			      struct dentry *de_source,
> +			      struct inode *in_target,
> +			      struct dentry *de_target)
> +{
> +	return -EPERM;
> +}
> +
> +static int vmufat_readdir(struct file *filp, void *dirent, filldir_t filldir)
> +{
> +	int filenamelen, i, error = 0;
> +	struct vmufat_file_info *saved_file = NULL;
> +	struct dentry *dentry = filp->f_dentry;
> +	struct inode *inode = dentry->d_inode;
> +	struct super_block *sb = inode->i_sb;
> +	struct memcard *vmudetails = sb->s_fs_info;
> +	struct buffer_head *bh;
> +
> +	int blck_read = vmudetails->dir_bnum;
> +	bh = vmufat_sb_bread(sb, blck_read);
> +	if (!bh) {
> +		error = -EIO;
> +		goto out;
> +	}
> +
> +	i = filp->f_pos;
> +
> +	/* handle . for this directory and .. for parent */
> +	switch ((unsigned int) filp->f_pos) {
> +	case 0:
> +		if (filldir(dirent, ".", 1, i++, inode->i_ino, DT_DIR) < 0)
> +			goto finish;
> +
> +		filp->f_pos++;
> +	case 1:
> +		if (filldir(dirent, "..", 2, i++,
> +			    dentry->d_parent->d_inode->i_ino, DT_DIR) < 0)
> +			goto finish;
> +
> +		filp->f_pos++;
> +	default:
> +		break;
> +	}
> +
> +	/* trap reading beyond the end of the directory */
> +	if ((i - 2) > (vmudetails->dir_len * 0x10)) {
> +		error = -EINVAL;
> +		goto release_bh;
> +	}
> +
> +	saved_file =
> +	    kmalloc(sizeof(struct vmufat_file_info), GFP_KERNEL);
> +	if (!saved_file) {
> +		error = -ENOMEM;
> +		goto release_bh;
> +	}
> +
> +	do {
> +		if ((i - 2) / 0x10 > (vmudetails->dir_bnum - blck_read)) {
> +			/* move to next block in directory */
> +			blck_read--;
> +			if (vmudetails->dir_bnum - vmudetails->dir_len <=
> +				blck_read)
> +				break;
> +			brelse(bh);
> +			bh = vmufat_sb_bread(sb, blck_read);
> +			if (!bh) {
> +				kfree(saved_file);
> +				error = -EIO;
> +				goto out;
> +			}
It is simpler to just do:
			if (!bh) {
				error = -EIO;
				goto finish;
			}
kfree() is a noop for NULL pointers, and likewise for brelse().
So simplify your error paths with this in mind.
This is true at several places in the file.


> +		}
> +
> +		saved_file->ftype = bh->b_data[vmufat_index(i - 2)];
> +
> +		if (saved_file->ftype == 0)
> +			break;
> +
> +		saved_file->fblk =
> +		    le16_to_cpu(((u16 *) bh->b_data)[1 +
> +			vmufat_index_16(i - 2)]);
This seems to be a common operation - use a helper function.

> +		if (saved_file->fblk == 0)
> +			saved_file->fblk = VMUFAT_ZEROBLOCK;
> +
> +		memcpy(saved_file->fname,
> +		       bh->b_data + 4 + vmufat_index(i - 2), VMUFAT_NAMELEN);
> +		filenamelen = strlen(saved_file->fname);
> +		if (filenamelen > VMUFAT_NAMELEN)
> +			filenamelen = VMUFAT_NAMELEN;
> +		if (filldir
> +		    (dirent, saved_file->fname, filenamelen, i++,
> +		     saved_file->fblk, DT_REG) < 0) {
This is an ugly way to avoid the 80 char limit - please redo.

> +			goto finish;
> +		}
> +
> +		filp->f_pos++;
> +	} while (1);
> +
> +finish:
> +	kfree(saved_file);
> +release_bh:
> +	brelse(bh);
> +out:
> +	return error;
> +}

I did not read furter from here.
But please try to see which comments apply to the rest of the file.

	Sam
--
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