On Sun, 06 Jan 2008 15:20:00 +0900, Tetsuo Handa said:
> --- linux-2.6-mm.orig/fs/ramfs/inode.c
> +++ linux-2.6-mm/fs/ramfs/inode.c
> @@ -36,6 +36,20 @@
> #include <asm/uaccess.h>
> #include "internal.h"
>
> +static struct inode *__ramfs_get_inode(struct super_block *sb, int mode,
> + dev_t dev, bool tmpfs_with_mac);
> +
> +#define TMPFS_WITH_MAC 1
> +#define TMPFS_WITHOUT_MAC 0
> +#include <linux/quotaops.h>
> +
> +#ifdef CONFIG_SYAORAN
> +#include "syaoran.h"
> +#include "syaoran_init.c"
> +#include "syaoran_main.c"
> +#include "syaoran_debug.c"
> +#endif
Ouch. The .c files should generally be built into their own .o files and
then the Makefile should do something like
obj-$(CONFIG_SYAORAN) += syaoran.o
unless there's *really* good reasons for including .c files (such as an
otherwise-messy variable-namespace issue or similar).
Also, has this been double-checked to Do The Right Thing if you have
*two* instances of ramfs mounted, one with Syaoran and one without? I don't
know the code well enough to know if you found *all* the places you need
something like:
> - inode = ramfs_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);
> +#ifdef CONFIG_SYAORAN
> + /*** SYAORAN start. ***/
> + if (dir->i_sb->s_op == &syaoran_ops) {
> + if (syaoran_may_create_node(dentry, S_IFLNK, 0) < 0)
> + return -EPERM;
> + inode = syaoran_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);
> + /*** SYAORAN end. ***/
> + } else
> +#endif
> + inode = ramfs_get_inode(dir->i_sb, S_IFLNK|S_IRWXUGO, 0);
(incidentally, all of these should probably be abstracted into a helper
function that's 'static inline' so we have just one #ifdef in the definition
in a .h file, and none in open .c code).
Similarly for other places you have #ifdef CONFIG_ in ramfs .c code - see if
you can abstract it out.
> +/*
> + * Original tmpfs doesn't set ramfs_dir_inode_operations.setattr field.
> + * Now I'm setting the field to share tmpfs/rootfs/syaoran code.
Question for the audience: *should* ramfs set that field so setattr works
on ramfs (even if it's just a stub similar to the SELinux fscontext= mount
stuff)?
Question for Tetsuo: What happens to this code if somebody actually does the
above change?
> --- linux-2.6-mm.orig/fs/Kconfig
> +++ linux-2.6-mm/fs/Kconfig
> @@ -978,6 +978,24 @@ config TMPFS_POSIX_ACL
> + "Applications using well-known device locations under /dev
> + get the device they want" (e.g. an application that accesses
> + /dev/null can always get a character special device
> + with major=1 and minor=3).
This should say "will always get", not "can always", as this code will
mandate, rather than just make possible.
> + The list of possible combinations of filename and its attributes
> + that can exist on this filesystem is defined at mount time
> + using a configuration file.
The format of this file needs to be documented. I'm not terribly thrilled by
the idea of passing a file to be read by the kernel, but I also understand
that if it isn't done before mount, you have a race condition betweet the
mount and the load. Perhaps write some configfs code so that you can
'mount /configfs; cat config.file > /configfs/syaoran; mount -t syaoran"?
Similarly, it looks like you create your debug files inside the ramfs - that
is probably a bad idea and possibly can exhaust resources. Convert it to
use debugfs instead?
> + if (!filename) {
> + printk(KERN_INFO "SYAORAN: Missing config-file path.\n");
> + return -EINVAL;
Does this (and the code right after Do The Right Thing if somebody does this:
mount -t syaoran -o noatime,noexec /some/path
(I admit not knowing if mount options common to all mounts are stripped out
by the VFS code or passed down to this code).
Or even worse, "-o noatime,accept=/some/path/ramfs.cfg"?
> + f = open_pathname(AT_FDCWD, filename, O_RDONLY, 0600);
> + if (IS_ERR(f)) {
> + printk(KERN_INFO "SYAORAN: Can't open '%s'\n", filename);
> + return -EINVAL;
> + }
Does this do what you think it does if run in a chroot process or if
some creative person does "accept=../../path/to/bad_data.cfg"?
That printk should be KERN_ERR, I think.
That's all that's immediately obvious to me - somebody who actually understands
the filesystem code better will probably need to review it for all the stuff
I missed before it can be included.
Attachment:
pgp4FQTiHIydZ.pgp
Description: PGP signature
