diff mbox

[v6,1/3] xfs: fix the size of xfs_mode_to_ftype table

Message ID 1483967189-27313-2-git-send-email-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Amir Goldstein Jan. 9, 2017, 1:06 p.m. UTC 
Fix the size of the xfs_mode_to_ftype conversion table,
which was too small to handle an invalid value of mode=S_IFMT.

Use a convenience macro S_DT(mode) to convert from
mode to dirent file type and change the name of the table
to xfs_dtype_to_ftype to correctly describe its index values.

Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c | 18 +++++++++---------
 fs/xfs/libxfs/xfs_dir2.h |  4 +++-
 fs/xfs/xfs_iops.c        |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

Comments

Christoph Hellwig Jan. 9, 2017, 3:51 p.m. UTC | #1 
On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
> Fix the size of the xfs_mode_to_ftype conversion table,
> which was too small to handle an invalid value of mode=S_IFMT.
> 
> Use a convenience macro S_DT(mode) to convert from
> mode to dirent file type and change the name of the table
> to xfs_dtype_to_ftype to correctly describe its index values.

This looks like an awful lot of magic.  Would a switch statement
generate so much worse code?
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amir Goldstein Jan. 9, 2017, 5:21 p.m. UTC | #2 
On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
>> Fix the size of the xfs_mode_to_ftype conversion table,
>> which was too small to handle an invalid value of mode=S_IFMT.
>>
>> Use a convenience macro S_DT(mode) to convert from
>> mode to dirent file type and change the name of the table
>> to xfs_dtype_to_ftype to correctly describe its index values.
>
> This looks like an awful lot of magic.  Would a switch statement
> generate so much worse code?

I doubt it really matters that much, so it's down to a matter of taste.
I personally like the existing map table better than switch if anyone cares ;-)
but I think that the strongest argument in favor of the existing code
is that it works, so no reason to change it.
IMHO, this minor fix and cleanup do not justify switching the code over to
switch statement.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Jan. 9, 2017, 9:17 p.m. UTC | #3 
On Mon, Jan 09, 2017 at 07:21:25PM +0200, Amir Goldstein wrote:
> On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
> >> Fix the size of the xfs_mode_to_ftype conversion table,
> >> which was too small to handle an invalid value of mode=S_IFMT.
> >>
> >> Use a convenience macro S_DT(mode) to convert from
> >> mode to dirent file type and change the name of the table
> >> to xfs_dtype_to_ftype to correctly describe its index values.
> >
> > This looks like an awful lot of magic.  Would a switch statement
> > generate so much worse code?
> 
> I doubt it really matters that much, so it's down to a matter of taste.
> I personally like the existing map table better than switch if anyone cares ;-)
> but I think that the strongest argument in favor of the existing code
> is that it works, so no reason to change it.
> IMHO, this minor fix and cleanup do not justify switching the code over to
> switch statement.

I can't feed a garbage index to a switch statement and have it fly
off the end of an array; plus we can throw asserts in a default: case.

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amir Goldstein Jan. 10, 2017, 7:54 a.m. UTC | #4 
On Mon, Jan 9, 2017 at 11:17 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Jan 09, 2017 at 07:21:25PM +0200, Amir Goldstein wrote:
>> On Mon, Jan 9, 2017 at 5:51 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Mon, Jan 09, 2017 at 03:06:27PM +0200, Amir Goldstein wrote:
>> >> Fix the size of the xfs_mode_to_ftype conversion table,
>> >> which was too small to handle an invalid value of mode=S_IFMT.
>> >>
>> >> Use a convenience macro S_DT(mode) to convert from
>> >> mode to dirent file type and change the name of the table
>> >> to xfs_dtype_to_ftype to correctly describe its index values.
>> >
>> > This looks like an awful lot of magic.  Would a switch statement
>> > generate so much worse code?
>>
>> I doubt it really matters that much, so it's down to a matter of taste.
>> I personally like the existing map table better than switch if anyone cares ;-)
>> but I think that the strongest argument in favor of the existing code
>> is that it works, so no reason to change it.
>> IMHO, this minor fix and cleanup do not justify switching the code over to
>> switch statement.
>
> I can't feed a garbage index to a switch statement and have it fly
> off the end of an array; plus we can throw asserts in a default: case.
>

I think we are in agreement that ASSERT for malformed on-disk data
is a bad idea, as patch 2 demonstrates, and that we are better off
returning -EFSCORRUPTED is such cases.

OK, so just that we are all clear on what I think you are suggesting:

1. retire xfs_mode_to_ftype[] table
2. create helper xfs_mode_to_ftype() that uses a switch statement
    and returns XFS_DIR3_FT_UNKNOWN for invalid modes
3. change xfs_dentry_to_name() to return -EFSCORRUPTED for unknown ftype
    and check return value on call sites where mode come from disk
    (e.g. xfs_generic_create(), xfs_vn_link(), xfs_vn_rename())
4. convert call sites xfs_dentry_to_name(n,d,0) (e.g.
xfs_cleanup_inode()) to use
    a helper xfs_dentry_to_name_unknown(n,d) which sets ftype to unknown
    instead of calling xfs_mode_to_ftype()
5. in xfs_dinode_verify(), sanity check that di_mode is a valid ftype

Should I cook this patch?

Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..48d7c45 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -41,15 +41,15 @@  struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
  * for file type specification. This will be propagated into the directory
  * structure if appropriate for the given operation and filesystem config.
  */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
+const unsigned char xfs_dtype_to_ftype[S_DT_MAX+1] = {
+	[0]                = XFS_DIR3_FT_UNKNOWN,
+	[S_DT(S_IFREG)]    = XFS_DIR3_FT_REG_FILE,
+	[S_DT(S_IFDIR)]    = XFS_DIR3_FT_DIR,
+	[S_DT(S_IFCHR)]    = XFS_DIR3_FT_CHRDEV,
+	[S_DT(S_IFBLK)]    = XFS_DIR3_FT_BLKDEV,
+	[S_DT(S_IFIFO)]    = XFS_DIR3_FT_FIFO,
+	[S_DT(S_IFSOCK)]   = XFS_DIR3_FT_SOCK,
+	[S_DT(S_IFLNK)]    = XFS_DIR3_FT_SYMLINK,
 };
 
 /*
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..4934d38 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -35,7 +35,9 @@  extern struct xfs_name	xfs_name_dotdot;
  * directory filetype conversion tables.
  */
 #define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
+#define S_DT_MAX       S_DT(S_IFMT)
+extern const unsigned char xfs_dtype_to_ftype[];
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..d2da9ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@  xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = xfs_dtype_to_ftype[S_DT(mode)];
 }
 
 STATIC void