Use i_size to determine whether a symlink is a fast symlink

Current way of determining whether a symlink is in fast symlink
format is to call ext2fs_inode_data_blocks2(). If number of data
blocks is zero and EXT4_INLINE_DATA_FL flag is not set, then symlink
data must be in inode->i_block.

This heuristic is becoming increasingly hard to maintain because
inode->i_blocks count can also be incremented for blocks used by
extended attributes. Before ea_inode feature, extra block could come
from xattr block, now more blocks can be added because of xattr
inodes.

To address the issue, add a ext2fs_is_fast_symlink() function that
gives a direct answer based on inode->i_size field. This is
equivalent to kernel's ext4_inode_is_fast_symlink() function.

This patch also fixes a few issues related to fast symlink handling:

  - Both rdump_symlink() and follow_link() interpreted symlinks with
    0 data blocks to always mean fast symlinks. This is incorrect
    because symlinks that are stored as inline data also have
    0 data blocks. Thus, they try to read everything from
    inode->i_block and miss the symlink suffix in inode extra area.

  - e2fsck_pass1_check_symlink() had code to handle inode with
    EXT4_INLINE_DATA_FL flag twice. The first if block always returns
    from the function so the second one is unreachable code.

Signed-off-by: Tahsin Erdogan <tahsin@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index cef4ec2..0a4b536 100644 (file)
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -778,41 +778,31 @@ static void dump_inline_data(FILE *out, const char *prefix, ext2_ino_t inode_num
                fprintf(out, "%sSize of inline data: %zu\n", prefix, size);
 }
 
-static void dump_fast_link(FILE *out, ext2_ino_t inode_num,
-                          struct ext2_inode *inode, const char *prefix)
+static void dump_inline_symlink(FILE *out, ext2_ino_t inode_num,
+                               struct ext2_inode *inode, const char *prefix)
 {
-       errcode_t retval = 0;
-       char *buf;
+       errcode_t retval;
+       char *buf = NULL;
        size_t size;
 
-       if (inode->i_flags & EXT4_INLINE_DATA_FL) {
-               retval = ext2fs_inline_data_size(current_fs, inode_num, &size);
-               if (retval)
-                       goto out;
-
-               retval = ext2fs_get_memzero(size + 1, &buf);
-               if (retval)
-                       goto out;
+       retval = ext2fs_inline_data_size(current_fs, inode_num, &size);
+       if (retval)
+               goto out;
 
-               retval = ext2fs_inline_data_get(current_fs, inode_num,
-                                               inode, buf, &size);
-               if (retval)
-                       goto out;
-               fprintf(out, "%sFast link dest: \"%.*s\"\n", prefix,
-                       (int)size, buf);
+       retval = ext2fs_get_memzero(size + 1, &buf);
+       if (retval)
+               goto out;
 
-               retval = ext2fs_free_mem(&buf);
-               if (retval)
-                       goto out;
-       } else {
-               size_t sz = EXT2_I_SIZE(inode);
+       retval = ext2fs_inline_data_get(current_fs, inode_num,
+                                       inode, buf, &size);
+       if (retval)
+               goto out;
 
-               if (sz > sizeof(inode->i_block))
-                       sz = sizeof(inode->i_block);
-               fprintf(out, "%sFast link dest: \"%.*s\"\n", prefix, (int) sz,
-                       (char *)inode->i_block);
-       }
+       fprintf(out, "%sFast link dest: \"%.*s\"\n", prefix,
+               (int)size, buf);
 out:
+       if (buf)
+               ext2fs_free_mem(&buf);
        if (retval)
                com_err(__func__, retval, "while dumping link destination");
 }
@@ -938,9 +928,12 @@ void internal_dump_inode(FILE *out, const char *prefix,
                fprintf(out, "Inode checksum: 0x%08x\n", crc);
        }
 
