| 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: Sat, 5 Jan 2013 12:07:16 -0800
From: "Darrick J. Wong" <darrick.wong@...cle.com>
To: Darren Hart <dvhart@...radead.org>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger@...ger.ca,
sgw@...ux.intel.com
Subject: Re: [PATCH 1/3] lib/ext2fs: Add ext2fs_symlink
On Fri, Jan 04, 2013 at 12:00:58PM -0800, Darren Hart wrote:
> Creating symlinks is a complex affair when accounting for slowlinks.
>
> Create a new function, ext2fs_symlink(), modeled after ext2fs_mkdir().
> Like ext2fs_mkdir(), ext2fs_symlink() takes on the task of allocating a
> new inode and block (for slowlinks), setting up sane default values in
> the inode, copying the target path to either the inode (for fastlinks)
> or to the first block (for slowlinks), and accounting for the inode and
> block stats. Disallow link targets longer than blocksize as the Linux
> kernel prevents this.
>
> It does not attempt to expand the parent directory, instead returning
> EXT2_ET_DIR_NO_SPACE and leaving it to the caller to expand just as
> ext2fs_mkdir() does. Ideally, I think both of these functions should
> make a single attempt to expand the directory.
>
> Note the one remaining FIXME. Do I need to use ext2fs_bmap2() for the
> slowlink without extents? If not, how is the 0 block associated with the
> inode?
>
> libext2fs.a impact (stripped):
> Orig | Patched | Delta
> 274752 | 276580 | 1828
>
> V2 incorporates feedback from Ted and Darrick:
> o Add error codes to the end of the table
> o Remove redundant EXT2_FT_SYMLINK definition
> o Time fields set by write_new_inode
> o Use size=target_len for slow links as well
> o Test for len < blocksize
> o Replace file ops with manual block handling
> o Keep line width under 80 characters
>
> Signed-off-by: Darren Hart <dvhart@...radead.org>
> Cc: "Theodore Ts'o" <tytso@....edu>
> Cc: "Darrick J. Wong" <darrick.wong@...cle.com>
> Cc: "Andreas Dilger" <adilger@...ger.ca>
> ---
> lib/ext2fs/Makefile.in | 8 +++
> lib/ext2fs/ext2_err.et.in | 3 +
> lib/ext2fs/symlink.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 172 insertions(+)
> create mode 100644 lib/ext2fs/symlink.c
>
> diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
> index 1a51c51..dc7b0d1 100644
> --- a/lib/ext2fs/Makefile.in
> +++ b/lib/ext2fs/Makefile.in
> @@ -80,6 +80,7 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \
> res_gdt.o \
> rw_bitmaps.o \
> swapfs.o \
> + symlink.o \
> tdb.o \
> undo_io.o \
> unix_io.o \
> @@ -155,6 +156,7 @@ SRCS= ext2_err.c \
> $(srcdir)/res_gdt.c \
> $(srcdir)/rw_bitmaps.c \
> $(srcdir)/swapfs.c \
> + $(srcdir)/symlink.c \
> $(srcdir)/tdb.c \
> $(srcdir)/test_io.c \
> $(srcdir)/tst_badblocks.c \
> @@ -893,6 +895,12 @@ swapfs.o: $(srcdir)/swapfs.c $(top_builddir)/lib/config.h \
> $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
> $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
> $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
> +symlink.o: $(srcdir)/symlink.c $(top_builddir)/lib/config.h \
> + $(top_builddir)/lib/dirpaths.h $(srcdir)/ext2_fs.h \
> + $(top_builddir)/lib/ext2fs/ext2_types.h $(srcdir)/ext2fs.h \
> + $(srcdir)/ext2_fs.h $(srcdir)/ext3_extents.h $(top_srcdir)/lib/et/com_err.h \
> + $(srcdir)/ext2_io.h $(top_builddir)/lib/ext2fs/ext2_err.h \
> + $(srcdir)/ext2_ext_attr.h $(srcdir)/bitops.h
> tdb.o: $(srcdir)/tdb.c $(top_builddir)/lib/config.h \
> $(top_builddir)/lib/dirpaths.h $(srcdir)/tdb.h
> test_io.o: $(srcdir)/test_io.c $(top_builddir)/lib/config.h \
> diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
> index c99a167..d20c6b7 100644
> --- a/lib/ext2fs/ext2_err.et.in
> +++ b/lib/ext2fs/ext2_err.et.in
> @@ -473,4 +473,7 @@ ec EXT2_ET_UNKNOWN_CSUM,
> ec EXT2_ET_MMP_CSUM_INVALID,
> "MMP block checksum does not match MMP block"
>
> +ec EXT2_ET_FILE_EXISTS,
> + "Ext2 file already exists"
> +
> end
> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> new file mode 100644
> index 0000000..3e3a527
> --- /dev/null
> +++ b/lib/ext2fs/symlink.c
> @@ -0,0 +1,161 @@
> +/*
> + * symlink.c --- make a symlink in the filesystem, based on mkdir.c
> + *
> + * Copyright (c) 2012, Intel Corporation.
> + * All Rights Reserved.
> + *
> + * %Begin-Header%
> + * This file may be redistributed under the terms of the GNU Library
> + * General Public License, version 2.
> + * %End-Header%
> + */
> +
> +#include "config.h"
> +#include <stdio.h>
> +#include <string.h>
> +#if HAVE_UNISTD_H
> +#include <unistd.h>
> +#endif
> +#include <fcntl.h>
> +#include <time.h>
> +#if HAVE_SYS_STAT_H
> +#include <sys/stat.h>
> +#endif
> +#if HAVE_SYS_TYPES_H
> +#include <sys/types.h>
> +#endif
> +
> +#include "ext2_fs.h"
> +#include "ext2fs.h"
> +
> +errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
> + const char *name, char *target)
> +{
> + ext2_extent_handle_t handle;
> + errcode_t retval;
> + struct ext2_inode inode;
> + ext2_ino_t scratch_ino;
> + blk64_t blk;
> + int fastlink;
> + int target_len;
> + char *block_buf = 0;
> +
> + EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
> +
> + /* The Linux kernel doesn't allow for links longer than a block */
> + target_len = strlen(target);
> + if (target_len > fs->blocksize) {
> + retval = EXT2_ET_INVALID_ARGUMENT;
> + goto cleanup;
> + }
> +
> + /*
> + * Allocate a data block for slow links
> + */
> + fastlink = (target_len < sizeof(inode.i_block));
> + if (!fastlink) {
> + retval = ext2fs_new_block2(fs, 0, 0, &blk);
> + if (retval)
> + goto cleanup;
> + retval = ext2fs_get_mem(fs->blocksize, &block_buf);
> + if (retval)
> + goto cleanup;
> + }
> +
> + /*
> + * Allocate an inode, if necessary
> + */
> + if (!ino) {
> + retval = ext2fs_new_inode(fs, parent, LINUX_S_IFLNK | 0755,
> + 0, &ino);
> + if (retval)
> + goto cleanup;
> + }
> +
> + /*
> + * Create the inode structure....
> + */
> + memset(&inode, 0, sizeof(struct ext2_inode));
> + inode.i_mode = LINUX_S_IFLNK | 0777;
> + inode.i_uid = inode.i_gid = 0;
> + ext2fs_iblk_set(fs, &inode, 1);
> + inode.i_links_count = 1;
> + inode.i_size = target_len;
> + /* The time fields are set by ext2fs_write_new_inode() */
> +
> + if (fastlink) {
> + /* Fast symlinks, target stored in inode */
> + strcpy((char *)&inode.i_block, target);
> + } else {
> + /* Slow symlinks, target stored in the first block */
> + strcpy(block_buf, target);
> + if (fs->super->s_feature_incompat &
> + EXT3_FEATURE_INCOMPAT_EXTENTS) {
> + /*
> + * The extent bmap is setup after the inode and block
> + * have been written out below.
> + */
> + inode.i_flags |= EXT4_EXTENTS_FL;
Given that you can have at most one data block anyway, does it matter to set
EXTENTS_FL?
Actually, now that I think of it -- any reason not to use the ext2fs_file_*
functions? They'll take care of block allocation, updating the extent tree,
etc, so you don't have to remember how to do that here.
I think you have to ext2fs_write_new_inode() before you can ext2fs_file_open().
Also, what happens to the inode + data block if the ext2fs_link fails? I don't
see any code that explicitly rolls back those allocations, but maybe I missed
something?
--D
> + } else {
> + inode.i_block[0] = blk;
> + }
> + }
> +
> + /*
> + * Write out the inode and inode data block. The inode generation
> + * number is assigned by write_new_inode, which means that the
> + * operations using ino must come after it.
> + */
> + retval = ext2fs_write_new_inode(fs, ino, &inode);
> + if (retval)
> + goto cleanup;
> +
> + if (!fastlink) {
> + retval = io_channel_write_blk(fs->io, blk, 1, block_buf);
> + if (retval)
> + goto cleanup;
> +
> + /* FIXME/VERIFY: Do we need ext2fs_bmap2() here? file ops use
> + * it, mkdir did not */
> +
> + if (fs->super->s_feature_incompat &
> + EXT3_FEATURE_INCOMPAT_EXTENTS) {
> + retval = ext2fs_extent_open2(fs, ino, &inode, &handle);
> + if (retval)
> + goto cleanup;
> + retval = ext2fs_extent_set_bmap(handle, 0, blk, 0);
> + ext2fs_extent_free(handle);
> + if (retval)
> + goto cleanup;
> + }
> + }
> +
> + /*
> + * Link the symlink into the filesystem hierarchy
> + */
> + if (name) {
> + retval = ext2fs_lookup(fs, parent, name, strlen(name), 0,
> + &scratch_ino);
> + if (!retval) {
> + retval = EXT2_ET_FILE_EXISTS;
> + goto cleanup;
> + }
> + if (retval != EXT2_ET_FILE_NOT_FOUND)
> + goto cleanup;
> + retval = ext2fs_link(fs, parent, name, ino, EXT2_FT_SYMLINK);
> + if (retval)
> + goto cleanup;
> + }
> +
> + /*
> + * Update accounting....
> + */
> + if (!fastlink)
> + ext2fs_block_alloc_stats2(fs, blk, +1);
> + ext2fs_inode_alloc_stats2(fs, ino, +1, 1);
> +
> +cleanup:
> + if (block_buf)
> + ext2fs_free_mem(&block_buf);
> + return retval;
> +}
> --
> 1.7.11.7
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists