On 2010-02-18, at 22:42, Aneesh Kumar K.V wrote:
+static struct dentry *handle_to_dentry(struct path *path,
+ struct file_handle *fh)
+{
+ inode = path->dentry->d_inode;
+ if (!S_ISREG(inode->i_mode) &&
+ !S_ISDIR(inode->i_mode) &&
+ !S_ISLNK(inode->i_mode)) {
Please follow normal indenting rules, like:
if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode) &&
!S_ISLNK(inode->i_mode)) {
+ /* should we do some check on handle size ?*/
+ handle = kmalloc(handle_size, GFP_KERNEL);
Good question. kmalloc() allows up to 32MB allocations, so a user
running 1024 threads calling this with bad handles could OOM most
systems today due to unswappable kernel allocations. Should there be
an upper limit on the handle size, like 4096 bytes or so? Should some
filesystem legitimately need a larger size the kernel can always be
changed without problems since the size isn't part of the ABI (it
should be an internal sanity check), but I can't imagine what needs to
be put into a file handle that would exceed this size.
+ dentry = exportfs_decode_fh(path->mnt, (struct fid *)handle,
+ handle_size, fh->handle_type,
+ vfs_dentry_acceptable, NULL);
One serious problem I can see with this is that the handle only
encodes the ino+generation of the inode, and nothing about the device
itself. NFS handles this by using the fsid to identify the
filesystem, but it would be highly confusing to applications if a
process with a different CWD gets a different file or an error trying
to open the file handle, or if a new filesystem gets mounted and the
file handle stops working. Having the file handle able to positively
identify a single inode in the system is the most important property
it has, I think.
Probably one of the important use cases of open_by_handle() is to have
one process doing pathname resolution and slave threads doing work on
those handles. Usually daemon threads change their working directory
to "/" to avoid pinning some directory. Even without that, it would
be impossible for a process to create handles in 2 different
filesystems, which is totally broken:
(CWD is /home/adilger)
name_to_handle("/usr/src/linux/COPYING", &fh);
fd = open_by_handle(&fh, O_RDONLY);
(fd = -1; errno = ENOENT or EBADF or whatever)
Without the handle identifying the filesystem in some way this is
mostly useless. Encoding the device number would be a simple (though
non-robust) way to do this, a better way would be with the filesystem
UUID and adding an in-kernel UUID->sb mapping function (which is
useful for other things as well).
+long do_sys_open_by_handle(int dfd, struct file_handle *fh, int
flags)
+{
+ if (!capable(CAP_SYS_ADMIN))
+ /* Allow open by handle only by sysadmin */
+ return -EPERM;
Hmm, I guess this avoids some of the security concerns, but makes it a
lot less useful. I was thinking this could be used for e.g. user NFS
serving or such, but if it is limited to root only then you might as
well just set up the in-kernel NFSd. By making the handle hard to
forge (e.g. generate random key per superblock, sha1(ino+gen+key) and
store that into fh; someone with more security experience can think of
a better scheme) then you can reasonably safely dispense with the
CAP_SYS_ADMIN check because you can be sure that the proper path
traversal has been done by a trusted process and there is no more
exposure than unix socket fd passing.
+ if ((!(flags & O_APPEND) || (flags & O_TRUNC)) &&
+ (flags & FMODE_WRITE) && IS_APPEND(inode)) {
Please use normal indenting style, aligning on the parenthesis.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