-       if (LINUX_S_ISLNK(inode->i_mode) &&
-           ext2fs_inode_data_blocks(current_fs, inode) == 0)
-               dump_fast_link(out, inode_num, inode, prefix);
+       if (LINUX_S_ISLNK(inode->i_mode) && ext2fs_is_fast_symlink(inode))
+               fprintf(out, "%sFast link dest: \"%.*s\"\n", prefix,
+                       (int)EXT2_I_SIZE(inode), (char *)inode->i_block);
+       else if (LINUX_S_ISLNK(inode->i_mode) &&
+                  (inode->i_flags & EXT4_INLINE_DATA_FL))
+               dump_inline_symlink(out, inode_num, inode, prefix);
        else if (LINUX_S_ISBLK(inode->i_mode) || LINUX_S_ISCHR(inode->i_mode)) {
                int major, minor;
                const char *devnote;

diff --git a/debugfs/dump.c b/debugfs/dump.c
index 4d38651..4d5daf0 100644 (file)
--- a/debugfs/dump.c
+++ b/debugfs/dump.c
@@ -208,9 +208,7 @@ static void rdump_symlink(ext2_ino_t ino, struct ext2_inode *inode,
                goto errout;
        }
 
-       /* Apparently, this is the right way to detect and handle fast
-        * symlinks; see do_stat() in debugfs.c. */
-       if (ext2fs_inode_data_blocks2(current_fs, inode) == 0)
+       if (ext2fs_is_fast_symlink(inode))
                strcpy(buf, (char *) inode->i_block);
        else {
                unsigned bytes = inode->i_size;

diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 4ea7962..6deaab2 100644 (file)
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -179,7 +179,6 @@ int e2fsck_pass1_check_symlink(ext2_filsys fs, ext2_ino_t ino,
 {
        unsigned int len;
        int i;
-       blk64_t blocks;
        ext2_extent_handle_t    handle;
        struct ext2_extent_info info;
        struct ext2fs_extent    extent;
@@ -223,12 +222,15 @@ int e2fsck_pass1_check_symlink(ext2_filsys fs, ext2_ino_t ino,
                return 1;
        }
 
-       blocks = ext2fs_inode_data_blocks2(fs, inode);
-       if (blocks) {
-               if (inode->i_flags & EXT4_INLINE_DATA_FL)
+       if (ext2fs_is_fast_symlink(inode)) {
+               if (inode->i_size >= sizeof(inode->i_block))
+                       return 0;
+
+               len = strnlen((char *)inode->i_block, sizeof(inode->i_block));
+               if (len == sizeof(inode->i_block))
                        return 0;
+       } else {
                if ((inode->i_size >= fs->blocksize) ||
-                   (blocks != fs->blocksize >> 9) ||
                    (inode->i_block[0] < fs->super->s_first_data_block) ||
                    (inode->i_block[0] >= ext2fs_blocks_count(fs->super)))
                        return 0;
@@ -247,34 +249,6 @@ int e2fsck_pass1_check_symlink(ext2_filsys fs, ext2_ino_t ino,
                }
                if (len == fs->blocksize)
                        return 0;
-       } else if (inode->i_flags & EXT4_INLINE_DATA_FL) {
-               char *inline_buf = NULL;
-               size_t inline_sz = 0;
-
-               if (ext2fs_inline_data_size(fs, ino, &inline_sz))
-                       return 0;
-               if (inode->i_size != inline_sz)
-                       return 0;
-               if (ext2fs_get_mem(inline_sz + 1, &inline_buf))
-                       return 0;
-               i = 0;
-               if (ext2fs_inline_data_get(fs, ino, inode, inline_buf, NULL))
-                       goto exit_inline;
-               inline_buf[inline_sz] = 0;
-               len = strnlen(inline_buf, inline_sz);
-               if (len != inline_sz)
-                       goto exit_inline;
-               i = 1;
-exit_inline:
-               ext2fs_free_mem(&inline_buf);
-               return i;
-       } else {
-               if (inode->i_size >= sizeof(inode->i_block))
-                       return 0;
-
-               len = strnlen((char *)inode->i_block, sizeof(inode->i_block));
-               if (len == sizeof(inode->i_block))
-                       return 0;
        }
        if (len != inode->i_size)
                if ((inode->i_flags & EXT4_ENCRYPT_FL) == 0)
@@ -1911,7 +1885,7 @@ void e2fsck_pass1(e2fsck_t ctx)
                        if (inode->i_flags & EXT4_INLINE_DATA_FL) {
                                FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
                                continue;
-                       } else if (ext2fs_inode_data_blocks(fs, inode) == 0) {
+                       } else if (ext2fs_is_fast_symlink(inode)) {
                                ctx->fs_fast_symlinks_count++;
                                check_blocks(ctx, &pctx, block_buf,
                                             ea_ibody_quota_blocks);

diff --git a/lib/ext2fs/alloc.c b/lib/ext2fs/alloc.c
index af21410..3fd9216 100644 (file)
--- a/lib/ext2fs/alloc.c
+++ b/lib/ext2fs/alloc.c
@@ -353,10 +353,11 @@ blk64_t ext2fs_find_inode_goal(ext2_filsys fs, ext2_ino_t ino,
        ext2_extent_handle_t    handle = NULL;
        errcode_t               err;
 
-       if (inode == NULL || ext2fs_inode_data_blocks2(fs, inode) == 0)
-               goto no_blocks;
-
-       if (inode->i_flags & EXT4_INLINE_DATA_FL)
+       /* Make sure data stored in inode->i_block is neither fast symlink nor
+        * inline data.
+        */
+       if (inode == NULL || ext2fs_is_fast_symlink(inode) ||
+           inode->i_flags & EXT4_INLINE_DATA_FL)
                goto no_blocks;
 
        if (inode->i_flags & EXT4_EXTENTS_FL) {

diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 90df218..b734f1a 100644 (file)
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1611,6 +1611,7 @@ errcode_t ext2fs_unlink(ext2_filsys fs, ext2_ino_t dir, const char *name,
 /* symlink.c */
 errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino,
                         const char *name, const char *target);
+int ext2fs_is_fast_symlink(struct ext2_inode *inode);
 
 /* mmp.c */
 errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf);

diff --git a/lib/ext2fs/namei.c b/lib/ext2fs/namei.c
index 307aecc..d485a72 100644 (file)
--- a/lib/ext2fs/namei.c
+++ b/lib/ext2fs/namei.c
@@ -50,7 +50,21 @@ static errcode_t follow_link(ext2_filsys fs, ext2_ino_t root, ext2_ino_t dir,
        if (link_count++ >= EXT2FS_MAX_NESTED_LINKS)
                return EXT2_ET_SYMLINK_LOOP;
 
-       if (ext2fs_inode_data_blocks(fs,&ei)) {
+       if (ext2fs_is_fast_symlink(&ei))
+               pathname = (char *)&(ei.i_block[0]);
+       else if (ei.i_flags & EXT4_INLINE_DATA_FL) {
+               retval = ext2fs_get_memzero(ei.i_size, &buffer);
+               if (retval)
+                       return retval;
+
+               retval = ext2fs_inline_data_get(fs, inode,
+                                               &ei, buffer, NULL);
+               if (retval) {
+                       ext2fs_free_mem(&buffer);
+                       return retval;
+               }
+               pathname = buffer;
+       } else {
                retval = ext2fs_bmap2(fs, inode, &ei, NULL, 0, 0, NULL, &blk);
                if (retval)
                        return retval;
@@ -65,8 +79,8 @@ static errcode_t follow_link(ext2_filsys fs, ext2_ino_t root, ext2_ino_t dir,
                        return retval;
                }
                pathname = buffer;
-       } else
-               pathname = (char *)&(ei.i_block[0]);
+       }
+
        retval = open_namei(fs, root, dir, pathname, ei.i_size, 1,
                            link_count, buf, res_inode);
        if (buffer)

diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
index 23a8570..b9d8f55 100644 (file)
--- a/lib/ext2fs/swapfs.c
+++ b/lib/ext2fs/swapfs.c
@@ -210,18 +210,24 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
                            struct ext2_inode_large *f, int hostorder,
                            int bufsize)
 {
-       unsigned i, has_data_blocks, extra_isize, attr_magic;
-       int has_extents = 0;
-       int has_inline_data = 0;
-       int islnk = 0;
+       unsigned i, extra_isize, attr_magic;
+       int has_extents, has_inline_data, islnk, fast_symlink;
        int inode_size;
        __u32 *eaf, *eat;
 
-       if (hostorder && LINUX_S_ISLNK(f->i_mode))
-               islnk = 1;
+       /*
+        * Note that t and f may point to the same address. That's why
+        * if (hostorder) condition is executed before swab calls and
+        * if (!hostorder) afterwards.
+        */
+       if (hostorder) {
+               islnk = LINUX_S_ISLNK(f->i_mode);
+               fast_symlink = ext2fs_is_fast_symlink(EXT2_INODE(f));
+               has_extents = (f->i_flags & EXT4_EXTENTS_FL) != 0;
+               has_inline_data = (f->i_flags & EXT4_INLINE_DATA_FL) != 0;
+       }
+
        t->i_mode = ext2fs_swab16(f->i_mode);
-       if (!hostorder && LINUX_S_ISLNK(t->i_mode))
-               islnk = 1;
        t->i_uid = ext2fs_swab16(f->i_uid);
        t->i_size = ext2fs_swab32(f->i_size);
        t->i_atime = ext2fs_swab32(f->i_atime);
@@ -231,27 +237,21 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
        t->i_gid = ext2fs_swab16(f->i_gid);
        t->i_links_count = ext2fs_swab16(f->i_links_count);
        t->i_file_acl = ext2fs_swab32(f->i_file_acl);
-       if (hostorder)
-               has_data_blocks = ext2fs_inode_data_blocks(fs,
-                                          (struct ext2_inode *) f);
        t->i_blocks = ext2fs_swab32(f->i_blocks);
-       if (!hostorder)
-               has_data_blocks = ext2fs_inode_data_blocks(fs,
-                                          (struct ext2_inode *) t);
-       if (hostorder && (f->i_flags & EXT4_EXTENTS_FL))
-               has_extents = 1;
-       if (hostorder && (f->i_flags & EXT4_INLINE_DATA_FL))
-               has_inline_data = 1;
        t->i_flags = ext2fs_swab32(f->i_flags);
-       if (!hostorder && (t->i_flags & EXT4_EXTENTS_FL))
-               has_extents = 1;
-       if (!hostorder && (t->i_flags & EXT4_INLINE_DATA_FL))
-               has_inline_data = 1;
        t->i_size_high = ext2fs_swab32(f->i_size_high);
+
+       if (!hostorder) {
+               islnk = LINUX_S_ISLNK(t->i_mode);
+               fast_symlink = ext2fs_is_fast_symlink(EXT2_INODE(t));
+               has_extents = (t->i_flags & EXT4_EXTENTS_FL) != 0;
+               has_inline_data = (t->i_flags & EXT4_INLINE_DATA_FL) != 0;
+       }
+
        /*
         * Extent data and inline data are swapped on access, not here
         */
-       if (!has_extents && !has_inline_data && (!islnk || has_data_blocks)) {
+       if (!has_extents && !has_inline_data && (!islnk || !fast_symlink)) {
                for (i = 0; i < EXT2_N_BLOCKS; i++)
                        t->i_block[i] = ext2fs_swab32(f->i_block[i]);
        } else if (t != f) {

diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
index 0e6f9a9..607396d 100644 (file)
--- a/lib/ext2fs/symlink.c
+++ b/lib/ext2fs/symlink.c
@@ -174,3 +174,14 @@ cleanup:
                ext2fs_free_mem(&block_buf);
        return retval;
 }
+
+/*
+ * Test whether an inode is a fast symlink.
+ *
+ * A fast symlink has its symlink data stored in inode->i_block.
+ */
+int ext2fs_is_fast_symlink(struct ext2_inode *inode)
+{
+       return LINUX_S_ISLNK(inode->i_mode) && EXT2_I_SIZE(inode) &&
+              EXT2_I_SIZE(inode) < sizeof(inode->i_block);
+}

diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index 956348f..27c94a6 100644 (file)
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -863,8 +863,9 @@ static int op_readlink(const char *path, char *buf, size_t len)
        len--;
        if (inode.i_size < len)
                len = inode.i_size;
-       if (ext2fs_inode_data_blocks2(fs, &inode) ||
-           (inode.i_flags & EXT4_INLINE_DATA_FL)) {
+       if (ext2fs_is_fast_symlink(&inode))
+               memcpy(buf, (char *)inode.i_block, len);
+       else {
                /* big/inline symlink */
 
                err = ext2fs_file_open(fs, ino, 0, &file);
@@ -888,9 +889,7 @@ out2:
                        ret = translate_error(fs, ino, err);
                        goto out;
                }
-       } else
-               /* inline symlink */
-               memcpy(buf, (char *)inode.i_block, len);
+       }
        buf[len] = 0;
 
        if (fs_writeable(fs)) {